[ovs-dev] [PATCH] dirs: dbdir default must be based on sysconfdir.

[Ben Pfaff]

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 <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.

Crap, that didn't make any sense, did it.  I'll repost the actual
revised patch.