Libguestfs - May 2017 - Re: [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@virtuozzo.com>
> ---
>  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 = safe_asprintf (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.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v