My apologies for the churn. I forgot to reply-all. ---------- Forwarded message ---------- From: Scott Mann <[email protected]> Date: Wed, Oct 22, 2014 at 4:38 PM Subject: Re: [ovs-dev] [PATCH] Build: shared libraries and versioning To: Ben Pfaff <[email protected]>
On Wed, Oct 22, 2014 at 3:35 PM, Ben Pfaff <[email protected]> wrote: > On Tue, Oct 21, 2014 at 04:02:53PM -0600, Scott Mann wrote: > > These changes cause shared libraries to be built by default. In > > particular, lib/libopenvwitch.so, lib/libsflow.so, > > ofproto/libofproto.so, and ovsdb/libovsdb.so will be built. Original > > behavior of building static objects excusively may be accomplished by > > providing the --disable-shared argument to configure. > > > > Additionally, versioning is introduced to each of the libraries objects > > paving the way for APIs to be built around them. A detailed comment > > outlining the rules for changing a version number is provided in > > configure.ac. Note that at this time, the version number is set to > > 1.0.0, no API is specified yet, and there are no requirements to maintain > > any sort of compatibility in any of the libraries. > > > > Signed-off-by: Scott Mann <[email protected]> > > Thanks for the patch. I have a few comments. > Thanks for the review! > > I would rather default to not building shared libraries. Can you > explain why it is better to build shared libraries by default? > Good question. It seemed a natural progression to me for what I am doing, but perhaps not everyone else. I'll reverse the behavior. > > > --- a/lib/util.c > > +++ b/lib/util.c > > @@ -471,9 +471,15 @@ set_program_name__(const char *argv0, const char > *version, const char *date, > > > > assert_single_threaded(); > > free(program_name); > > This code has a few minor C style violations. "if(" should be "if (", > there should be a space after each comma, and there should be a space > on each side of "+": > Thanks for these comments, I will fix them. > > > + /* Remove libtool prefix, if it is there */ > > + if(strncmp(basename,"lt-",3) == 0) { > > + char *tmp_name = basename; > > + basename = xstrdup(basename+3); > > + free(tmp_name); > > + } > > program_name = basename; > > - free(program_version); > > > > + free(program_version); > > if (!strcmp(version, VERSION)) { > > program_version = xasprintf("%s (Open vSwitch) "VERSION"\n" > > "Compiled %s %s\n", > > diff --git a/lib/vlog.h b/lib/vlog.h > > index 974a301..ca7c553 100644 > > --- a/lib/vlog.h > > +++ b/lib/vlog.h > > @@ -91,10 +91,13 @@ extern struct list vlog_modules; > > > > /* Creates and initializes a global instance of a module named MODULE. > */ > > #define VLOG_DEFINE_MODULE(MODULE) > \ > > - VLOG_DEFINE_MODULE__(MODULE) > \ > > - OVS_CONSTRUCTOR(init_##MODULE) { > \ > > - list_insert(&vlog_modules, &VLM_##MODULE.list); > \ > > + VLOG_DEFINE_MODULE__(MODULE) > \ > > + OVS_CONSTRUCTOR(init_##MODULE) { > \ > > I think that there deserves to be a comment here to explain why a > module with the given name might already exist. (Actually, I'm not > even sure why myself.) If it's a problem with possibly inserting the > same module twice, then it would probably be better to check whether > the module's 'list' pointers are null. On the other hand, if the > problem is two different modules with the same name, then we have a > bigger problem: it will become impossible to change the log levels, > etc., of the module that does not get inserted. > It is a problem with inserting the same module twice, but I did not think of checking the module's list pointer - I'll make that change. As for the cause, the constructor is called each time the shared object is loaded, which appears to occur more than once when running the test suite, although I have not investigated why. If that is surprising, I could look into it. Of course, the static objects are "loaded" once, so the constructor is only called before main(). Is this a reasonable explanation? Or does it warrant further investigation? > > > + struct vlog_module *mp = vlog_module_from_name(#MODULE); > \ > > + if (mp == NULL) { > \ > > + list_insert(&vlog_modules, &VLM_##MODULE.list); > \ > > } > \ > > + } > \ > _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
