On Wed, 15 Apr 2009 18:30:03 +0300 Sasha Khapyorsky <[email protected]> wrote:
> Hi Ira, > > On 14:44 Wed 11 Mar , Ira Weiny wrote: > > > > From: Ira Weiny <[email protected]> > > Date: Wed, 11 Mar 2009 10:45:25 -0700 > > Subject: [PATCH] Update mad formatting functions. > > > > Add mad_snprintf w/ man page > > Add mad_fprintf w/ man page > > Add comments to document current functions. > > Rename parameters to avoid confusion with other functions which take > > "buf" > > Mark mad_print_field as deprecated > > > > Signed-off-by: Ira Weiny <[email protected]> > > Nice stuff! I have some implementation comments below. > [snip] > > diff --git a/libibmad/man/mad_fprintf.3 b/libibmad/man/mad_fprintf.3 > > new file mode 100644 > > index 0000000..e69bd44 > > --- /dev/null > > +++ b/libibmad/man/mad_fprintf.3 > > @@ -0,0 +1,82 @@ > > +.\" -*- nroff -*- > > +.\" > > +.TH MAD_FPRINTF 3 "Feb 26, 2009" "OpenIB" "OpenIB Programmer\'s Manual" > > +.SH "NAME" > > +mad_fprintf, mad_snprintf \- formatted output conversion for mad packets > > +.SH "SYNOPSIS" > > +.nf > > +.B #include <infiniband/mad.h> > > +.sp > > +.BI "MAD_EXPORT int mad_snprintf(char " "*s" ", size_t "n ", uint8_t " > > "*buf" ", const char " "*format" ", ...); > > +.BI "MAD_EXPORT int mad_fprintf(FILE " "*stream" ", uint8_t " "*buf" ", > > const char " "*format" ", ...); > > +.fi > > +.SH "DESCRIPTION" > > +Similar to the printf family of functions. The exception being they do > > +.B not > > +accept all conversion specifiers and they accept a "buf" parameter which > > +represents a mad data buffer. This buffer is used to extract and print > > fields > > +as specified with the > > +.B %F > > +format specifier. > > +.PP > > +The following conversion specifiers are > > +.B not > > +supported. > > +.B e, E, f, g, G, a, A, C, S, m > > +and > > +.B n > > +.PP > > +.B F > > +The %F specifier is used to print out fields decoded from the "buf" data > > +buffer. > > ib_mad_f table also has a name string field. I think it can be useful > too - will help to unify outputs. Of course this can be done as > subsequent patch. Yes but I don't think we should force users to use any specific output. If they want to print the "name" of a field that should be a separate specifier __not__ automatic. Is this what you mean? > > > +.I enum MAD_FIELDS\fR > > +values should be used to specify the field to be decoded. > > +.PP > > +.SH "EXAMPLES" > > +.nf > > +char portinfo[64]; > > +void *pi = portinfo; > > +.PP > > +if (!smp_query(pi, portid, IB_ATTR_PORT_INFO, portnum, timeout)) > > +.in +8 > > + return -1; > > +.in -16 > > +.PP > > +mad_fprintf(stdout, pi, "Port info (%s):\\n" > > +.in +16 > > +" %-10s: %F\\n" > > +" %-10s: %F\\n" > > +" %-10s: %F\\n" > > +" %-10s: %F\\n" > > +" %-10s: %F\\n" > > +" %-10s: %F\\n", > > +portid2str(portid), > > +"LID", IB_PORT_LID_F, > > +"LMC", IB_PORT_LMC_F, > > +"state", IB_PORT_STATE_F, > > +"physstate", IB_PORT_PHYS_STATE_F, > > +"linkwidth", IB_PORT_LINK_WIDTH_ACTIVE_F, > > +"linkspeed", IB_PORT_LINK_SPEED_ACTIVE_F > > +); > > +.in -16 > > +.PP > > +Results in the output. > > +.PP > > +Port info (DR path slid 0; dlid 0; 0,1,14): > > +.in +3 > > +LID : 0x0016 > > Lids are printed as decimals. Well I thought I copied the output from the example but I see that it is printing decimal. So?? :-/ I fixed it. As an aside, not all LID's are decimal. Should we change this? from fields.c ... {BE_OFFS(256, 16), "DrSmpDLID", mad_dump_hex}, {BE_OFFS(272, 16), "DrSmpSLID", mad_dump_hex}, ... {BITSOFFS(224, 16), "RedirectLID", mad_dump_hex}, {BITSOFFS(480, 16), "TrapLID", mad_dump_hex}, ... {BITSOFFS(320, 16), "PathRecDLid", mad_dump_hex}, {BITSOFFS(336, 16), "PathRecSLid", mad_dump_hex}, ... {BITSOFFS(288, 16), "McastMemMLid", mad_dump_hex}, [snip] > > diff --git a/libibmad/man/mad_snprintf.3 b/libibmad/man/mad_snprintf.3 > > new file mode 100644 > > index 0000000..c004ab9 > > --- /dev/null > > +++ b/libibmad/man/mad_snprintf.3 > > @@ -0,0 +1,2 @@ > > +.TH MAD_SNPRINTF 3 "Feb 26, 2009" "OpenIB" "OpenIB Programmer\'s Manual" > > +.so man3/mad_fprintf.3 > > diff --git a/libibmad/src/fields.c b/libibmad/src/fields.c > > index 19c8fc1..acb1180 100644 > > --- a/libibmad/src/fields.c > > +++ b/libibmad/src/fields.c > > @@ -38,7 +38,9 @@ > > > > #include <stdio.h> > > #include <stdlib.h> > > +#define _GNU_SOURCE > > Where is _GNU_SOURCE really used (I didn't find)? Yep you are right, I don't need this. [snip] > > + } > > + str = number(str, n-rc, &rc, num, base, field_width, precision, > > flags); > > + if (rc <= 1) > > + break; > > + } > > +max_len_hit: > > + *str = '\0'; > > + return str-s; > > +} > > Now instead of reimplementing *printf() functions with potential need > to follow their extensions/conventions/update/etc wouldn't it be easier > (and in long term safer) to just rebuild format string by resolving > known %X conversions and then to pass it with rest parameters to > standard libc's *printf()? > > In this way we will support all what *printf()s know + our conversions. > I thought about that but decided not to do it. I can't remember why though... ;-) So maybe I agree with you, let me try and remember and if I can't I will change it. [snip] Ira _______________________________________________ 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
