Re: [Freeipmi-devel] FIID Re-Implementation

[Albert Chu]

Just a few comment I'd make about the new interface as I'm discovering
issues as I transition tools/the rest of the library to the new interface.

1) Due to packet fields being optional/variable length, byte boundary
restrictions must be put into place when reading/writing blocks of data.
 For example, suppose we the following template:

{4, "foo"},
{4, "bar"},
{8, "foobar"},

Suppose I set a 4 bits of the field "foo", 0 bits in "bar", and all 8
bits of "foobar".  When I call the fiid_obj_get_all() function to get
all of the packet data, I would have a packet with 12 bits of data,
which doesn't make any sense.  Since I can't make assumptions about what
the other 4 bits in that byte actually are, I currently return an error
to the user.

2) All "reserved" fields and previously ignored fields must now be set.
 For example, a number of packets have reserved fields like:

{2, "foo"},
{2, "reserved"},
{4, "bar"},

In the past, we could safely ignore the reserved field.  But now that
fields are optional/variable length, if the reserved field is not
actually set, we would assume its optional.

I originally proposed redoing fiid templates like so:

#define FIID_FIELD_REQUIRED         0x0
#define FIID_FIELD_OPTIONAL         0x1
#define FIID_FIELD_DEPENDENT        0x2

#define FIID_FIELD_FIXED_LENGTH     0x0
#define FIID_FIELD_VARIABLE_LENGTH  0x1

typedef struct fiid_field
{
  uint32_t max_field_len;
  char key[FIID_FIELD_MAX];
  uint8_t requirement_flag;
  uint8_t length_flag;
} fiid_field_t;

But later changed my mind to not change anything.  I'm now debating if
it'd be wise to go back to this model.  It'll be more painful to re-do
all of the templates, but could be better longer term??  We would add an
additional #define to specify fields as RESERVED, indicating that they
are "automatically" set with 0s.

Al

--
Albert Chu
address@hidden
925-422-5311
Computer Scientist
High Performance Systems Division
Lawrence Livermore National Laboratory


----- Original Message -----
From: Albert Chu <address@hidden>
Date: Thursday, January 19, 2006 9:00 am
Subject: Re: [Freeipmi-devel] FIID Re-Implementation

> Just a bit of an update so people can make comments on the current
> direction: 
> 
> typedef struct fiid_field
> {
>  uint32_t max_field_len;
>  char key[FIID_FIELD_MAX];
> } fiid_field_t;
> 
> typedef fiid_field_t fiid_template_t[];
> 
> struct fiid_field_data
> {
>  uint32_t max_field_len;
>  char key[FIID_FIELD_MAX];
>  unsigned int field_len;
> };
> 
> struct fiid_obj
> {
>  uint32_t magic;
>  uint8_t *data;
>  unsigned int data_len;
>  struct fiid_field_data *field_data;
>  unsigned int field_data_len;
> };
> 
> int8_t fiid_template_field_lookup (fiid_template_t tmpl, uint8_t 
> *field);int8_t fiid_obj_field_lookup (fiid_obj_t obj, uint8_t *field);
> fiid_obj_t fiid_obj_create (fiid_template_t tmpl);
> int8_t fiid_obj_destroy (fiid_obj_t obj);
> fiid_obj_t fiid_obj_dup (fiid_obj_t src_obj);
> int8_t fiid_obj_clear (fiid_obj_t obj);
> int8_t fiid_obj_clear_field (fiid_obj_t obj, uint8_t *field);
> int8_t fiid_obj_set (fiid_obj_t obj, uint8_t *field, uint64_t val);
> int8_t fiid_obj_get (fiid_obj_t obj, uint8_t *field, uint64_t *val);
> int8_t fiid_obj_set_data (fiid_obj_t obj, uint8_t *field, uint8_t 
> *data,uint32_t data_len);
> int8_t fiid_obj_get_data (fiid_obj_t obj, uint8_t *field, uint8_t 
> *data,uint32_t data_len);
> int8_t fiid_obj_set_block (fiid_obj_t obj, uint8_t *field_start, 
> uint8_t*field_end, uint8_t *data, uint32_t data_len);
> int8_t fiid_obj_get_block (fiid_obj_t obj, uint8_t *field_start, 
> uint8_t*field_end, uint8_t *data, uint32_t data_len);
> 
> A few comments:
> 
> 1) Now that we're moving to abstract away the internal 
> implementation of
> fiid obj's, many of the original functions like fiid_obj_field_start()
> are static functions and are hidden from the user.
> 
> 2) I believe the choice to call the early interface functions
> "fiid_obj_malloc", "fiid_obj_free", and "fiid_obj_memset", were based
> upon the fact that it was known that a fiid_obj_t was a void * 
> pointer. Now that we have abstracted that away, the function have 
> been renamed
> "fiid_obj_create", "fiid_obj_destroy", and "fiid_obj_clear".  This 
> is a
> semantic issue and I would be willing to change this if people 
> disagree.  
> 
> 3) After a fiid object is created, the templates are never passed 
> aroundanymore.  
> 
> 4) I'm going to develop an iterator interface for this API soon too.
> 
> Al
> 
> --
> Albert Chu
> address@hidden
> 925-422-5311
> Computer Scientist
> High Performance Systems Division
> Lawrence Livermore National Laboratory
> 
> 
> ----- Original Message -----
> From: Albert Chu <address@hidden>
> Date: Thursday, January 12, 2006 1:45 pm
> Subject: Re: [Freeipmi-devel] FIID Re-Implementation
> 
> > 
> > > typedef struct fiid_field
> > > {
> > >  uint32_t max_field_len;
> > >  char key[FIID_FIELD_MAX];
> > >  uint8_t requirement_flag;
> > >  uint8_t length_flag;
> > > } fiid_field_t;
> > 
> > Hmmm.  Thinking about this again.  I'm not sure the flags are even
> > necessary.  I believe the 'len' field of a 'struct fiid_setting' 
> > will be
> > able to take care of both of these issues.  If the 'len' written 
> to 
> > thefiid_obj_t is zero, then clearly it is optional or dependent and
> > shouldn't be sent in the packet.  Whatever 'len' it is set to will
> > clearly determine if it is variable length or not.
> > 
> > Al
> > 
> > --
> > Albert Chu
> > address@hidden
> > 925-422-5311
> > Computer Scientist
> > High Performance Systems Division
> > Lawrence Livermore National Laboratory
> > 
> > 
> > ----- Original Message -----
> > From: Albert Chu <address@hidden>
> > Date: Thursday, January 12, 2006 8:45 am
> > Subject: [Freeipmi-devel] FIID Re-Implementation
> > 
> > > Howdy all,
> > > 
> > > I would like to discuss this topic from the FreeIPMI meeting 
> more 
> > > deeply.
> > > I would like to propose we leave my ipmi 2.0 branch 
> > > (al_ipmi_2_0_branch)sitting.  I think it's best not to merge it 
> > > into the head until our
> > > agreed upon objectives for the next generation library are met 
> > for 
> > > ipmi1.5 first.  I am open to discussion about this too though.
> > > 
> > > Fiid Re-Architecture:
> > > ---------------------
> > > 
> > > As was discussed at the meeting, there are two major problems now
> > > showing up due to IPMI 2.0, many more "optional" or "dependent" 
> > fields> exist and many are variable length. Some examples:
> > > 
> > > - The length of authcode/integrity fields are dependent on the 
> > digest> algorithm used.
> > > 
> > > - OEM fields are sent dependent on if the payload is 
> specifically 
> > > citedas a OEM type.
> > > 
> > > The following is my proposal for the fiid re-architecture.  It 
> > > attemptsto abstract information far more to deal with the above 
> > > issues and
> > > issues with later IPMI versions.
> > > 
> > > #defines and structure re-definitions
> > > -------------------------------------
> > > 
> > > #define FIID_FIELD_REQUIRED         0x0
> > > #define FIID_FIELD_OPTIONAL         0x1
> > > #define FIID_FIELD_DEPENDENT        0x2
> > > 
> > > #define FIID_FIELD_FIXED_LENGTH     0x0
> > > #define FIID_FIELD_VARIABLE_LENGTH  0x1
> > > 
> > > typedef struct fiid_field
> > > {
> > >  uint32_t max_field_len;
> > >  char key[FIID_FIELD_MAX];
> > >  uint8_t requirement_flag;
> > >  uint8_t length_flag;
> > > } fiid_field_t;
> > > 
> > > typedef fiid_field_t const fiid_template_t[];
> > > 
> > > struct fiid_setting
> > > {
> > >  fiid_field_t field;
> > >  uint32_t len;
> > > }
> > > 
> > > struct fiid_obj
> > > {
> > >  void *data;
> > >  unsigned int data_len;
> > >  struct fiid_setting *settings;
> > >  unsigned int settings_len;
> > > };
> > > 
> > > struct struct fiid_obj *fiid_obj_t;
> > > 
> > > Notes:
> > > ------
> > > 
> > > fiid_field_t now includes a flags.  One flag will determine if 
> > the 
> > > fieldin the template is required, dependent, or optional.  
> > > Dependent means
> > > some factor determines if the field should be sent. An example 
> of 
> > this> is the authcode field of an IPMI 1.5 packet.  It is only 
> sent 
> > if if 
> > > theauth-type is not NONE.  An optional field is something that 
> > simply> optional.  Off the top of my head, the last byte in a 
> "Get 
> > Chassis> Status" command is optional.  The other flag is to 
> > determine if the 
> > > thefield is fixed or variable length.
> > > 
> > > fiid_obj_t holds the data, the data length for bounds checking, 
> > object> settings, and the length of the settings array.
> > > 
> > > The fiid_obj_settings_t contains the data in the fields of a
> > > fiid_template_t. Note, that I have currently elected to not have
> > > pointers back to the template, because users have the ability to
> > > alter/create/destroy templates through fiid_template_make.  
> > > Instead, the
> > > data from the template will be copied over.
> > > 
> > > It also includes a len field to indicate how much data has been 
> > copied> into the object for this particular field.  So if the len 
> > is 0, don't
> > > copy the data into a packet (i.e. perhaps the data was 
> optional). 
> > This> len variable fixes the variable length issue of some fields 
> > in IPMI 
> > > 2.0. By abstracting away the "settings" in the above format, 
> this 
> > > shouldhopefully make things more extensible in future versions 
> of 
> > > IPMI.
> > > fiid interface:
> > > ---------------
> > > 
> > > I currently don't imagine a need to alter the fiid API 
> (although 
> > some> changes may be done for cleanup if we decide to).  Here's a 
> > list of
> > > changes that would have to be done underneath in the code:
> > > 
> > > fiid_obj_calloc/alloc/malloc - Must build fiid_obj_settings_t 
> > array 
> > > and build structure.  
> > > 
> > > fiid_obj_free - Must free new structures appropriately.
> > > 
> > > fiid_obj_memset/fiid_obj_memset_field - Must clear 'len' field in
> > > fiid_obj_settings_t array.
> > > 
> > > fiid_obj_set/fiid_obj_set_data - Must set 'len' field 
> appropriately.> > 
> > > fiid_obj_get/fiid_obj_get_data - Must account for variable 'len'
> > > appropriately.
> > > 
> > > fiid_obj_set/fiid_obj_get/fiid_obj_set_data/fiid_obj_get_data: The
> > > template is no longer required.  Would we elect to remove it 
> from 
> > > the API??
> > > 
> > > libfreeipmi changes:
> > > --------------------
> > > 
> > > Various code logic must be altered to appropriately use the fiid
> > > interface and abstract away the new structures:
> > > 
> > > i.e.
> > > 
> > > memcpy(pkt,obj + field_offset,field_len)
> > > 
> > > would have to change to:
> > > 
> > > fiid_obj_get_data(obj,tmpl,field_name,pkt,pkt_len)
> > > 
> > > Memsetting of objects must be changed to using the 
> fiid_obj_memset()> > function, because memset() itself is bad.
> > > 
> > > I think you get the idea.
> > > 
> > > API changes:
> > > ------------
> > > 
> > > If we desire to (we need not) we can eliminate passing of 
> > templates to
> > > many API functions.
> > > 
> > > Needless to say there are various issues that we need to discuss.
> > > 
> > > I believe we came to a reasonable agreement on this change at the
> > > FreeIPMI meeting, so I will (probably soon) begin a branch to 
> > begin 
> > > thisre-work.  When it gets time to the details, we will work it 
> out.> > 
> > > Al
> > > 
> > > Al
> > > 
> > > --
> > > Albert Chu
> > > address@hidden
> > > 925-422-5311
> > > Computer Scientist
> > > High Performance Systems Division
> > > Lawrence Livermore National Laboratory
> > > 
> > > 
> > > 
> > > 
> > > _______________________________________________
> > > Freeipmi-devel mailing list
> > > address@hidden
> > > http://lists.gnu.org/mailman/listinfo/freeipmi-devel
> > > 
> > 
> > 
> 
>