Re: [llnl-devel] Re: [Freeipmi-devel] kcs byte array model - validation

[Mark A. Grondona]

>>>>> On Wed, 10 Dec 2003,
>>>>> "ben" == Ben Woodard wrote:

  ben> On Wed, 2003-12-10 at 08:55, Albert Chu wrote:

  +> Ben says he prefers having the marshalling functions malloc() a buffer
  +> instead of us passing a buffer in.  I personally prefer passing in a
  +> buffer and declaring a global macro IPMI_CMD_MAX_LEN.  We can debate
  +> about this.

  ben> The reason that I prefer this is the function who really knows what size
  ben> the thing is going to be on the wire is the marshal function. No one
  ben> else knows exactly how big it has to be. So having it malloc the right
  ben> sized buffer is clean and efficient.

  ben> In the case where you pass in a buffer, there is no guarantee that the
  ben> caller actually passed in a good buffer, and a buffer that is the right
  ben> size. Even though you may have defined IPMI_CMD_MAX_LEN there is no way
  ben> to enforce that people use it. In practice people often times do things
  ben> like, "how big should this be. Well it can't be over 1KB so. char
  ben> buf[1000];" They never even look for something like IPMI_CMD_MAX_LEN
  ben> even if you put it in the documentation. The problem doesn't arise if
  ben> the user wisely assumes something like 1000 (and waste gobs of space)
  ben> but say for whatever reason they decide that they know that an ipmi
  ben> packet is 64 bytes and so they make a 64 byte buffer but in some cases
  ben> the buffer really needs to be 68 bytes.

  ben> Plus if they do pass in too small of a buffer for the marshal function
  ben> to work. How do they recover from the error. Most of the time
  ben> programmers will not take the time to write something that recovers
  ben> gracefully. They will write something like:

  ben> char buf[60];
  ben> int len=marshall(&data,buf,buflen);
  ben> write(fd,buf,buflen);

Will user's of the freeipmi API be calling the marshall and unmarshall
functions directly? If so, you might want to think about whether that
is strictly necessary. A nice API would abstract marshall/buffer/send
operations from the user, so they do not even have to worry that
their structure/request is being packed up for the network. An especially
nice API would wrap your two operations above into one:

 /* send a request foo to the foo server s */
 foo_send_req (foo_server_t s, struct foo_req_t * foo);

This hides the actual protocol implementation from the user of the API,
who frankly doesn't care how the data gets from here to there. 

In this case, the argument of whether to use a buffer allocated on
the heap or the stack is irrelevant. In fact, it could change without
breaking the API.

If the API user needs to do something between marshalling the packet
data and sending it over the wire, and thus the above abstraction
won't work, you might want to think about why they need to do that,
and whether you can alleviate their need in some other way.

mark


  ben> So now the write is failing because it is getting a size of -1 and a
  ben> programmer really has to look to find the reason why.

  ben> Also, say they pass in a bogus pointer. When they try to debug the
  ben> program it will initially look like it failed in your library and they
  ben> will spend quite a while tracing through your code to figure out that
  ben> the problem is actually in their program.

  ben> Also I have seen at least one case where someone wrote some code where
  ben> they made the packet buffer a file static variable in their kind of
  ben> middle level library and forgot about it. Then they later on redid their
  ben> program so that it was multithreaded. Every so often the program would
  ben> break in weird ways and eventually I tracked it down to this global
  ben> buffer being reused in a weird race condition. Sure it was a bug on
  ben> their part and using a file static variable for something like a buffer
  ben> for a packet isn't the wisest thing but many people do it. However, the
  ben> fact that the lower level library didn't allocate its own memory
  ben> provoked the problem.

  ben> So those are my reasons for suggesting that the marshal function mallocs
  ben> the buffer.