Libguestfs - Oct 2017 - Re: [PATCH 1/2] lib: Allow db_dump package to be a weak dependency (RHBZ#1409024).

On Thursday, 5 October 2017 15:41:31 CEST Richard W.M. Jones
wrote:
> ---
>  lib/inspect-apps.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/inspect-apps.c b/lib/inspect-apps.c
> index f0cf16b38..020a3332c 100644
> --- a/lib/inspect-apps.c
> +++ b/lib/inspect-apps.c
> @@ -122,9 +122,14 @@ guestfs_impl_inspect_list_applications2 (guestfs_h *g,
const char *root)
>    if (STREQ (type, "linux") || STREQ (type, "hurd")) {
>      if (STREQ (package_format, "rpm")) {
>  #ifdef DB_DUMP
> -      ret = list_applications_rpm (g, root);
> -      if (ret == NULL)
> -        return NULL;
> +      /* Allow db_dump to be missing at runtime so that it can be
> +       * installed as a weak dependency.
> +       */
> +      if (system (DB_DUMP " -V >/dev/null 2>&1") == 0)
{
> +        ret = list_applications_rpm (g, root);
> +        if (ret == NULL)
> +          return NULL;
> +      }
>  #endif
This approach has few issues:
a) DB_DUMP is not escaped, so a path with special characters will play
   badly with shell
b) `DB_DUMP -V` is executed every time RPM packages are extracted
c) there is no feedback to the users (not even a debug()) that DB_DUMP
   is not installed, and thus information on the installed RPM packages
   are not extracted
d) the same issue happens for the tools used to extract icons (netpbm,
   and icoutils)

I still do think my approach [1] is better in this regard, since it
already copes with all the points above.

Why should be optional tools searched at build time, hardcoded, and
then still need to be checked at runtime?  The concern that there is
no way to know which tools are used is sort of moot, since the internal
command API (in the library) logs program and arguments.

[1] https://www.redhat.com/archives/libguestfs/2017-February/msg00016.html

-- 
Pino Toscano

On Fri, Oct 06, 2017 at 10:09:03AM +0200, Pino Toscano
wrote:
> On Thursday, 5 October 2017 15:41:31 CEST Richard W.M. Jones wrote:
> > ---
> >  lib/inspect-apps.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/lib/inspect-apps.c b/lib/inspect-apps.c
> > index f0cf16b38..020a3332c 100644
> > --- a/lib/inspect-apps.c
> > +++ b/lib/inspect-apps.c
> > @@ -122,9 +122,14 @@ guestfs_impl_inspect_list_applications2
(guestfs_h *g, const char *root)
> >    if (STREQ (type, "linux") || STREQ (type,
"hurd")) {
> >      if (STREQ (package_format, "rpm")) {
> >  #ifdef DB_DUMP
> > -      ret = list_applications_rpm (g, root);
> > -      if (ret == NULL)
> > -        return NULL;
> > +      /* Allow db_dump to be missing at runtime so that it can be
> > +       * installed as a weak dependency.
> > +       */
> > +      if (system (DB_DUMP " -V >/dev/null 2>&1")
== 0) {
> > +        ret = list_applications_rpm (g, root);
> > +        if (ret == NULL)
> > +          return NULL;
> > +      }
> >  #endif
> 
> This approach has few issues:
> a) DB_DUMP is not escaped, so a path with special characters will play
>    badly with shell
> b) `DB_DUMP -V` is executed every time RPM packages are extracted
> c) there is no feedback to the users (not even a debug()) that DB_DUMP
>    is not installed, and thus information on the installed RPM packages
>    are not extracted
> d) the same issue happens for the tools used to extract icons (netpbm,
>    and icoutils)
> 
> I still do think my approach [1] is better in this regard, since it
> already copes with all the points above.
> 
> Why should be optional tools searched at build time, hardcoded, and
> then still need to be checked at runtime?  The concern that there is
> no way to know which tools are used is sort of moot, since the internal
> command API (in the library) logs program and arguments.
> 
> [1] https://www.redhat.com/archives/libguestfs/2017-February/msg00016.html
I mostly agree with all of what you say, EXCEPT that in the past we've
had cases where people have installed broken binaries on
/usr/local/bin and then reported bugs against libguestfs.

Therefore at configure time we need to store the actual path of the
binaries we want to use (and the binaries that the packager tested
against) and we shouldn't search $PATH at runtime.

(qemu-img is also problematic here, see recent thread on this list)

So some generic mechanism is fine, and looking for binaries at runtime
is also fine, but not one which is searching $PATH.

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 Friday, 6 October 2017 12:23:00 CEST Richard W.M. Jones
wrote:
> On Fri, Oct 06, 2017 at 10:09:03AM +0200, Pino Toscano wrote:
> > On Thursday, 5 October 2017 15:41:31 CEST Richard W.M. Jones wrote:
> > > ---
> > >  lib/inspect-apps.c | 11 ++++++++---
> > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/lib/inspect-apps.c b/lib/inspect-apps.c
> > > index f0cf16b38..020a3332c 100644
> > > --- a/lib/inspect-apps.c
> > > +++ b/lib/inspect-apps.c
> > > @@ -122,9 +122,14 @@ guestfs_impl_inspect_list_applications2
(guestfs_h *g, const char *root)
> > >    if (STREQ (type, "linux") || STREQ (type,
"hurd")) {
> > >      if (STREQ (package_format, "rpm")) {
> > >  #ifdef DB_DUMP
> > > -      ret = list_applications_rpm (g, root);
> > > -      if (ret == NULL)
> > > -        return NULL;
> > > +      /* Allow db_dump to be missing at runtime so that it can
be
> > > +       * installed as a weak dependency.
> > > +       */
> > > +      if (system (DB_DUMP " -V >/dev/null
2>&1") == 0) {
> > > +        ret = list_applications_rpm (g, root);
> > > +        if (ret == NULL)
> > > +          return NULL;
> > > +      }
> > >  #endif
> > 
> > This approach has few issues:
> > a) DB_DUMP is not escaped, so a path with special characters will play
> >    badly with shell
> > b) `DB_DUMP -V` is executed every time RPM packages are extracted
> > c) there is no feedback to the users (not even a debug()) that DB_DUMP
> >    is not installed, and thus information on the installed RPM
packages
> >    are not extracted
> > d) the same issue happens for the tools used to extract icons (netpbm,
> >    and icoutils)
> > 
> > I still do think my approach [1] is better in this regard, since it
> > already copes with all the points above.
> > 
> > Why should be optional tools searched at build time, hardcoded, and
> > then still need to be checked at runtime?  The concern that there is
> > no way to know which tools are used is sort of moot, since the
internal
> > command API (in the library) logs program and arguments.
> > 
> > [1]
https://www.redhat.com/archives/libguestfs/2017-February/msg00016.html
> 
> I mostly agree with all of what you say, EXCEPT that in the past we've
> had cases where people have installed broken binaries on
> /usr/local/bin and then reported bugs against libguestfs.
I would consider this situation broken anyway, and as mentioned above
it will be spotted by looking at the verbose logs (which show the
executable run, and with my changes they are passed as absolute).
> Therefore at configure time we need to store the actual path of the
> binaries we want to use (and the binaries that the packager tested
> against) and we shouldn't search $PATH at runtime.
The current system though will not prevent any of those mistakes if
the user builds libguestfs after installing the binaries in /usr/local
(or anywhere else in $PATH): the configure of libguestfs will pick
them, and thus back at the issue.  Instead, looking them only at
runtime means the user can just cleanup the broken binaries, with no
need to rebuild libguestfs later.
> So some generic mechanism is fine, and looking for binaries at runtime
> is also fine, but not one which is searching $PATH.
If $PATH has broken stuff, then it is a bigger problem than just
libguestfs.  Do not get me wrong: I understand the concern here, but
IMHO it should be better to just rely on the good status of the system
(as it is supposed to be), rather than "hacking around it".

-- 
Pino Toscano