Libguestfs - Nov 2014 - Re: [PATCH] list-applications: Add support for pacman

OK, I'll make the suggested changes and I'll try to come up with a new
patch by tomorrow or the day after tomorrow. I just noticed that a
space is missing between STREQ and ( in the key assignment code which
violates the project's coding style. I'll fix that too.

For a test-case, I can write a make-archlinux-img.sh script and send
it in another patch. It's not big deal.

Another thing, I left out of the patch the epoch translation. Pacman's
version formats looks like this: epoch:version-rel. You have the epoch
hard-coded to 0 in list_applications_deb() and
list_applications_rpm(), so I did the same but I can implement it for
the sake of completeness. Should I be using the
guestfs___parse_unsigned_int() function for the string to int
conversion?

Nikos

On 17 November 2014 00:16, Richard W.M. Jones <rjones@redhat.com>
wrote:
> On Sun, Nov 16, 2014 at 03:24:16PM +0200, Nikos Skalkotos wrote:
>> Extend the guestfs_inspect_list_applications2 API call to work on Arch
>> Linux guest images.
>
> Generally looks good.  I have a few minor comments inline below.
>
> But also I think we could use a test case (see tests/guests/).  I
> don't think we'd reject the patch for not having a test case, but
the
> test case would ensure it doesn't break or start misbehaving in
> future, so it's good for Arch.
>
>>  src/inspect-apps.c | 125
+++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 125 insertions(+)
>>
>> diff --git a/src/inspect-apps.c b/src/inspect-apps.c
>> index b62b432..b7a3b0e 100644
>> --- a/src/inspect-apps.c
>> +++ b/src/inspect-apps.c
>> @@ -60,6 +60,7 @@
>>  static struct guestfs_application2_list *list_applications_rpm
(guestfs_h *g, struct inspect_fs *fs);
>>  #endif
>>  static struct guestfs_application2_list *list_applications_deb
(guestfs_h *g, struct inspect_fs *fs);
>> +static struct guestfs_application2_list *list_applications_pacman
(guestfs_h *g, struct inspect_fs *fs);
>>  static struct guestfs_application2_list *list_applications_windows
(guestfs_h *g, struct inspect_fs *fs);
>>  static void add_application (guestfs_h *g, struct
guestfs_application2_list *, const char *name, const char *display_name, int32_t
epoch, const char *version, const char *release, const char *arch, const char
*install_path, const char *publisher, const char *url, const char *description);
>>  static void sort_applications (struct guestfs_application2_list *);
>> @@ -145,6 +146,11 @@ guestfs__inspect_list_applications2 (guestfs_h *g,
const char *root)
>>          break;
>>
>>        case OS_PACKAGE_FORMAT_PACMAN:
>> +     ret = list_applications_pacman (g, fs);
>> +     if (ret == NULL)
>> +       return NULL;
>> +     break;
>> +
>>        case OS_PACKAGE_FORMAT_EBUILD:
>>        case OS_PACKAGE_FORMAT_PISI:
>>        case OS_PACKAGE_FORMAT_PKGSRC:
>> @@ -494,6 +500,125 @@ list_applications_deb (guestfs_h *g, struct
inspect_fs *fs)
>>    return ret;
>>  }
>>
>> +static struct guestfs_application2_list *
>> +list_applications_pacman (guestfs_h *g, struct inspect_fs *fs)
>> +{
>> +  CLEANUP_FREE char *desc_file = NULL, *fname = NULL;
>> +  struct guestfs_application2_list *apps = NULL, *ret = NULL;
>> +  struct guestfs_dirent *curr = NULL;
>> +  FILE *fp;
>> +  char line[1024];
>> +  size_t i, len;
>> +  CLEANUP_FREE char *name = NULL, *version = NULL, *desc = NULL;
>> +  CLEANUP_FREE char *arch = NULL, *url = NULL;
>> +  char *release = NULL;
>> +  char **key = NULL;
>> +  const size_t path_len = strlen ("/var/lib/pacman/local/")
+ strlen ("/desc");
>> +  struct guestfs_dirent_list *local_db = NULL;
>> +
>> +  local_db = guestfs_readdir (g, "/var/lib/pacman/local");
>> +  if (local_db == NULL)
>> +    return NULL;
>> +
>> +  /* Allocate 'apps' list. */
>> +  apps = safe_malloc (g, sizeof *apps);
>> +  apps->len = 0;
>> +  apps->val = NULL;
>> +
>> +
>
> Extra blank line here.
>
>> +  for (i = 0; i < local_db->len; i++) {
>> +    curr = &local_db->val[i];
>> +
>> +    if (curr->ftyp != 'd' || STREQ (curr->name,
".") || STREQ (curr->name, ".."))
>> +      continue;
>> +
>> +    free (fname);
>> +    fname = safe_malloc (g, strlen (curr->name) + path_len + 1);
>> +    sprintf (fname, "/var/lib/pacman/local/%s/desc",
curr->name);
>> +    free (desc_file);
>> +    desc_file = guestfs___download_to_tmp (g, fs, fname,
curr->name, 8192);
>> +
>> +    /* The desc files are small (4K). If the desc file does not exist,
or is
>> +     * larger than the 8K limit we've used, the database is
probably corrupted,
>> +     * but we'll continue with the next package anyway.
>> +     */
>> +    if (desc_file == NULL)
>> +      continue;
>> +
>> +    fp = fopen (desc_file, "r");
>> +    if (fp == NULL) {
>> +      perrorf (g, "fopen: %s", desc_file);
>> +      goto out;
>> +    }
>> +
>> +    while (fgets (line, sizeof line, fp) != NULL) {
>
> This might be cleaner if it used getline(3).  I notice that we don't
> use getline in the src/ directory at the moment.  However we do use it
> elsewhere, and we import it from gnulib if it's not provided by libc.
>
> It's also easier to use than fgets / static buffers.
>
>> +      len = strlen (line);
>> +      if (len > 0 && line[len-1] == '\n') {
>> +        line[--len] = '\0';
>> +      }
>> +
>> +      /* empty line */
>> +      if (len == 0) {
>> +        key = NULL;
>> +        continue;
>> +      }
>> +
>> +      if (key != NULL) {
>> +        *key = safe_strdup (g, line);
>> +        key = NULL;
>> +        continue;
>> +      }
>> +
>> +      if (STREQ(line, "%NAME%"))
>> +        key = &name;
>> +      else if (STREQ(line, "%VERSION%"))
>> +        key = &version;
>> +      else if (STREQ(line, "%DESC%"))
>> +        key = &desc;
>> +      else if (STREQ(line, "%URL%"))
>> +        key = &url;
>> +      else if (STREQ(line, "%ARCH%"))
>> +        key = &arch;
>> +    }
>> +
>> +    if (name) {
>> +      if (version) {
>> +        char *p = strchr (version, '-');
>> +        if (p) {
>> +          *p = '\0';
>> +          release = p + 1;
>> +        }
>> +      }
>> +      add_application (g, apps, name, "", 0, version ? :
"", release ? : "",
>> +                       arch ? : "", "",
"", url ? : "", desc ? : "");
>> +    }
>> +
>> +    free (name);
>> +    free (version);
>> +    free (desc);
>> +    free (arch);
>> +    free (url);
>> +    name = version = desc = arch = url = NULL;
>> +    release = NULL; /* Haven't allocated memory for release */
>> +
>> +    if (fclose (fp) == -1) {
>> +      perrorf (g, "fclose: %s", desc_file);
>> +      goto out;
>> +    }
>> +
>> +  }
>> +
>> +  ret = apps;
>> +
>> +  out:
>> +    guestfs_free_dirent_list (local_db);
>
> Maybe use CLEANUP_FREE_DIRENT_LIST instead of having to call
> guestfs_free_dirent_list along the exit path?
>
>> +
>> +    if (ret == NULL)
>> +      guestfs_free_application2_list (apps);
>> +
>> +  return ret;
>> +}
>> +
>>  static void list_applications_windows_from_path (guestfs_h *g, struct
guestfs_application2_list *apps, const char **path, size_t path_len);
>>
>>  static struct guestfs_application2_list *
>
> 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

On Mon, Nov 17, 2014 at 01:41:28PM +0200, Nikos Skalkotos
wrote:
> OK, I'll make the suggested changes and I'll try to come up with a
new
> patch by tomorrow or the day after tomorrow. I just noticed that a
> space is missing between STREQ and ( in the key assignment code which
> violates the project's coding style. I'll fix that too.
> 
> For a test-case, I can write a make-archlinux-img.sh script and send
> it in another patch. It's not big deal.
> 
> Another thing, I left out of the patch the epoch translation. Pacman's
> version formats looks like this: epoch:version-rel. You have the epoch
> hard-coded to 0 in list_applications_deb() and
> list_applications_rpm(),
Hmm, this is a bug ...
> so I did the same but I can implement it for
> the sake of completeness. Should I be using the
> guestfs___parse_unsigned_int() function for the string to int
> conversion?
Yes, or sscanf.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

OK, I'll try to fix the epoch thing for all three.

Nikos


On 17 November 2014 13:49, Richard W.M. Jones <rjones@redhat.com>
wrote:
> On Mon, Nov 17, 2014 at 01:41:28PM +0200, Nikos Skalkotos wrote:
>> OK, I'll make the suggested changes and I'll try to come up
with a new
>> patch by tomorrow or the day after tomorrow. I just noticed that a
>> space is missing between STREQ and ( in the key assignment code which
>> violates the project's coding style. I'll fix that too.
>>
>> For a test-case, I can write a make-archlinux-img.sh script and send
>> it in another patch. It's not big deal.
>>
>> Another thing, I left out of the patch the epoch translation.
Pacman's
>> version formats looks like this: epoch:version-rel. You have the epoch
>> hard-coded to 0 in list_applications_deb() and
>> list_applications_rpm(),
>
> Hmm, this is a bug ...
>
>> so I did the same but I can implement it for
>> the sake of completeness. Should I be using the
>> guestfs___parse_unsigned_int() function for the string to int
>> conversion?
>
> Yes, or sscanf.
>
> Rich.
>
> --
> Richard Jones, Virtualization Group, Red Hat
http://people.redhat.com/~rjones
> Read my programming and virtualization blog: http://rwmj.wordpress.com
> virt-top is 'top' for virtual machines.  Tiny program with many
> powerful monitoring features, net stats, disk stats, logging, etc.
> http://people.redhat.com/~rjones/virt-top