[ofa-general] Re: [PATCH] libibmad: Declare some enums as typedefs for cleaner function interfaces
Ira Weiny
weiny2 at llnl.gov
Wed Feb 4 10:30:05 PST 2009
On Wed, 4 Feb 2009 20:14:21 +0200
Sasha Khapyorsky <sashak at voltaire.com> wrote:
> Hi Ira,
>
> On 18:54 Mon 02 Feb , Ira Weiny wrote:
> > Begining to clean up the libibmad interface.
> >
> > Ira
> >
> >
> > From 7e2f639905af92a6d4466d42af2e3e65bd717ffb Mon Sep 17 00:00:00 2001
> > From: weiny2 at llnl.gov <weiny2 at llnl.gov>
> > Date: Mon, 2 Feb 2009 10:21:18 -0800
> > Subject: [PATCH] Declare some enums as typedefs for cleaner function interfaces
>
> I don't understand how enum typedefing makes things cleaner - actually
> this will enforce me explicitly to verify an actual type in header
> files. Sometimes typedefs could help with porting, but it is not the
> case here.
Yes, this will force you to use the correct type. But I was looking at it from
the user standpoint. If I give the user a uint8_t buffer and tell them to use
this library to decode fields how do they know which values to pass in this
call.
uint32_t mad_get_field(void *buf, int base_offs, int field);
Using mad_field_t or even enum MAD_FIELDS allows one to use tags/cscope to find
the valid values for that parameter easily. Grepping will work but is still
cumbersome.
Again, I am trying to write a library which makes it easier for someone who
might not be familiar with IB to extract diagnostic data. I understand you
wanting the decoding of the data to be more flexible and abstract but we should
make the interface for decoding that data easier to use. I feel the following
patch does this.
Would you prefer to use:
uint32_t mad_get_field(void *buf, int base_offs, enum MAD_FIELDS field);
?
Ira
>
> Sasha
>
> >
> >
> > Signed-off-by: weiny2 at llnl.gov <weiny2 at llnl.gov>
> > ---
> > libibmad/include/infiniband/mad.h | 38 ++++++++++++++++++------------------
> > libibmad/src/fields.c | 22 ++++++++++----------
> > libibmad/src/resolve.c | 10 ++++----
> > 3 files changed, 35 insertions(+), 35 deletions(-)
> >
> > diff --git a/libibmad/include/infiniband/mad.h b/libibmad/include/infiniband/mad.h
> > index 9ff4a3e..f235ab0 100644
> > --- a/libibmad/include/infiniband/mad.h
> > +++ b/libibmad/include/infiniband/mad.h
> > @@ -203,7 +203,7 @@ typedef struct ib_field {
> > ib_mad_dump_fn *def_dump_fn;
> > } ib_field_t;
> >
> > -enum MAD_FIELDS {
> > +typedef enum MAD_FIELDS {
> > IB_NO_FIELD,
> >
> > IB_GID_PREFIX_F,
> > @@ -525,7 +525,7 @@ enum MAD_FIELDS {
> > IB_GUID_GUID0_F,
> >
> > IB_FIELD_LAST_ /* must be last */
> > -};
> > +} mad_field_t;
> >
> > /*
> > * SA RMPP section
> > @@ -595,21 +595,21 @@ typedef struct ib_vendor_call {
> > #define MAD_DEF_RETRIES 3
> > #define MAD_DEF_TIMEOUT_MS 1000
> >
> > -enum {
> > +typedef enum {
> > IB_DEST_LID,
> > IB_DEST_DRPATH,
> > IB_DEST_GUID,
> > IB_DEST_DRSLID,
> > -};
> > +} mad_dest_t;
> >
> > -enum {
> > +typedef enum {
> > IB_NODE_CA = 1,
> > IB_NODE_SWITCH,
> > IB_NODE_ROUTER,
> > NODE_RNIC,
> >
> > IB_NODE_MAX = NODE_RNIC
> > -};
> > +} mad_node_type_t;
> >
> > /******************************************************************************/
> >
> > @@ -631,20 +631,20 @@ static inline int ib_portid_set(ib_portid_t * portid, int lid, int qp, int qkey)
> > }
> >
> > /* fields.c */
> > -MAD_EXPORT uint32_t mad_get_field(void *buf, int base_offs, int field);
> > -MAD_EXPORT void mad_set_field(void *buf, int base_offs, int field,
> > +MAD_EXPORT uint32_t mad_get_field(void *buf, int base_offs, mad_field_t field);
> > +MAD_EXPORT void mad_set_field(void *buf, int base_offs, mad_field_t field,
> > uint32_t val);
> > /* field must be byte aligned */
> > -MAD_EXPORT uint64_t mad_get_field64(void *buf, int base_offs, int field);
> > -MAD_EXPORT void mad_set_field64(void *buf, int base_offs, int field,
> > +MAD_EXPORT uint64_t mad_get_field64(void *buf, int base_offs, mad_field_t field);
> > +MAD_EXPORT void mad_set_field64(void *buf, int base_offs, mad_field_t field,
> > uint64_t val);
> > -MAD_EXPORT void mad_set_array(void *buf, int base_offs, int field, void *val);
> > -MAD_EXPORT void mad_get_array(void *buf, int base_offs, int field, void *val);
> > -MAD_EXPORT void mad_decode_field(uint8_t * buf, int field, void *val);
> > -MAD_EXPORT void mad_encode_field(uint8_t * buf, int field, void *val);
> > -MAD_EXPORT int mad_print_field(int field, const char *name, void *val);
> > -MAD_EXPORT char *mad_dump_field(int field, char *buf, int bufsz, void *val);
> > -MAD_EXPORT char *mad_dump_val(int field, char *buf, int bufsz, void *val);
> > +MAD_EXPORT void mad_set_array(void *buf, int base_offs, mad_field_t field, void *val);
> > +MAD_EXPORT void mad_get_array(void *buf, int base_offs, mad_field_t field, void *val);
> > +MAD_EXPORT void mad_decode_field(uint8_t * buf, mad_field_t field, void *val);
> > +MAD_EXPORT void mad_encode_field(uint8_t * buf, mad_field_t field, void *val);
> > +MAD_EXPORT int mad_print_field(mad_field_t field, const char *name, void *val);
> > +MAD_EXPORT char *mad_dump_field(mad_field_t field, char *buf, int bufsz, void *val);
> > +MAD_EXPORT char *mad_dump_val(mad_field_t field, char *buf, int bufsz, void *val);
> >
> > /* mad.c */
> > MAD_EXPORT void *mad_encode(void *buf, ib_rpc_t * rpc, ib_dr_path_t * drpath,
> > @@ -729,7 +729,7 @@ MAD_EXPORT int ib_resolve_smlid(ib_portid_t * sm_id, int timeout);
> > MAD_EXPORT int ib_resolve_guid(ib_portid_t * portid, uint64_t * guid,
> > ib_portid_t * sm_id, int timeout);
> > MAD_EXPORT int ib_resolve_portid_str(ib_portid_t * portid, char *addr_str,
> > - int dest_type, ib_portid_t * sm_id);
> > + mad_dest_t dest, ib_portid_t * sm_id);
> > MAD_EXPORT int ib_resolve_self(ib_portid_t * portid, int *portnum,
> > ibmad_gid_t * gid);
> >
> > @@ -737,7 +737,7 @@ int ib_resolve_smlid_via(ib_portid_t * sm_id, int timeout, const void *srcport);
> > int ib_resolve_guid_via(ib_portid_t * portid, uint64_t * guid,
> > ib_portid_t * sm_id, int timeout, const void *srcport);
> > int ib_resolve_portid_str_via(ib_portid_t * portid, char *addr_str,
> > - int dest_type, ib_portid_t * sm_id,
> > + mad_dest_t dest, ib_portid_t * sm_id,
> > const void *srcport);
> > int ib_resolve_self_via(ib_portid_t * portid, int *portnum, ibmad_gid_t * gid,
> > const void *srcport);
> > diff --git a/libibmad/src/fields.c b/libibmad/src/fields.c
> > index d5a1eb4..d435a2f 100644
> > --- a/libibmad/src/fields.c
> > +++ b/libibmad/src/fields.c
> > @@ -479,37 +479,37 @@ static void _get_array(void *buf, int base_offs, const ib_field_t * f,
> > memcpy(val, (uint8_t *) buf + base_offs + bitoffs / 8, f->bitlen / 8);
> > }
> >
> > -uint32_t mad_get_field(void *buf, int base_offs, int field)
> > +uint32_t mad_get_field(void *buf, int base_offs, mad_field_t field)
> > {
> > return _get_field(buf, base_offs, ib_mad_f + field);
> > }
> >
> > -void mad_set_field(void *buf, int base_offs, int field, uint32_t val)
> > +void mad_set_field(void *buf, int base_offs, mad_field_t field, uint32_t val)
> > {
> > _set_field(buf, base_offs, ib_mad_f + field, val);
> > }
> >
> > -uint64_t mad_get_field64(void *buf, int base_offs, int field)
> > +uint64_t mad_get_field64(void *buf, int base_offs, mad_field_t field)
> > {
> > return _get_field64(buf, base_offs, ib_mad_f + field);
> > }
> >
> > -void mad_set_field64(void *buf, int base_offs, int field, uint64_t val)
> > +void mad_set_field64(void *buf, int base_offs, mad_field_t field, uint64_t val)
> > {
> > _set_field64(buf, base_offs, ib_mad_f + field, val);
> > }
> >
> > -void mad_set_array(void *buf, int base_offs, int field, void *val)
> > +void mad_set_array(void *buf, int base_offs, mad_field_t field, void *val)
> > {
> > _set_array(buf, base_offs, ib_mad_f + field, val);
> > }
> >
> > -void mad_get_array(void *buf, int base_offs, int field, void *val)
> > +void mad_get_array(void *buf, int base_offs, mad_field_t field, void *val)
> > {
> > _get_array(buf, base_offs, ib_mad_f + field, val);
> > }
> >
> > -void mad_decode_field(uint8_t * buf, int field, void *val)
> > +void mad_decode_field(uint8_t * buf, mad_field_t field, void *val)
> > {
> > const ib_field_t *f = ib_mad_f + field;
> >
> > @@ -528,7 +528,7 @@ void mad_decode_field(uint8_t * buf, int field, void *val)
> > _get_array(buf, 0, f, val);
> > }
> >
> > -void mad_encode_field(uint8_t * buf, int field, void *val)
> > +void mad_encode_field(uint8_t * buf, mad_field_t field, void *val)
> > {
> > const ib_field_t *f = ib_mad_f + field;
> >
> > @@ -602,21 +602,21 @@ static int _mad_print_field(const ib_field_t * f, const char *name, void *val,
> > valsz ? valsz : ALIGN(f->bitlen, 8) / 8);
> > }
> >
> > -int mad_print_field(int field, const char *name, void *val)
> > +int mad_print_field(mad_field_t field, const char *name, void *val)
> > {
> > if (field <= IB_NO_FIELD || field >= IB_FIELD_LAST_)
> > return -1;
> > return _mad_print_field(ib_mad_f + field, name, val, 0);
> > }
> >
> > -char *mad_dump_field(int field, char *buf, int bufsz, void *val)
> > +char *mad_dump_field(mad_field_t field, char *buf, int bufsz, void *val)
> > {
> > if (field <= IB_NO_FIELD || field >= IB_FIELD_LAST_)
> > return 0;
> > return _mad_dump_field(ib_mad_f + field, 0, buf, bufsz, val);
> > }
> >
> > -char *mad_dump_val(int field, char *buf, int bufsz, void *val)
> > +char *mad_dump_val(mad_field_t field, char *buf, int bufsz, void *val)
> > {
> > if (field <= IB_NO_FIELD || field >= IB_FIELD_LAST_)
> > return 0;
> > diff --git a/libibmad/src/resolve.c b/libibmad/src/resolve.c
> > index b62360b..faac1f9 100644
> > --- a/libibmad/src/resolve.c
> > +++ b/libibmad/src/resolve.c
> > @@ -92,7 +92,7 @@ int ib_resolve_guid_via(ib_portid_t * portid, uint64_t * guid,
> > }
> >
> > int ib_resolve_portid_str_via(ib_portid_t * portid, char *addr_str,
> > - int dest_type, ib_portid_t * sm_id,
> > + mad_dest_t dest, ib_portid_t * sm_id,
> > const void *srcport)
> > {
> > uint64_t guid;
> > @@ -101,7 +101,7 @@ int ib_resolve_portid_str_via(ib_portid_t * portid, char *addr_str,
> > ib_portid_t selfportid = { 0 };
> > int selfport = 0;
> >
> > - switch (dest_type) {
> > + switch (dest) {
> > case IB_DEST_LID:
> > lid = strtol(addr_str, 0, 0);
> > if (!IB_LID_VALID(lid))
> > @@ -136,16 +136,16 @@ int ib_resolve_portid_str_via(ib_portid_t * portid, char *addr_str,
> > return 0;
> >
> > default:
> > - IBWARN("bad dest_type %d", dest_type);
> > + IBWARN("bad dest %d", dest);
> > }
> >
> > return -1;
> > }
> >
> > -int ib_resolve_portid_str(ib_portid_t * portid, char *addr_str, int dest_type,
> > +int ib_resolve_portid_str(ib_portid_t * portid, char *addr_str, mad_dest_t dest,
> > ib_portid_t * sm_id)
> > {
> > - return ib_resolve_portid_str_via(portid, addr_str, dest_type,
> > + return ib_resolve_portid_str_via(portid, addr_str, dest,
> > sm_id, NULL);
> > }
> >
> > --
> > 1.5.4.5
> >
More information about the general
mailing list