On Wed, 4 Feb 2009 20:14:21 +0200 Sasha Khapyorsky <[email protected]> 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: [email protected] <[email protected]> > > 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: [email protected] <[email protected]> > > --- > > 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 > > _______________________________________________ general mailing list [email protected] http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
