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

Extend the guestfs_inspect_list_applications2 API call to work on Arch
Linux guest images.
---
 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;
+
+
+  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) {
+      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);
+
+    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 *
-- 
2.1.3

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

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