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

Pavel Butsykin pbutsykin at virtuozzo.com
Thu May 4 14:31:44 UTC 2017


On 04.05.2017 15:34, Richard W.M. Jones wrote:
> 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 at 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.
>

This is not so important for me, I'm just so used. Will fix.

>> +  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.

Thanks, I didn't know about safe_asprintf. Will fix.

>> +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.

Yeah, that's better. Will fix.

> The rest looks fine to me.
>
> Rich.
>




More information about the Libguestfs mailing list