[stgt] SPAM: [PATCH 2/2] dynamically link libibverbs and librdma

[Pete Wyckoff]

dorons at Voltaire.COM wrote on Thu, 02 Oct 2008 13:30 +0300:
> dynamically link libibverbs and librdma for using
> stgt without having userspace IB (e.g tcp mode).

A couple of comments.

> @@ -1,4 +1,6 @@
>  mandir = /usr/share/man
> +libdir = /usr/lib64
> +bindir = /usr/sbin

Fine for most 64-bit machines, not sure if we are worried about others.

>  tgtd: $(TGTD_OBJS)
> -       $(CC) $^ -o $@ $(LIBS)
> +       $(CC) -Xlinker -E $^ -o $@ $(LIBS)

You may need to do something like:

    $(LD) -o $@ -Wl,-whole-archive libtgtrdma.so -Wl,-no-whole-archive $(LIBS)

to make sure the constructor functions in the .so get called properly.

> +$(SO_LIBS): $(ISER_OBJS)
> +       rm -f $@ $(SO_NAME)
> +       $(LD) -shared -soname $(SO_LIBS) -o $(SO_LIBS) $(ISER_OBJS)
> +       ln -s $(SO_LIBS) $(SO_NAME)
> +

You must put -lrdmacm -libverbs on this $(LD) line.  I learned that
the hard way once or twice.  Else the symbol versions don't work out
at runtime.

> +install_lib: $(SO_LIBS)
> +	rm -f $(DESTDIR)$(libdir)/$(SO_NAME)
> +	install -m 755 $(SO_LIBS) $(DESTDIR)$(libdir)
> +	ln -s $(DESTDIR)/usr/lib64/$(SO_LIBS) $(DESTDIR)$(libdir)/$(SO_NAME)

maybe use $(libdir) in here too

> +/*
> + * iSCSI extensions for RDMA (iSER) data path
> + *
> + * Copyright (C) 2007 Dennis Dalessandro (dennis at osc.edu)
> + * Copyright (C) 2007 Ananth Devulapalli (ananth at osc.edu)
> + * Copyright (C) 2007 Pete Wyckoff (pw at osc.edu)

Actually you wrote this new file, in 2008.  And it needs a better
description.

> +#include <stdio.h>
> +#include <dlfcn.h>
> +#include <syslog.h>
> +
> +static void *pverbs;
> +static void *prdma;
> +static void *ptgtrdma;
> +
> +__attribute__((constructor)) static void iser_transport_init(void)
> +{
> +	void (*iser_init)(void);
> +	char *error;
> +
> +	pverbs = dlopen("libibverbs.so",RTLD_NOW|RTLD_GLOBAL);
> +	if (!pverbs) {
> +		goto Exit; /* do not register iser transport */
> +	}
> +
> +	prdma = dlopen("librdmacm.so",RTLD_NOW|RTLD_GLOBAL);
> +	if (!prdma) {
> +		goto Exit; /* do not register iser transport */
> +	}
> +
> +	ptgtrdma = dlopen("libtgtrdma.so",RTLD_NOW|RTLD_GLOBAL);
> +	if (!ptgtrdma) {
> +		goto Exit;
> +	}
> +
> +	dlerror();    /* Clear any existing error */
> +	iser_init = dlsym(ptgtrdma, "iser_transport_init");
> +	if ((error = dlerror()) != NULL)  {
> +		syslog(LOG_ERR, "%s\n", error);
> +		goto Exit;
> +	}
> +
> +	(*iser_init)();

My understanding is that iser_transport_init() would have been
called by the dlopen() of libtgtrdma.so, as it is already marked
((constructor)).  Did you verify that this doesn't happen?  Maybe
this requires the whole-archive bit above.

Also I figured dlopen("libtgtrdma.so") would pull in the other two
libs on its own.  Please check, as it would be much cleaner just
to have the single dlopen of libtgtrdma.so, and no dlsym or other
libs.

> +__attribute__((destructor)) static void iser_transport_close(void)
> +{
> +	syslog(LOG_INFO, "iser transport cleanup");
> +	if (pverbs)
> +		dlclose(pverbs);
> +	if (prdma)
> +		dlclose(prdma);
> +	if (ptgtrdma)
> +		dlclose(ptgtrdma);
> +}

Interesting.  I guess tgtd knows how to exit cleanly these days.
But this stuff isn't strictly necessary; you can just exit and the
OS will clean it all.

> diff --git a/usr/iscsi/libtgt_rdma.c b/usr/iscsi/libtgt_rdma.c
> index d3b5147..d6ba5fa 100644
> --- a/usr/iscsi/libtgt_rdma.c
> +++ b/usr/iscsi/libtgt_rdma.c
> @@ -30,6 +30,8 @@
>  #include <sys/epoll.h>
>  #include <infiniband/verbs.h>
>  #include <rdma/rdma_cma.h>
> +#include <dlfcn.h>
> +#include <syslog.h>
>  
>  #include "util.h"
>  #include "iscsid.h"
> @@ -1754,7 +1756,9 @@ static struct iscsi_transport iscsi_iser = {
>  	.ep_getpeername		= iscsi_rdma_getpeername,
>  };
>  
> -__attribute__((constructor)) static void iser_transport_init(void)
> +void iser_transport_init(void)
>  {
> +	syslog(LOG_INFO, "iser transport register");
>  	iscsi_transport_register(&iscsi_iser);
> +	return;
>  }

Oh!  You removed the constructor.  Now I see why it doesn't work.
Maybe try my suggestion instead?

		-- Pete
--
To unsubscribe from this list: send the line "unsubscribe stgt" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html