[ovs-dev] [PATCH] dirs: dbdir default must be based on sysconfdir.
Ben Pfaff
blp at nicira.com
Thu Aug 2 18:28:38 UTC 2012
On Thu, Aug 02, 2012 at 10:51:26AM -0700, Ansis Atteka wrote:
> On Wed, Aug 1, 2012 at 11:23 PM, Ben Pfaff <blp at nicira.com> 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 <blp at nicira.com>
> > ---
> > 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 at 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.
--8<--------------------------cut here-------------------------->8--
From: Ben Pfaff <blp at nicira.com>
Date: Thu, 2 Aug 2012 11:28:26 -0700
Subject: [PATCH] debian: Configure Debian packages to use /var/lib/openvswitch for conf.db.
Working through symlinks is undesirable when one can avoid it.
The Debian packaging still sets up the symlinks for compatibility with
existing software that expects the database to be in /etc/openvswitch.
Signed-off-by: Ben Pfaff <blp at nicira.com>
---
debian/rules | 4 ++--
utilities/ovs-lib.in | 11 +++++++++--
2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/debian/rules b/debian/rules
index 51a2e51..88a9a28 100755
--- a/debian/rules
+++ b/debian/rules
@@ -38,8 +38,8 @@ configure-stamp:
cd _debian && ( \
test -e Makefile || \
../configure --prefix=/usr --localstatedir=/var --enable-ssl \
- --sysconfdir=/etc CFLAGS="$(CFLAGS)" \
- $(DATAPATH_CONFIGURE_OPTS))
+ --sysconfdir=/etc --with-dbdir=/var/lib/openvswitch \
+ CFLAGS="$(CFLAGS)" $(DATAPATH_CONFIGURE_OPTS))
touch configure-stamp
#Architecture
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
--
1.7.2.5
More information about the dev
mailing list