[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