Re: [Freeipmi-devel] FIID Re-Implementation

[Albert Chu]

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