Libguestfs - Sep 2014 - Re: [PATCH] daemon: augeas: filter out AUG_NO_STDINC from aug-init (RHBZ#1144927)

On Monday 22 September 2014 13:50:18 Richard W.M. Jones
wrote:
> On Mon, Sep 22, 2014 at 10:49:42AM +0200, Pino Toscano wrote:
> > The lenses in our custom path need the system lens for base
> > definitions. Disabling the system path was worthless anyway, since
> > our API does not allow user-specified custom paths.
> > 
> > The only possible use for AUG_NO_STDINC to aug-init could have been
> > to not load the lenses right at init time loading them later;
> > however, this is what the AUG_NO_LOAD flag (= 32) does already.
> > ---
> > 
> >  daemon/augeas.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/daemon/augeas.c b/daemon/augeas.c
> > index ce49726..4f1d9a8 100644
> > --- a/daemon/augeas.c
> > +++ b/daemon/augeas.c
> > @@ -133,6 +133,11 @@ do_aug_init (const char *root, int flags)
> > 
> >      return -1;
> >    
> >    }
> > 
> > +  /* Filter out AUG_NO_STDINC, since the lenses in our custom path
> > +   * need the lenses from the system path.
> > +   */
> > +  flags &= ~AUG_NO_STDINC;
> 
> This is wrong.  It should be possible to specify this, plus it is
> specified in the API.
As I wrote it above, specifying that flag serves no purpose at all. I 
don't see how describing it in API means it will actually be useful, 
especially with the current aug_* API.
> The right solution is to get all the required Augeas lenses into
> Fedora & RHEL 7.1 (backporting the upstream patches if necessary),
> then remove the hacks from libguestfs.
That would imply being incompatible with upstream Augeas, since at least 
the local LVM lens has changes which are not even accepted upstream 
(they have been sent though, nobody looked at them yet).

-- 
Pino Toscano

On Mon, Sep 22, 2014 at 02:56:18PM +0200, Pino Toscano
wrote:
> On Monday 22 September 2014 13:50:18 Richard W.M. Jones wrote:
> > On Mon, Sep 22, 2014 at 10:49:42AM +0200, Pino Toscano wrote:
> > > The lenses in our custom path need the system lens for base
> > > definitions. Disabling the system path was worthless anyway,
since
> > > our API does not allow user-specified custom paths.
> > > 
> > > The only possible use for AUG_NO_STDINC to aug-init could have
been
> > > to not load the lenses right at init time loading them later;
> > > however, this is what the AUG_NO_LOAD flag (= 32) does already.
> > > ---
> > > 
> > >  daemon/augeas.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/daemon/augeas.c b/daemon/augeas.c
> > > index ce49726..4f1d9a8 100644
> > > --- a/daemon/augeas.c
> > > +++ b/daemon/augeas.c
> > > @@ -133,6 +133,11 @@ do_aug_init (const char *root, int flags)
> > > 
> > >      return -1;
> > >    
> > >    }
> > > 
> > > +  /* Filter out AUG_NO_STDINC, since the lenses in our custom
path
> > > +   * need the lenses from the system path.
> > > +   */
> > > +  flags &= ~AUG_NO_STDINC;
> > 
> > This is wrong.  It should be possible to specify this, plus it is
> > specified in the API.
> 
> As I wrote it above, specifying that flag serves no purpose at all. I 
> don't see how describing it in API means it will actually be useful, 
> especially with the current aug_* API.
It's not really clear to me whether setting an entry under
/augeas/load could be used.  In any case ...
> > The right solution is to get all the required Augeas lenses into
> > Fedora & RHEL 7.1 (backporting the upstream patches if necessary),
> > then remove the hacks from libguestfs.
> 
> That would imply being incompatible with upstream Augeas, since at least 
> the local LVM lens has changes which are not even accepted upstream 
> (they have been sent though, nobody looked at them yet).
... the bigger problem is that Augeas lenses are fragile since they
depend on essentially internal details of the standard library.  This
isn't the first time a lens has broken.

We shouldn't be in the business of shipping lenses with libguestfs.
Augeas can ship them and deal with the problems of consistency between
lenses.  If they don't review patches fast enough we should encourage
them to review faster.

What Augeas patch(es) still need review?  I'll see if Dominic or David
can expedite the process for us.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On Monday 22 September 2014 14:09:54 Richard W.M. Jones
wrote:
> On Mon, Sep 22, 2014 at 02:56:18PM +0200, Pino Toscano wrote:
> > On Monday 22 September 2014 13:50:18 Richard W.M. Jones wrote:
> > > On Mon, Sep 22, 2014 at 10:49:42AM +0200, Pino Toscano wrote:
> > > > The lenses in our custom path need the system lens for base
> > > > definitions. Disabling the system path was worthless anyway,
> > > > since
> > > > our API does not allow user-specified custom paths.
> > > > 
> > > > The only possible use for AUG_NO_STDINC to aug-init could
have
> > > > been
> > > > to not load the lenses right at init time loading them
later;
> > > > however, this is what the AUG_NO_LOAD flag (= 32) does
already.
> > > > ---
> > > > 
> > > >  daemon/augeas.c | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/daemon/augeas.c b/daemon/augeas.c
> > > > index ce49726..4f1d9a8 100644
> > > > --- a/daemon/augeas.c
> > > > +++ b/daemon/augeas.c
> > > > @@ -133,6 +133,11 @@ do_aug_init (const char *root, int
flags)
> > > > 
> > > >      return -1;
> > > >    
> > > >    }
> > > > 
> > > > +  /* Filter out AUG_NO_STDINC, since the lenses in our
custom
> > > > path
> > > > +   * need the lenses from the system path.
> > > > +   */
> > > > +  flags &= ~AUG_NO_STDINC;
> > > 
> > > This is wrong.  It should be possible to specify this, plus it is
> > > specified in the API.
> > 
> > As I wrote it above, specifying that flag serves no purpose at all.
> > I
> > don't see how describing it in API means it will actually be
useful,
> > especially with the current aug_* API.
> It's not really clear to me whether setting an entry under
> /augeas/load could be used.
No, it doesn't.

On the other hand, I stand corrected, as we do have a way (although not 
officially recommended) to set environment variables in the daemon, and 
thus set AUGEAS_LENS_LIB. I just doubt anyone actually made use of it, 
though...
> >  In any case ...
> 
> > > The right solution is to get all the required Augeas lenses into
> > > Fedora & RHEL 7.1 (backporting the upstream patches if
necessary),
> > > then remove the hacks from libguestfs.
> > 
> > That would imply being incompatible with upstream Augeas, since at
> > least the local LVM lens has changes which are not even accepted
> > upstream (they have been sent though, nobody looked at them yet).
> 
> ... the bigger problem is that Augeas lenses are fragile since they
> depend on essentially internal details of the standard library.  This
> isn't the first time a lens has broken.
> 
> We shouldn't be in the business of shipping lenses with libguestfs.
> Augeas can ship them and deal with the problems of consistency between
> lenses.
Well, we are not the only one shipping lenses outside of Augeas -- at 
least on my system I see abrt and libvirt shipping lenses. So if they 
decide to break third-party lenses, it's just bad for them
> If they don't review patches fast enough we should encourage
> them to review faster.
> 
> What Augeas patch(es) still need review?  I'll see if Dominic or David
> can expedite the process for us.
- guestfs_lvm_conf.aug: https://github.com/hercules-team/augeas/pull/155
- guestfs_shadow.aug: already upstream, not been any upstream release
  after its inclusion

-- 
Pino Toscano