[ofa-general] [PATCH 2/2] libibmad: print warning when _do_madrpc fails with destination port information

[Hal Rosenstock]

On Thu, 2008-06-26 at 10:54 -0700, Ira Weiny wrote:
> On Thu, 26 Jun 2008 06:00:29 -0700
> Hal Rosenstock <hrosenstock at xsigo.com> wrote:
> 
> > On Wed, 2008-06-25 at 16:09 -0700, Ira Weiny wrote:
> > > diff --git a/libibmad/src/portid.c b/libibmad/src/portid.c
> > > index 7279e14..01f9530 100644
> > > --- a/libibmad/src/portid.c
> > > +++ b/libibmad/src/portid.c
> > ...
> > > @@ -65,29 +66,24 @@ char *
> > >  portid2str(ib_portid_t *portid)
> > >  {
> > >         static char buf[1024] = "local";
> > > +       char drpath[512];
> > >         char *s = buf;
> > >         int i;
> > >  
> > >         if (portid->lid > 0) {
> > >                 s += sprintf(s, "Lid %d", portid->lid);
> > >                 if (portid->grh_present) {
> > > -                       s += sprintf(s, " Gid 0x%" PRIx64 "%" PRIx64,
> > > -                                       ntohll(*(uint64_t *)portid-
> > > >gid),
> > > -                                       ntohll(*(uint64_t *)(portid-
> > > >gid+8)));
> > > +                       char gid[sizeof
> > > "ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff"];
> > > +                       inet_ntop(AF_INET6, portid->gid, gid, sizeof
> > > (gid));
> > > +                       s += sprintf(s, " Gid %s", gid);
> > 
> > Should this be:
> > 		if (inet_ntop(AF_INET6, portid->gid, gid, sizeof (gid))
> > 			s += sprintf(s, " Gid %s", gid);
> > 
> > >                 }
> 
> That is probably safer but the memory should be ok to print from.  I will make
> the change.

Per the man page, it doesn't look like it would fail but it is safer as
you point out.

> 
> > 
> > Also, I'm all for this change but I'm also for consistency (I may be a
> > minority of one on this) so I have to ask whether you plan on updating
> > diags and/or OpenSM for GID format.
> 
> Well...  I would like to start moving it over.  I think it is more standard.

I hope we get this all converted over rapidly.

> > 
> > Note also that if this is ported to Windows a wrapper for inet_ntop will
> > be needed.
> > 
> 
> Well...  I don't want to break Windows

It's just a future porting issue if this is ported over.

>  but does Windows really not support this?

Not AFAIT although a wrapper looks pretty simple. 

>   I am not sure but the man page indicates it is POSIX.
> 
> http://osdir.com/ml/web.wget.patches/2005-08/msg00008.html

Not sure we would want to require POSIX for Windows.

-- Hal

> Ira
>