On Thu, Aug 02, 2012 at 11:28:38AM -0700, Ben Pfaff wrote: > On Thu, Aug 02, 2012 at 10:51:26AM -0700, Ansis Atteka wrote: > > On Wed, Aug 1, 2012 at 11:23 PM, Ben Pfaff <[email protected]> wrote: > > > > > Some in-tree and out-of-tree code sets the OVS_SYSCONFDIR environment > > > variable to control where /etc files go (mostly for test purposes). When > > > the database directory (dbdir) was split off from the sysconfdir, the > > > configure-time default continued to be based on the sysconfdir, but > > > overriding the sysconfdir at runtime with OVS_SYSCONFDIR didn't have any > > > effect on the dbdir, which caused a visible change in behavior for code > > > that set the OVS_SYSCONFDIR environment variable. This commit reverts > > > that > > > change in behavior, by basing the dbdir on OVS_SYSCONFDIR if that > > > environment variable is set (but the OVS_DBDIR environment variable is > > > not). > > > > > > Signed-off-by: Ben Pfaff <[email protected]> > > > --- > > > lib/dirs.c.in | 15 +++++++++++++-- > > > python/automake.mk | 29 +++++++++++++++++++++++------ > > > python/ovs/dirs.py | 20 ++++++++++++-------- > > > python/ovs/dirs.py.template | 17 +++++++++++++++++ > > > 4 files changed, 65 insertions(+), 16 deletions(-) > > > create mode 100644 python/ovs/dirs.py.template > > > > > > diff --git a/lib/dirs.c.in b/lib/dirs.c.in > > > index 2b998b9..5a892b2 100644 > > > --- a/lib/dirs.c.in > > > +++ b/lib/dirs.c.in > > > @@ -18,6 +18,7 @@ > > > #include <config.h> > > > #include "dirs.h" > > > #include <stdlib.h> > > > +#include "util.h" > > > > > > struct directory { > > > const char *value; /* Actual value; NULL if not yet > > > determined. */ > > > @@ -68,8 +69,18 @@ ovs_logdir(void) > > > const char * > > > ovs_dbdir(void) > > > { > > > - static struct directory d = { NULL, @DBDIR@, "OVS_DBDIR" }; > > > - return get_dir(&d); > > > + static char *dbdir; > > > > > While getenv() returns char *, the contents should not be > > modified. So perhaps define it as const instead? > > Yes, thank you, fixed. > > > > + if (!dbdir) { > > > + dbdir = getenv("OVS_DBDIR"); > > > + if (!dbdir || !dbdir[0]) { > > > + char *sysconfdir = getenv("OVS_SYSCONFDIR"); > > > + > > > + dbdir = (sysconfdir > > > > > I guess python implementation handles empty string case differently. > > Yes, unfortunately. It is a pre-existing bug that has been there for > a long time, so it does not seem to matter much. I think that we > should fix it, but I'd rather do that in another patch. > > > > --- a/python/ovs/dirs.py > > > +++ b/python/ovs/dirs.py > > > @@ -1,9 +1,13 @@ > > > -# These are the default directories. They will be replaced by the > > > -# configured directories at install time. > > > - > > > import os > > > -PKGDATADIR = os.environ.get("OVS_PKGDATADIR", > > > "/usr/local/share/openvswitch") > > > -RUNDIR = os.environ.get("OVS_RUNDIR", "/var/run") > > > -LOGDIR = os.environ.get("OVS_LOGDIR", "/usr/local/var/log") > > > -LOGDIR = os.environ.get("OVS_DBDIR", "/usr/local/etc/openvswitch") > > > > I guess the upper line has a conflict when applied to the top of the > > master. I assume this > > line is still deleted when the conflict is resolved? > > Yes. > > > > +DBDIR = os.environ.get("OVS_DBDIR") > > > +if not DBDIR: > > > + sysconfdir = os.environ.get("OVS_SYSCONFDIR") > > > + if sysconfdir: > > > > > The upper "if" in C and Python implementation diverge > > here (Empty string case). > > Because "" is false in Python, I think that in fact this brings the > Python version closer to the C version. > > > > +DBDIR = os.environ.get("OVS_DBDIR") > > > +if not DBDIR: > > > + sysconfdir = os.environ.get("OVS_SYSCONFDIR") > > > + if sysconfdir: > > > > > I guess Python and C implementation diverge here as well, when > > OVS_SYSCONFDIR="" > > This file is the source used to generate the previous > python/ovs/dirs.py file above, so it will have the same defects. > > > git am says: > > aatteka@aatteka-MacBookPro:~/NiciraGit/openvswitch$ git am dirs\:\ dbdir\ > > default\ must\ be\ based\ on\ sysconfdir.patch > > Applying: dirs: dbdir default must be based on sysconfdir. > > /home/aatteka/NiciraGit/openvswitch/.git/rebase-apply/patch:121: trailing > > whitespace. > > ## > > warning: 1 line adds whitespace errors. > > Thanks, I fixed that. > > > Also, appplying on top of the master gives me a conflict. > > Probably just in dirs.py? I've rebased past Mehak's change now, to > resolve that. > > > Does this need any update to other files too (e.g. man pages) on how > > environment variables resolve paths? > > I don't think so. There isn't a lot about this in the manpages, and > what is there seems to be correct. The ovs-ctl --help output (one > place where the directories are described) already says the correct > thing. > > I just noticed that utilities/ovs-lib.in needs the same fix as the > Python and C versions: > > diff --git a/utilities/ovs-lib.in b/utilities/ovs-lib.in > index 893e8d1..3c63ddd 100644 > --- a/utilities/ovs-lib.in > +++ b/utilities/ovs-lib.in > @@ -22,14 +22,21 @@ > # All of these should be substituted by the Makefile at build time. > logdir=${OVS_LOGDIR-'@LOGDIR@'} # /var/log/openvswitch > rundir=${OVS_RUNDIR-'@RUNDIR@'} # /var/run/openvswitch > -dbdir=${OVS_DBDIR-'@DBDIR@'} # /etc/openvswitch > - # or /var/lib/openvswitch > sysconfdir=${OVS_SYSCONFDIR-'@sysconfdir@'} # /etc > etcdir=$sysconfdir/openvswitch # /etc/openvswitch > datadir=${OVS_PKGDATADIR-'@pkgdatadir@'} # /usr/share/openvswitch > bindir=${OVS_BINDIR-'@bindir@'} # /usr/bin > sbindir=${OVS_SBINDIR-'@sbindir@'} # /usr/sbin > > +# /etc/openvswitch or /var/lib/openvswitch > +if test X"$OVS_DBDIR" != X; then > + dbdir=$OVS_DBDIR > +elif test X"$OVS_SYSCONFDIR" != X; then > + dbdir=$OVS_SYSCONFDIR/openvswitch > +else > + dbdir='@DBDIR@' > +fi > + > VERSION='@VERSION@' > > LC_ALL=C; export LC_ALL > > Here's the full revised patch.
Crap, that didn't make any sense, did it. I'll repost the actual revised patch. _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
