[Freeipmi-devel] Re: [llnl-devel] Sounds good ... I'll start adding the packet dump functions into a new

[Ben Woodard]

AB, I think that the big difference here is we tend to look at IPMI as a
network protocol which also can be run locally and from talking to Al
about the API changes that you have been making, it seems like you view
IPMI as something that is primarily run locally but which can sometimes
be run over the network.

On Wed, 2003-12-03 at 05:08, Anand Babu wrote:
> Let us restart the debate:
> 
> "struct / #pragma pack(1)" vs "byte array"
> ==========================================
> 
> "struct / #pragma pack(1)"
> --------------------------
>  - Elegant

More correctly:
- Oversimplistic
- Compiler dependent
- Indicative of inexperienced programmers who haven't fixed numerous
bugs in software that has been ported around from arch to arch.

> 
> "byte array"
> ------------
>  - Most Portable - across compilers, platforms
> 
> Nobody likes the "byte array" model. Code is utterly unreadable. 
> But when portability is #1 goal, there is no choice.

This is not true.

First of all most experienced network programmers prefer it because they
have been bitten by those kinds of oversimplifications too many times.
Therefore, your assertion that nobody likes it is patently wrong. This
morning over coffee we did a sort of ad-hoc poll of all the systems
programmers that happened to be there. 6 out of 6 of us all consider the
additional step of marshaling the packet before it is sent over the
network rather than simply writing the structure out to a packet
mandatory. 

Second, the argument that the code is unreadable is simply a function of
how well you write the code. If you write the code in a way that it is 
unreadable, then it will be unreadable. If you are careful to write code
that is reasonable then it will be readable.

One way to keep the code really clean and readable is to make sure that
you only marshal right before you send it out on the wire and
immediately after you pick it up off of the wire. So 99% of the time you
are working with the normal structures and then just before you send it,
you marshal a packet from the structures.

> 
> I will support "struct / #pragma pack" approach, until someone
> convinces me with clear examples when and where this model will break,
> how often we will hit them.

The very obvious example is if someone compiles freeipmi on a sparc
station or a AIX box or any big endian machine then tries to send a
packet from that machine to an ia64 box to shut it down using ipmi
power. In the real world SA's run on who knows what and they are
controlling a variety of different machines. 

Right here at the lab we have SAs which work from their Mac laptops.

Even with #pragma pack(1) the compiler can:
1) ignore the pragma if it wants.
2) reorder the members of a structure
3) reorder the arrangement of the bit fields

pragma is a suggestion to the compiler it is not a guarantee.

Also there are other fairly good reasons for using marshaling and
unmarshaling the packets rather than having the structures you use in
your program be tightly tied to the network protocol.

1) having the structures be exact representations of wire protocol puts
your data structures into a straight jacket. You can't add anything to
them if you find that you need something later on in the development of
the program. For example say later on for some reason you find it useful
to add a reference count to the data structure. You can't do that
because it would mess up your wire representation. Or some time later
you want to put the data structures on a linked list. Simply adding a
next member wouldn't be as easy if you can't mess with the structure
without breaking the wire protocol.

2) protocol changes - say version 2 of the protocol comes out. It would
be useful to be able to use the same data structures and possibly have
two different packet representations that are sent over the wire. Both
contain most of the same information but maybe V2 includes more
information than is in V1.

3) protocol variations - right now you are only running and testing
against one kind of hardware but down the road it is not unlikely that
you will run into some hardware that doesn't work exactly the same way
and you need to put in some sort of variation or kludge to work with the
new hardware without breaking the old format. Having your data
structures tightly bound to the wire protocol makes this very very
difficult.


> 
> -ab
> 
> Ben Woodard <address@hidden> writes:
> 
> >Say I had a structure like:
> >
> >struct foo{
> >     int type;
> >     char status;
> >     unsigned int code;
> >     char name[5];
> >     char reg_a:1;
> >     char reg_b:3;
> >};
> >
> >#define PKTFOO_NAME_OFF 0
> >#define PKTFOO_TYPE_OFF 5
> >#define PKTFOO_CODE_OFF 7
> >#define PKTFOO_STATUS_OFF 9
> >#define PKTFOO_REGISTER_OFF 10
> >#define PKTFOO_A_BITOFF 1
> >#define PKTFOO_B_BITOFF 2
> >
> >inline void marshal_string( char *buf, unsigned int offset, const char
> >*str, unsigned int len){
> >     memcpy(buf+offset,str,len);
> >}
> >
> >inline void marchal_2byte_int_be( char *buf, unsigned int offset, int
> >val){
> >     int num=val&0xff00;
> >     buf[offset]=num;
> >     num=val&0xff;
> >     buf[offset+1]=num;
> >}
> >
> >...
> >
> >On Sat, 2003-11-15 at 10:07, Albert Chu wrote:
> >> ipmi_error.c file at some point.
> >> 
> >> > We should take packing effect seriously and fix them, before the code
> >> > evolves further.
> >> >
> >> > I'm not having a clear picture of how you are going to implement
> >> > marshalling. Can you throw me an example code.
> >> 
> >> Ben Woodard has far more experience in this area than me, b/c he worked
> >> on porting/fixing lpd for a number of years.  But he suggests something
> >> along the lines of:
> >> 
> >> marshall_ipmi_pkt(ipmi_pkt *pkt, char *buf, int buflen) {
> >>    memcpy(&buf[0], pkt->char_val, 1);
> >>    memcpy(&buf[1], to_little_endian(pkt->int_val), 4);
> >>    memcpy(&buf[5], pkt->netfn << 2 | pkt->lun, 1);
> >> }
> >> 
> >> Unmarshalling would be the opposite, putting values into the appropriate
> >> fields.  Its painful and mind numbing to code, but several developers
> >> agree that it must be done to avoid problems down the road.
> >> 
> >> I found yet another reason to do the marshalling and unmarshalling.
> >> 
> >> struct foo {
> >> RMCP bug??
> >> X-Accept-Language: en
> >> Content-Type: text/plain; charset=us-ascii
> >> Content-Disposition: inline
> >> Content-Transfer-Encoding: 7bit
> >> 
> >> Hey AB,
> >> 
> >> As far as I can tell, you are never converting the RMCP IANA Enterprise
> >> number from little endian to big endian, and RMCP packets are required
> >> to be in big endian form (12.3.1)...  Is it working??  Perhaps Intel
> >> made their chips handle both??
> >> 
> >> --
> >> Albert Chu
> >> address@hidden
> >> Lawrence Livermore National Laboratories
> >> 
> >> ----- Original Message -----
> >> From: Albert Chu <address@hidden>
> >> Date: Saturday, November 15, 2003 9:43 am
> >> Subject: [Freeipmi-devel] Re: [llnl-devel] FreeIPMI conflict/issue list
> >> 
> >> > > Albert Chu <address@hidden> writes:
> >> > > I think we should remove ipmi_errno and rename the file to
> >> > > ipmi_error.c
> >> > > 
> >> > > You tell me what you are going to fix. I wont touch that part of the
> >> > > code. :)
> >> > 
> >> > Sounds good ...  I'll start adding the packet dump functions into a 
> >> > newipmi_error.c file at some point.
> >> > 
> >> > > We should take packing effect seriously and fix them, before the 
> >> > code> evolves further.
> >> > > 
> >> > > I'm not having a clear picture of how you are going to implement
> >> > > marshalling. Can you throw me an example code.
> >> > 
> >> > Ben Woodard has far more experience in this area than me, b/c he 
> >> > workedon porting/fixing lpd for a number of years.  But he suggests 
> >> > somethingalong the lines of:
> >> > 
> >> > marshall_ipmi_pkt(ipmi_pkt *pkt, char *buf, int buflen) {
> >> >   memcpy(&buf[0], pkt->char_val, 1);
> >> >   memcpy(&buf[1], to_little_endian(pkt->int_val), 4);
> >> >   memcpy(&buf[5], pkt->netfn << 2 | pkt->lun, 1);
> >> > }
> >> > 
> >> > Unmarshalling would be the opposite, putting values into the 
> >> > appropriatefields.  Its painful and mind numbing to code, but 
> >> > several developers
> >> > agree that it must be done to avoid problems down the road.
> >> > 
> >> > I found yet another reason to do the marshalling and unmarshalling.
> >> > 
> >> > struct foo {
> >> >   int a: 2;
> >> >   int b: 6;
> >> > };
> >> > 
> >> > On some compilers/machines/whatever foo's will be aligned "a:b" in
> >> > memory, while others align it "b:a" ... 
> >> > 
> >> > Al
> >> > 
> >> > --
> >> > Albert Chu
> >> > address@hidden
> >> > Lawrence Livermore National Laboratories
> >> > 
> >> > ----- Original Message -----
> >> > From: Anand Babu <address@hidden>
> >> > Date: Friday, November 14, 2003 4:05 pm
> >> > Subject: Re: [llnl-devel] FreeIPMI conflict/issue list
> >> > 
> >> > > Albert Chu <address@hidden> writes:
> >> > > >I didn't even notice the ipmi_errno variable.  Is this even 
> >> > > necessary? 
> >> > > >I would think this is only necessary if we wrote much higher level
> >> > > >functions, such as "ipmi_setup_session" or something.  In the 
> >> > current> >framework errors will exist in packet completion codes or in
> >> > > >sendto/recvfrom.  I feel its ok for errors from sendto/recvfrom 
> >> > to be
> >> > > >passed back to the user via libc errno.  I still don't know your 
> >> > > code as
> >> > > >well as you, so you perhaps see something I don't see.
> >> > > 
> >> > > I think we should remove ipmi_errno and rename the file to
> >> > > ipmi_error.c
> >> > > 
> >> > > You tell me what you are going to fix. I wont touch that part of the
> >> > > code. :)
> >> > > 
> >> > > >Although its painful, and a lot of code, ultimately I think this 
> >> > > is the
> >> > > >safest approach.  The compiler aligns the structures in the 
> >> > memory 
> >> > > just>like we want them to, but there is no guarantee they'll do 
> >> > > that in the
> >> > > >future.  
> >> > > >
> >> > > >An added bonus is that marshall/unmarshall functions will allow 
> >> > us to
> >> > > >fix endian issues.  We have a lot of IBM laptops floating 
> >> > around.  If
> >> > > >any of the sysadmins want to use ipmipower on those laptops, 
> >> > > libfreeipmi>will completly bomb right now.
> >> > > 
> >> > > We should take packing effect seriously and fix them, before the 
> >> > code> evolves further.
> >> > > 
> >> > > I'm not having a clear picture of how you are going to implement
> >> > > marshalling. Can you throw me an example code.
> >> > > 
> >> > > 
> >> > > Happy Hacking
> >> > > -- 
> >> > > Anand Babu
> >> > > CaliforniaDigital.com
> >> > > Office# +1-510-687-7045
> >> > > Cell# +1-510-396-0717
> >> > > Home# +1-510-894-0586
> >> > > 
> >> > > Free as in Freedom <www.gnu.org>
> >> > > 
> >> > 
> >> > 
> >> > 
> >> > _______________________________________________
> >> > Freeipmi-devel mailing list
> >> > address@hidden
> >> > http://mail.nongnu.org/mailman/listinfo/freeipmi-devel
> >> > 
> >> 
> >> 
> >> _______________________________________________
> >> llnl-devel mailing list
> >> address@hidden
> >> http://californiadigital.com/cgi-bin/mailman/listinfo/llnl-devel
> >-- 
> >Ben Woodard <address@hidden>
> >Red Hat Inc.
> >
> >
> >_______________________________________________
> >llnl-devel mailing list
> >address@hidden
> >http://californiadigital.com/cgi-bin/mailman/listinfo/llnl-devel
-- 
Ben Woodard <address@hidden>
Red Hat Inc.