Re: [Libguestfs] [PATCH v1 1/2] appliance: search all types of appliances for each path separately

On Thu, May 04, 2017 at 02:20:28PM +0300, Pavel Butsykin wrote: > This patch changes appliance search using paths with multiple directories. Now > all appliance checks will be done separately for each directory. For example > if the path LIBGUESTFS_PATH=/a:/b:/c, then all applainces are searched first in > /a, then in /b and then in /c. It allows to flexibly configure the libguestfs > to interact with different appliances. > > Signed-off-by: Pavel Butsykin <pbutsykin(a)virtuozzo.com&gt; > --- > lib/appliance.c | 231 ++++++++++++++++++++++++++------------------------------ > 1 file changed, 108 insertions(+), 123 deletions(-) > > diff --git a/lib/appliance.c b/lib/appliance.c > index f12918573..c21525961 100644 > --- a/lib/appliance.c > +++ b/lib/appliance.c > @@ -37,18 +37,23 @@ > #include "guestfs.h" > #include "guestfs-internal.h" > > +typedef struct appliance_files { > + char *kernel; > + char *initrd; > + char *image; > +} appliance_files; As a matter of preference, I think that typedefs hide the meaning of the code, so it becomes unclear what we are passing around as a parameter. Attached is a patch which turns this into a simple struct.

> + r = contains_fixed_appliance (g, path, NULL); > if (r == 1) { > const size_t len = strlen (path); > - *kernel = safe_malloc (g, len + 6 /* "kernel" */ + 2); > - *initrd = safe_malloc (g, len + 6 /* "initrd" */ + 2); > - *appliance = safe_malloc (g, len + 4 /* "root" */ + 2); > - sprintf (*kernel, "%s/kernel", path); > - sprintf (*initrd, "%s/initrd", path); > - sprintf (*appliance, "%s/root", path); > - return 0; > + appliance->kernel = safe_malloc (g, len + 6 /* "kernel" */ + 2); > + appliance->initrd = safe_malloc (g, len + 6 /* "initrd" */ + 2); > + appliance->image = safe_malloc (g, len + 4 /* "root" */ + 2); > + sprintf (appliance->kernel, "%s/kernel", path); > + sprintf (appliance->initrd, "%s/initrd", path); > + sprintf (appliance->image, "%s/root", path); > + return 1; This is copied from the existing code, and isn't wrong, but you might as well simplify it by using safe_asprintf, ie: appliance->kernel = (g, "%s/kernel", path); There are some further cases of the same thing below.

> +static int > +search_appliance (guestfs_h *g, appliance_files *appliance) > +{ > + const char *pelem = g->path; > + > + /* Note that if g->path is an empty string, we want to check the > + * current directory (for backwards compatibility with > + * libguestfs < 1.5.4). > + */ > + do { > + size_t len = strcspn (pelem, PATH_SEPARATOR); > + /* Empty element or "." means current directory. */ > + char *path = (len == 0) ? safe_strdup (g, ".") : > + safe_strndup (g, pelem, len); Better to use: CLEANUP_FREE char *path = ... etc ... and drop the call to free (path) just after.

The rest looks fine to me. Rich.