Re: [Freeipmi-devel] FIID Re-Implementation

[Albert Chu]

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 around
anymore.  

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
> > 
> 
>