[ofa-general] Re: [PATCH] Update mad formatting functions.

[Ira Weiny]

On Wed, 15 Apr 2009 18:30:03 +0300
Sasha Khapyorsky <sashak at voltaire.com> wrote:

> Hi Ira,
> 
> On 14:44 Wed 11 Mar     , Ira Weiny wrote:
> > 
> > From: Ira Weiny <weiny2 at llnl.gov>
> > 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 <weiny2 at llnl.gov>
> 
> 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