[ofa-general] Re: [PATCH v3 2/3] Convert iblinkinfo.pl to C and use new ibnetdisc library.

[Ira Weiny]

On Thu, 23 Apr 2009 10:15:42 +0300
Sasha Khapyorsky <sashak at voltaire.com> wrote:

> On 15:42 Fri 03 Apr     , Ira Weiny wrote:
> > From a677ae35fe7a5966f05b5859df8f00e9b18df864 Mon Sep 17 00:00:00 2001
> 
> You know :)
> 
> > From: Ira Weiny <weiny2 at llnl.gov>
> > Date: Fri, 3 Apr 2009 15:28:18 -0700
> > Subject: [PATCH] Convert iblinkinfo.pl to C and use new ibnetdisc library.
> > 
> > Signed-off-by: Ira Weiny <weiny2 at llnl.gov>
> 
> Applied. Thanks. Couple of notes are below.
> 
> > ---
> >  infiniband-diags/Makefile.am              |    7 +-
> >  infiniband-diags/configure.in             |    1 +
> >  infiniband-diags/scripts/iblinkinfo.pl    |  327 ------------------------
> >  infiniband-diags/scripts/iblinkinfo.pl.in |   40 +++
> >  infiniband-diags/src/iblinkinfo.c         |  386 +++++++++++++++++++++++++++++
> >  5 files changed, 432 insertions(+), 329 deletions(-)
> >  delete mode 100755 infiniband-diags/scripts/iblinkinfo.pl
> >  create mode 100755 infiniband-diags/scripts/iblinkinfo.pl.in
> >  create mode 100644 infiniband-diags/src/iblinkinfo.c
> > 
> > diff --git a/infiniband-diags/Makefile.am b/infiniband-diags/Makefile.am
> > index 7b8523a..b480a4a 100644
> > --- a/infiniband-diags/Makefile.am
> > +++ b/infiniband-diags/Makefile.am
> > @@ -1,6 +1,7 @@
> >  SUBDIRS = libibnetdisc
> >  
> > -INCLUDES = -I$(top_builddir)/include/ -I$(srcdir)/include -I$(includedir) -I$(includedir)/infiniband
> > +INCLUDES = -I$(top_builddir)/include/ -I$(srcdir)/include -I$(includedir) -I$(includedir)/infiniband \
> > +	-I$(top_builddir)/libibnetdisc/include
> 
> Here $(top_srcdir) should be used instead of $(top_builddir).
> 'top_builddir' points project build directory, not source directory
> (where not generated header files are located). And build like:
> 
>   mkdir /tmp/ib-diags && cd /tmp/ib-diags
>   /path/to/management/infiniband-diags/configure && make
> 
> will fail.
> 
> I'm fixing this.

yep, thanks.

> 
> >  if DEBUG
> >  DBGFLAGS = -ggdb -D_DEBUG_
> > @@ -11,7 +12,7 @@ endif
> >  sbin_PROGRAMS = src/ibaddr src/ibnetdiscover src/ibping src/ibportstate \
> >  	        src/ibroute src/ibstat src/ibsysstat src/ibtracert \
> >  	        src/perfquery src/sminfo src/smpdump src/smpquery \
> > -	        src/saquery src/vendstat
> > +	        src/saquery src/vendstat src/iblinkinfo
> >  
> >  if ENABLE_TEST_UTILS
> >  sbin_PROGRAMS += src/ibsendtrap src/mcm_rereg_test
> > @@ -55,6 +56,8 @@ src_saquery_SOURCES = src/saquery.c
> >  src_ibsendtrap_SOURCES = src/ibsendtrap.c
> >  src_vendstat_SOURCES = src/vendstat.c
> >  src_mcm_rereg_test_SOURCES = src/mcm_rereg_test.c
> > +src_iblinkinfo_SOURCES = src/iblinkinfo.c
> > +src_iblinkinfo_LDADD = -libnetdisc
> 
> I think here should be '-L$(top_builddir)/libibnetdisc -libnetdisc'.
> Otherwise we are assuming pre-installed library (we could run
> 'make install', but it fails due to 'make' error :)).
> 
> Adding this too.

Oops...  Sorry, that got put into the "convert ibnetdiscover" patch.  But I
used $(top_srcdir) I can change it.  See below:

--- a/infiniband-diags/Makefile.am
+++ b/infiniband-diags/Makefile.am
@@ -41,7 +41,8 @@ LDADD = libcommon.a
 
 libcommon_a_SOURCES = src/ibdiag_common.c
 src_ibaddr_SOURCES = src/ibaddr.c
-src_ibnetdiscover_SOURCES = src/ibnetdiscover.c src/grouping.c
+src_ibnetdiscover_SOURCES = src/ibnetdiscover.c
+src_ibnetdiscover_LDFLAGS = -L$(top_srcdir)/libibnetdisc -libnetdisc
 src_ibping_SOURCES = src/ibping.c
 src_ibportstate_SOURCES = src/ibportstate.c
 src_ibroute_SOURCES = src/ibroute.c
@@ -57,7 +58,7 @@ src_ibsendtrap_SOURCES = src/ibsendtrap.c
 src_vendstat_SOURCES = src/vendstat.c
 src_mcm_rereg_test_SOURCES = src/mcm_rereg_test.c
 src_iblinkinfo_SOURCES = src/iblinkinfo.c
-src_iblinkinfo_LDADD = -libnetdisc
+src_iblinkinfo_LDFLAGS = -L$(top_srcdir)/libibnetdisc -libnetdisc
 
 man_MANS = man/ibaddr.8 man/ibcheckerrors.8 man/ibcheckerrs.8 \
 	man/ibchecknet.8 man/ibchecknode.8 man/ibcheckport.8 \


[snip]

> > +
> > +	static char const str_opts[] = "S:D:n:C:P:t:sldgphuf:R";
> > +	static const struct option long_opts[] = {
> > +		{ "S", 1, 0, 'S'},
> > +		{ "D", 1, 0, 'D'},
> > +		{ "num-hops", 1, 0, 'n'},
> > +		{ "down-links-only", 0, 0, 'd'},
> > +		{ "line-mode", 0, 0, 'l'},
> > +		{ "ca-name", 1, 0, 'C'},
> > +		{ "ca-port", 1, 0, 'P'},
> > +		{ "timeout", 1, 0, 't'},
> > +		{ "show", 0, 0, 's'},
> > +		{ "print-port-guids", 0, 0, 'g'},
> > +		{ "print-additional", 0, 0, 'p'},
> > +		{ "help", 0, 0, 'h'},
> > +		{ "usage", 0, 0, 'u'},
> > +		{ "node-name-map", 1, 0, 1},
> > +		{ "debug", 0, 0, 2},
> > +		{ "compat", 0, 0, 3},
> > +		{ "from", 1, 0, 'f'},
> > +		{ "R", 0, 0, 'R'},
> > +		{ }
> > +	};
> > +
> > +	f = stdout;
> > +
> > +	argv0 = argv[0];
> > +
> > +	while (1) {
> > +		int ch = getopt_long(argc, argv, str_opts, long_opts, NULL);
> 
> Any reason to not use new ibdiag_process_opts() here? It should simplify
> an options processing and unify the usage message.
> 
> Of course this can be done as subsequent patch.
> 

The only reason was that I started this patch before ibdiag_process_opts was
created!  I did use ibdiag_process_opts for ibqueryerrors.  And yes I will be
cleaning this up in subsequent patches.

Ira