[Libguestfs] [PATCH] appliance: reorder the steps to search for appliance

[Pavel Butsykin]

On 25.04.2017 18:33, Richard W.M. Jones wrote:
> On Tue, Apr 25, 2017 at 06:13:37PM +0300, Pavel Butsykin wrote:
>> On 25.04.2017 16:04, Richard W.M. Jones wrote:
>>> Can you see what:
>>>
>>>    guestfish get-path
>>
>> /usr/lib64/guestfs
>>
>>> prints?  Are you setting LIBGUESTFS_PATH at all?
>>>
>>
>> No.
>>
>> # ls /usr/lib64/guestfs
>> initrd  kernel  README.fixed  root  supermin.d
>>
>> libguestfs by default uses a single path to search fixed appliance and
>> supermin.d. It seemed to me that the presence of options "--disable
>> appliance --disable-daemon" must exclude the use of
>> build_supermin_appliance.
>> But libguestfs in any case trying to find supermin.d.
>
> OK, I see - what's actually happening is that you've got a path which
> is both a fixed appliance and a supermin appliance (although maybe it
> only appears to be a supermin appliance -- libguestfs just looks for
> the "supermin.d" entry and decides it's a supermin appliance).
>
>> Maybe it's not
>> quite right to make such decisions based on the contents of the
>> directory.
>
> Yes that's right, but see below.
>

Moreover, we have a situation with undefined behavior. if we only use
the fixed appliance, then we can't rely on the contents of directories,
because there are many ways to stumble on supermin.d

>> Also, I still don't understand what the priority of the search
>> appliances was supposed to actually use. Because in the documentation
>> the first step is to search for fixed appliance, but is actually
>> supermin.d(for building appliance).
>
> There's not a priority in this situation.  It wasn't intended that two
> different appliances would be on the exact same path.

As I see using the same path for different appliances is not a problem.
Because build_appliance() first looks for the supermin.d in the
g->path, and then the fixed appliance in the same g->path. Therefore,
setting the two paths into LIBGUESTFS_PATH=/lib64/guestfs/appliance2:
/lib64/guestfs doesn't help.

>
> Some possible solutions:
>
> (1) Make the check in lib/appliance.c: contains_supermin_appliance
> more robust.
>
> It could perhaps be changed so that it checks that at least
> "base.tar.gz" and "packages" are found within the supermin.d
> directory.  Those are the minimum two files that must be in a supermin
> appliance (see supermin(8)).

Empty supermin.d is not a problem in this situation, supermin.d with
content may belong to a different libguestfs version or come from
arbitrary utilities/packages.

> (2) If the fixed appliance was located somewhere else, you could do:
>
>    LIBGUESTFS_PATH=/path/to/somewhere/else:/usr/lib64/guestfs
>    export LIBGUESTFS_PATH
>
> and then it would look for the fixed appliance in
> /path/to/somewhere/else and use it.  If the fixed appliance exists
> there, then it would never check /usr/lib64/guestfs.

It's a bit wrong, the first step is search supermin.d in all specified
directories:
   /* Step (1). */
   r = find_path (g, contains_supermin_appliance, NULL, &supermin_path);
   if (r == -1)
     return -1;

   if (r == 1)
     /* Step (2): build supermin appliance. */
     return build_supermin_appliance (g, supermin_path,
                                      kernel, initrd, appliance);
...

So, supermin.d will be found in /usr/lib64/guestfs and the search for
the fixed applaince will not even. Here find_path() searches for
all paths in g->path.

>
> (3) You could also compile a different path into libguestfs.
>
> Unfortunately there's no way to specify it at configure time, but see
> GUESTFS_DEFAULT_PATH in the sources.
>
> (4) Delete /usr/lib64/guestfs/supermin.d

It can even be dangerous, because supermin.d may belong to any
packages or utilities.

I propose to consider a patch that adds some certainty of behaviour in
the work with appliances, and solves the above notation issue. What do
you think? (see path in the next message)

> Rich.
>