[PATCH for discussion] lib: update inspect_list_applications to return app_arch
Here's a partially implemented fix for RHBZ#859949. Seeing as this is my first libguestfs patch, I'd like some other eyeballs on it to make sure I've not done anything completely crazy. If the rpm case looks ok, I'll update the deb and windows cases if/where applicable.
(Work in progress) For now this only works for RPM, the others just return an empty string. --- generator/structs.ml | 1 + src/inspect-apps.c | 88 +++++++++++++++++++++++++++++----------------------- 2 files changed, 51 insertions(+), 38 deletions(-) diff --git a/generator/structs.ml b/generator/structs.ml index d62fcc5..8ac9fc5 100644 --- a/generator/structs.ml +++ b/generator/structs.ml @@ -183,6 +183,7 @@ let structs = [ "app_epoch", FInt32; "app_version", FString; "app_release", FString; + "app_arch", FString; "app_install_path", FString; "app_trans_path", FString; "app_publisher", FString; diff --git a/src/inspect-apps.c b/src/inspect-apps.c index f65c70a..3dbfb5b 100644 --- a/src/inspect-apps.c +++ b/src/inspect-apps.c @@ -31,6 +31,7 @@ #ifdef HAVE_ENDIAN_H #include <endian.h> #endif +#include <arpa/inet.h> #include <pcre.h> @@ -46,7 +47,7 @@ static struct guestfs_application_list *list_applications_rpm (guestfs_h *g, str #endif static struct guestfs_application_list *list_applications_deb (guestfs_h *g, struct inspect_fs *fs); static struct guestfs_application_list *list_applications_windows (guestfs_h *g, struct inspect_fs *fs); -static void add_application (guestfs_h *g, struct guestfs_application_list *, const char *name, const char *display_name, int32_t epoch, const char *version, const char *release, const char *install_path, const char *publisher, const char *url, const char *description); +static void add_application (guestfs_h *g, struct guestfs_application_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_application_list *); /* Unlike the simple inspect-get-* calls, this one assumes that the @@ -181,6 +182,37 @@ read_rpm_name (guestfs_h *g, return 0; } +/* tag constants, see rpmtag.h in RPM for complete list */ +#define RPMTAG_VERSION 1001 +#define RPMTAG_RELEASE 1002 +#define RPMTAG_ARCH 1022 + +static char * +get_rpm_header_tag (guestfs_h *g, const void *header_start, size_t header_len, uint32_t tag) +{ + uint32_t num_fields, offset; + const void *cursor = header_start + 8, *store; + + /* This function parses the RPM header structure to pull out various + * tag strings (version, release, arch, etc.). For more detail on the + * header format, see: + * http://www.rpm.org/max-rpm/s1-rpm-file-format-rpm-file-format.html#S2-RPM-FI... + */ + + num_fields = ntohl (*(uint32_t *) header_start); + store = header_start + 8 + (16 * num_fields); + + while (cursor < store ){ + if (ntohl (*(uint32_t *) cursor) == tag){ + offset = ntohl(*(uint32_t *) (cursor + 8)); + return safe_strdup(g, store + offset); + } + cursor += 16; + } + + return NULL; +} + struct read_package_data { struct rpm_names_list *list; struct guestfs_application_list *apps; @@ -194,16 +226,13 @@ read_package (guestfs_h *g, { struct read_package_data *data = datav; struct rpm_name nkey, *entry; - char *p; - size_t len; - ssize_t max; - char *nul_name_nul, *version, *release; + char *version, *release, *arch; /* This function reads one (key, value) pair from the Packages * database. The key is the link field (see struct rpm_name). The - * value is a long binary string, but we can extract the version - * number from it as below. First we have to look up the link field - * in the list of links (which is sorted by link field). + * value is a long binary string, but we can extract the header data + * from it as below. First we have to look up the link field in the + * list of links (which is sorted by link field). */ /* Ignore bogus entries. */ @@ -219,41 +248,23 @@ read_package (guestfs_h *g, /* We found a matching link entry, so that gives us the application * name (entry->name). Now we can get other data for this - * application out of the binary value string. XXX This is a real - * hack. + * application out of the binary value string. */ - /* Look for \0<name>\0 */ - len = strlen (entry->name); - nul_name_nul = safe_malloc (g, len + 2); - nul_name_nul[0] = '\0'; - memcpy (&nul_name_nul[1], entry->name, len); - nul_name_nul[len+1] = '\0'; - p = memmem (value, valuelen, nul_name_nul, len+2); - free (nul_name_nul); - if (!p) - return 0; - - /* Following that are \0-delimited version and release fields. */ - p += len + 2; /* Note we have to skip \0 + name + \0. */ - max = valuelen - (p - (char *) value); - if (max < 0) - max = 0; - version = safe_strndup (g, p, max); - - len = strlen (version); - p += len + 1; - max = valuelen - (p - (char *) value); - if (max < 0) - max = 0; - release = safe_strndup (g, p, max); + version = get_rpm_header_tag(g, value, valuelen, RPMTAG_VERSION); + release = get_rpm_header_tag(g, value, valuelen, RPMTAG_RELEASE); + arch = get_rpm_header_tag(g, value, valuelen, RPMTAG_ARCH); /* Add the application and what we know. */ add_application (g, data->apps, entry->name, "", 0, version, release, - "", "", "", ""); + arch ? arch : "(none)", "", "", "", ""); free (version); free (release); + if (arch) + /* arch will occasionally be NULL, because some packages + * (e.g. "gpgkeys") do not have an arch */ + free (arch); return 0; } @@ -378,7 +389,7 @@ list_applications_deb (guestfs_h *g, struct inspect_fs *fs) else if (STREQ (line, "")) { if (installed_flag && name && version) add_application (g, apps, name, "", 0, version, release ? : "", - "", "", "", ""); + "", "", "", "", ""); free (name); free (version); free (release); @@ -519,7 +530,7 @@ list_applications_windows_from_path (guestfs_h *g, add_application (g, apps, name, display_name, 0, version ? : "", - "", + "", "", install_path ? : "", publisher ? : "", url ? : "", @@ -542,7 +553,7 @@ list_applications_windows_from_path (guestfs_h *g, static void add_application (guestfs_h *g, struct guestfs_application_list *apps, const char *name, const char *display_name, int32_t epoch, - const char *version, const char *release, + const char *version, const char *release, const char *arch, const char *install_path, const char *publisher, const char *url, const char *description) @@ -555,6 +566,7 @@ add_application (guestfs_h *g, struct guestfs_application_list *apps, apps->val[apps->len-1].app_epoch = epoch; apps->val[apps->len-1].app_version = safe_strdup (g, version); apps->val[apps->len-1].app_release = safe_strdup (g, release); + apps->val[apps->len-1].app_arch = safe_strdup (g, arch); apps->val[apps->len-1].app_install_path = safe_strdup (g, install_path); /* XXX Translated path is not implemented yet. */ apps->val[apps->len-1].app_trans_path = safe_strdup (g, ""); -- 1.7.11.4
On Mon, Oct 15, 2012 at 04:30:24PM -0400, John Eckersberg wrote:
(Work in progress)
For now this only works for RPM, the others just return an empty string. --- generator/structs.ml | 1 + src/inspect-apps.c | 88 +++++++++++++++++++++++++++++----------------------- 2 files changed, 51 insertions(+), 38 deletions(-)
diff --git a/generator/structs.ml b/generator/structs.ml index d62fcc5..8ac9fc5 100644 --- a/generator/structs.ml +++ b/generator/structs.ml @@ -183,6 +183,7 @@ let structs = [ "app_epoch", FInt32; "app_version", FString; "app_release", FString; + "app_arch", FString; "app_install_path", FString; "app_trans_path", FString; "app_publisher", FString;
Unfortunately (and this a rather serious shortcoming of the generator) you cannot add fields to an existing struct and maintain backwards binary compatibility. Let's not worry about this bit right now though, I need to think about what to do a bit more.
diff --git a/src/inspect-apps.c b/src/inspect-apps.c index f65c70a..3dbfb5b 100644 --- a/src/inspect-apps.c +++ b/src/inspect-apps.c @@ -31,6 +31,7 @@ #ifdef HAVE_ENDIAN_H #include <endian.h> #endif +#include <arpa/inet.h>
Don't need this -- see below.
#include <pcre.h>
@@ -46,7 +47,7 @@ static struct guestfs_application_list *list_applications_rpm (guestfs_h *g, str #endif static struct guestfs_application_list *list_applications_deb (guestfs_h *g, struct inspect_fs *fs); static struct guestfs_application_list *list_applications_windows (guestfs_h *g, struct inspect_fs *fs); -static void add_application (guestfs_h *g, struct guestfs_application_list *, const char *name, const char *display_name, int32_t epoch, const char *version, const char *release, const char *install_path, const char *publisher, const char *url, const char *description); +static void add_application (guestfs_h *g, struct guestfs_application_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_application_list *);
/* Unlike the simple inspect-get-* calls, this one assumes that the @@ -181,6 +182,37 @@ read_rpm_name (guestfs_h *g, return 0; }
+/* tag constants, see rpmtag.h in RPM for complete list */ +#define RPMTAG_VERSION 1001 +#define RPMTAG_RELEASE 1002 +#define RPMTAG_ARCH 1022 + +static char * +get_rpm_header_tag (guestfs_h *g, const void *header_start, size_t header_len, uint32_t tag) +{ + uint32_t num_fields, offset; + const void *cursor = header_start + 8, *store; + + /* This function parses the RPM header structure to pull out various + * tag strings (version, release, arch, etc.). For more detail on the + * header format, see: + * http://www.rpm.org/max-rpm/s1-rpm-file-format-rpm-file-format.html#S2-RPM-FI... + */ + + num_fields = ntohl (*(uint32_t *) header_start);
It's probably better to use the functions from <endian.h> like 'be32toh', 'le32toh' etc. We already use them, eg. in src/inspect-fs-windows.c.
+ store = header_start + 8 + (16 * num_fields); + + while (cursor < store ){ + if (ntohl (*(uint32_t *) cursor) == tag){ + offset = ntohl(*(uint32_t *) (cursor + 8)); + return safe_strdup(g, store + offset); + } + cursor += 16;
Can a malicious RPM in the guest cause an infinite loop in the host?
+ } + + return NULL; +} + struct read_package_data { struct rpm_names_list *list; struct guestfs_application_list *apps; @@ -194,16 +226,13 @@ read_package (guestfs_h *g, { struct read_package_data *data = datav; struct rpm_name nkey, *entry; - char *p; - size_t len; - ssize_t max; - char *nul_name_nul, *version, *release; + char *version, *release, *arch;
/* This function reads one (key, value) pair from the Packages * database. The key is the link field (see struct rpm_name). The - * value is a long binary string, but we can extract the version - * number from it as below. First we have to look up the link field - * in the list of links (which is sorted by link field). + * value is a long binary string, but we can extract the header data + * from it as below. First we have to look up the link field in the + * list of links (which is sorted by link field). */
/* Ignore bogus entries. */ @@ -219,41 +248,23 @@ read_package (guestfs_h *g,
/* We found a matching link entry, so that gives us the application * name (entry->name). Now we can get other data for this - * application out of the binary value string. XXX This is a real - * hack. + * application out of the binary value string. */
- /* Look for \0<name>\0 */ - len = strlen (entry->name); - nul_name_nul = safe_malloc (g, len + 2); - nul_name_nul[0] = '\0'; - memcpy (&nul_name_nul[1], entry->name, len); - nul_name_nul[len+1] = '\0'; - p = memmem (value, valuelen, nul_name_nul, len+2); - free (nul_name_nul); - if (!p) - return 0; - - /* Following that are \0-delimited version and release fields. */ - p += len + 2; /* Note we have to skip \0 + name + \0. */ - max = valuelen - (p - (char *) value); - if (max < 0) - max = 0; - version = safe_strndup (g, p, max); - - len = strlen (version); - p += len + 1; - max = valuelen - (p - (char *) value); - if (max < 0) - max = 0; - release = safe_strndup (g, p, max); + version = get_rpm_header_tag(g, value, valuelen, RPMTAG_VERSION); + release = get_rpm_header_tag(g, value, valuelen, RPMTAG_RELEASE); + arch = get_rpm_header_tag(g, value, valuelen, RPMTAG_ARCH);
Good, this seems like a considerable improvement.
/* Add the application and what we know. */ add_application (g, data->apps, entry->name, "", 0, version, release, - "", "", "", ""); + arch ? arch : "(none)", "", "", "", "");
Don't use "(none)". If you need a special value, use either "" or "unknown" (I suspect in this case empty string), and document it in the API (generator/actions.ml).
free (version); free (release); + if (arch) + /* arch will occasionally be NULL, because some packages + * (e.g. "gpgkeys") do not have an arch */ + free (arch);
return 0; } @@ -378,7 +389,7 @@ list_applications_deb (guestfs_h *g, struct inspect_fs *fs) else if (STREQ (line, "")) { if (installed_flag && name && version) add_application (g, apps, name, "", 0, version, release ? : "", - "", "", "", ""); + "", "", "", "", ""); free (name); free (version); free (release); @@ -519,7 +530,7 @@ list_applications_windows_from_path (guestfs_h *g,
add_application (g, apps, name, display_name, 0, version ? : "", - "", + "", "", install_path ? : "", publisher ? : "", url ? : "", @@ -542,7 +553,7 @@ list_applications_windows_from_path (guestfs_h *g, static void add_application (guestfs_h *g, struct guestfs_application_list *apps, const char *name, const char *display_name, int32_t epoch, - const char *version, const char *release, + const char *version, const char *release, const char *arch, const char *install_path, const char *publisher, const char *url, const char *description) @@ -555,6 +566,7 @@ add_application (guestfs_h *g, struct guestfs_application_list *apps, apps->val[apps->len-1].app_epoch = epoch; apps->val[apps->len-1].app_version = safe_strdup (g, version); apps->val[apps->len-1].app_release = safe_strdup (g, release); + apps->val[apps->len-1].app_arch = safe_strdup (g, arch); apps->val[apps->len-1].app_install_path = safe_strdup (g, install_path); /* XXX Translated path is not implemented yet. */ apps->val[apps->len-1].app_trans_path = safe_strdup (g, ""); -- 1.7.11.4
Does it work? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into Xen guests. http://et.redhat.com/~rjones/virt-p2v
On Mon, Oct 15, 2012 at 10:00:15PM +0100, Richard W.M. Jones wrote:
+ if (arch) + free (arch);
BTW you don't need to write if (foo) free (foo); because free (NULL) is perfectly OK in modern C. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top
"Richard W.M. Jones" <rjones@redhat.com> writes:
On Mon, Oct 15, 2012 at 04:30:24PM -0400, John Eckersberg wrote:
(Work in progress)
For now this only works for RPM, the others just return an empty string. --- generator/structs.ml | 1 + src/inspect-apps.c | 88 +++++++++++++++++++++++++++++----------------------- 2 files changed, 51 insertions(+), 38 deletions(-)
diff --git a/generator/structs.ml b/generator/structs.ml index d62fcc5..8ac9fc5 100644 --- a/generator/structs.ml +++ b/generator/structs.ml @@ -183,6 +183,7 @@ let structs = [ "app_epoch", FInt32; "app_version", FString; "app_release", FString; + "app_arch", FString; "app_install_path", FString; "app_trans_path", FString; "app_publisher", FString;
Unfortunately (and this a rather serious shortcoming of the generator) you cannot add fields to an existing struct and maintain backwards binary compatibility. Let's not worry about this bit right now though, I need to think about what to do a bit more.
diff --git a/src/inspect-apps.c b/src/inspect-apps.c index f65c70a..3dbfb5b 100644 --- a/src/inspect-apps.c +++ b/src/inspect-apps.c @@ -31,6 +31,7 @@ #ifdef HAVE_ENDIAN_H #include <endian.h> #endif +#include <arpa/inet.h>
Don't need this -- see below.
#include <pcre.h>
@@ -46,7 +47,7 @@ static struct guestfs_application_list *list_applications_rpm (guestfs_h *g, str #endif static struct guestfs_application_list *list_applications_deb (guestfs_h *g, struct inspect_fs *fs); static struct guestfs_application_list *list_applications_windows (guestfs_h *g, struct inspect_fs *fs); -static void add_application (guestfs_h *g, struct guestfs_application_list *, const char *name, const char *display_name, int32_t epoch, const char *version, const char *release, const char *install_path, const char *publisher, const char *url, const char *description); +static void add_application (guestfs_h *g, struct guestfs_application_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_application_list *);
/* Unlike the simple inspect-get-* calls, this one assumes that the @@ -181,6 +182,37 @@ read_rpm_name (guestfs_h *g, return 0; }
+/* tag constants, see rpmtag.h in RPM for complete list */ +#define RPMTAG_VERSION 1001 +#define RPMTAG_RELEASE 1002 +#define RPMTAG_ARCH 1022 + +static char * +get_rpm_header_tag (guestfs_h *g, const void *header_start, size_t header_len, uint32_t tag) +{ + uint32_t num_fields, offset; + const void *cursor = header_start + 8, *store; + + /* This function parses the RPM header structure to pull out various + * tag strings (version, release, arch, etc.). For more detail on the + * header format, see: + * http://www.rpm.org/max-rpm/s1-rpm-file-format-rpm-file-format.html#S2-RPM-FI... + */ + + num_fields = ntohl (*(uint32_t *) header_start);
It's probably better to use the functions from <endian.h> like 'be32toh', 'le32toh' etc. We already use them, eg. in src/inspect-fs-windows.c.
Ok, I'll revise to use those instead.
+ store = header_start + 8 + (16 * num_fields); + + while (cursor < store ){ + if (ntohl (*(uint32_t *) cursor) == tag){ + offset = ntohl(*(uint32_t *) (cursor + 8)); + return safe_strdup(g, store + offset); + } + cursor += 16;
Can a malicious RPM in the guest cause an infinite loop in the host?
As the patch is presently written, one could create a malicious RPM such that the value read into num_fields is prohibitively large, but I'd expect it to segfault once cursor overruns the memory for the header. This should be bound by header_len, which somewhere along the way in my refactorings I botched. Good catch, I'll fix that.
+ } + + return NULL; +} + struct read_package_data { struct rpm_names_list *list; struct guestfs_application_list *apps; @@ -194,16 +226,13 @@ read_package (guestfs_h *g, { struct read_package_data *data = datav; struct rpm_name nkey, *entry; - char *p; - size_t len; - ssize_t max; - char *nul_name_nul, *version, *release; + char *version, *release, *arch;
/* This function reads one (key, value) pair from the Packages * database. The key is the link field (see struct rpm_name). The - * value is a long binary string, but we can extract the version - * number from it as below. First we have to look up the link field - * in the list of links (which is sorted by link field). + * value is a long binary string, but we can extract the header data + * from it as below. First we have to look up the link field in the + * list of links (which is sorted by link field). */
/* Ignore bogus entries. */ @@ -219,41 +248,23 @@ read_package (guestfs_h *g,
/* We found a matching link entry, so that gives us the application * name (entry->name). Now we can get other data for this - * application out of the binary value string. XXX This is a real - * hack. + * application out of the binary value string. */
- /* Look for \0<name>\0 */ - len = strlen (entry->name); - nul_name_nul = safe_malloc (g, len + 2); - nul_name_nul[0] = '\0'; - memcpy (&nul_name_nul[1], entry->name, len); - nul_name_nul[len+1] = '\0'; - p = memmem (value, valuelen, nul_name_nul, len+2); - free (nul_name_nul); - if (!p) - return 0; - - /* Following that are \0-delimited version and release fields. */ - p += len + 2; /* Note we have to skip \0 + name + \0. */ - max = valuelen - (p - (char *) value); - if (max < 0) - max = 0; - version = safe_strndup (g, p, max); - - len = strlen (version); - p += len + 1; - max = valuelen - (p - (char *) value); - if (max < 0) - max = 0; - release = safe_strndup (g, p, max); + version = get_rpm_header_tag(g, value, valuelen, RPMTAG_VERSION); + release = get_rpm_header_tag(g, value, valuelen, RPMTAG_RELEASE); + arch = get_rpm_header_tag(g, value, valuelen, RPMTAG_ARCH);
Good, this seems like a considerable improvement.
/* Add the application and what we know. */ add_application (g, data->apps, entry->name, "", 0, version, release, - "", "", "", ""); + arch ? arch : "(none)", "", "", "", "");
Don't use "(none)". If you need a special value, use either "" or "unknown" (I suspect in this case empty string), and document it in the API (generator/actions.ml).
Aye. I originally used just "" but I changed it to match the output that RPM gives. I'll put it back.
free (version); free (release); + if (arch) + /* arch will occasionally be NULL, because some packages + * (e.g. "gpgkeys") do not have an arch */ + free (arch);
return 0; } @@ -378,7 +389,7 @@ list_applications_deb (guestfs_h *g, struct inspect_fs *fs) else if (STREQ (line, "")) { if (installed_flag && name && version) add_application (g, apps, name, "", 0, version, release ? : "", - "", "", "", ""); + "", "", "", "", ""); free (name); free (version); free (release); @@ -519,7 +530,7 @@ list_applications_windows_from_path (guestfs_h *g,
add_application (g, apps, name, display_name, 0, version ? : "", - "", + "", "", install_path ? : "", publisher ? : "", url ? : "", @@ -542,7 +553,7 @@ list_applications_windows_from_path (guestfs_h *g, static void add_application (guestfs_h *g, struct guestfs_application_list *apps, const char *name, const char *display_name, int32_t epoch, - const char *version, const char *release, + const char *version, const char *release, const char *arch, const char *install_path, const char *publisher, const char *url, const char *description) @@ -555,6 +566,7 @@ add_application (guestfs_h *g, struct guestfs_application_list *apps, apps->val[apps->len-1].app_epoch = epoch; apps->val[apps->len-1].app_version = safe_strdup (g, version); apps->val[apps->len-1].app_release = safe_strdup (g, release); + apps->val[apps->len-1].app_arch = safe_strdup (g, arch); apps->val[apps->len-1].app_install_path = safe_strdup (g, install_path); /* XXX Translated path is not implemented yet. */ apps->val[apps->len-1].app_trans_path = safe_strdup (g, ""); -- 1.7.11.4
Does it work?
I tested it against a F17 guest I had, and it correctly pulls the arch out and displays it. However, I just realized that it doesn't return both versions of the package. For example, if I have glibc.x86_64 and glibc.i686 installed, only one of them is returned in the output. I need to fix that as well. But the header parsing works.
Two more patches for this. The first one contains only the changes we've already discussed. The second one just adds arch to the virt-inspector output. I split it out since we haven't discussed that one yet.
--- generator/actions.ml | 6 ++++++ src/inspect-apps.c | 46 +++++++++++++++++++++++++--------------------- 2 files changed, 31 insertions(+), 21 deletions(-) diff --git a/generator/actions.ml b/generator/actions.ml index dc71234..afafe9b 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -1527,6 +1527,12 @@ The release string of the application or package, for package managers that use this. If unavailable this is returned as an empty string C<\"\">. +=item C<app_arch> + +The architecture string of the application or package, for package +managers that use this. If unavailable this is returned as an empty +string C<\"\">. + =item C<app_install_path> The installation path of the application (on operating systems diff --git a/src/inspect-apps.c b/src/inspect-apps.c index 3dbfb5b..c8e586f 100644 --- a/src/inspect-apps.c +++ b/src/inspect-apps.c @@ -31,7 +31,6 @@ #ifdef HAVE_ENDIAN_H #include <endian.h> #endif -#include <arpa/inet.h> #include <pcre.h> @@ -126,7 +125,7 @@ guestfs__inspect_list_applications (guestfs_h *g, const char *root) #ifdef DB_DUMP /* This data comes from the Name database, and contains the application - * names and the first 4 bytes of the link field. + * names and the first 4 bytes of each link field. */ struct rpm_names_list { struct rpm_name *names; @@ -162,22 +161,30 @@ read_rpm_name (guestfs_h *g, void *listv) { struct rpm_names_list *list = listv; + const unsigned char *link_p; char *name; /* Ignore bogus entries. */ if (keylen == 0 || valuelen < 4) return 0; - /* The name (key) field won't be NUL-terminated, so we must do that. */ - name = safe_malloc (g, keylen+1); - memcpy (name, key, keylen); - name[keylen] = '\0'; - - list->names = safe_realloc (g, list->names, - (list->len + 1) * sizeof (struct rpm_name)); - list->names[list->len].name = name; - memcpy (list->names[list->len].link, value, 4); - list->len++; + /* A name entry will have as many links as installed instances of + * that pacakge. For example, if glibc.i686 and glibc.x86_64 are + * both installed, then there will be a link for each Packages + * entry. Add an entry onto list for all installed instances. + */ + for (link_p = value; link_p < value + valuelen; link_p += 8) { + /* The name (key) field won't be NUL-terminated, so we must do that. */ + name = safe_malloc (g, keylen+1); + memcpy (name, key, keylen); + name[keylen] = '\0'; + + list->names = safe_realloc (g, list->names, + (list->len + 1) * sizeof (struct rpm_name)); + list->names[list->len].name = name; + memcpy (list->names[list->len].link, link_p, 4); + list->len++; + } return 0; } @@ -199,12 +206,12 @@ get_rpm_header_tag (guestfs_h *g, const void *header_start, size_t header_len, u * http://www.rpm.org/max-rpm/s1-rpm-file-format-rpm-file-format.html#S2-RPM-FI... */ - num_fields = ntohl (*(uint32_t *) header_start); + num_fields = be32toh (*(uint32_t *) header_start); store = header_start + 8 + (16 * num_fields); - while (cursor < store ){ - if (ntohl (*(uint32_t *) cursor) == tag){ - offset = ntohl(*(uint32_t *) (cursor + 8)); + while (cursor < store && cursor < header_start + header_len) { + if (be32toh (*(uint32_t *) cursor) == tag){ + offset = be32toh(*(uint32_t *) (cursor + 8)); return safe_strdup(g, store + offset); } cursor += 16; @@ -257,14 +264,11 @@ read_package (guestfs_h *g, /* Add the application and what we know. */ add_application (g, data->apps, entry->name, "", 0, version, release, - arch ? arch : "(none)", "", "", "", ""); + arch ? arch : "", "", "", "", ""); free (version); free (release); - if (arch) - /* arch will occasionally be NULL, because some packages - * (e.g. "gpgkeys") do not have an arch */ - free (arch); + free (arch); return 0; } -- 1.7.11.4
On Tue, Oct 16, 2012 at 04:33:34PM -0400, John Eckersberg wrote:
--- generator/actions.ml | 6 ++++++ src/inspect-apps.c | 46 +++++++++++++++++++++++++--------------------- 2 files changed, 31 insertions(+), 21 deletions(-)
diff --git a/generator/actions.ml b/generator/actions.ml index dc71234..afafe9b 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -1527,6 +1527,12 @@ The release string of the application or package, for package managers that use this. If unavailable this is returned as an empty string C<\"\">.
+=item C<app_arch> + +The architecture string of the application or package, for package +managers that use this. If unavailable this is returned as an empty +string C<\"\">. +
We still can't add app_arch directly to this structure, because it will change the size of the struct, breaking the ABI for existing callers. There are some alternatives, none great I'm afraid: (1) Reuse some existing field that would never be used by RPM (app_install_path? -- think this is used by Windows only). However this would stop us from adding arch support on Windows. (2) Create a new call inspect_list_applications2 and a new struct application2. (3) Same as (2), but disguise this using linker symbol versioning. This would be the best option but is rather complex to implement. However libvirt and glibc both provide examples of how to do it. In the meantime, one thing that strikes me about this patch is that it's split up wrong. There is a part of this patch which simplifies the parsing of the existing name and version fields. And there is another part which adds support for architecture (including adding it to virt-inspector). The first patch could be applied immediately, leaving the complex issue above to the second patch. The current split doesn't really make sense.
+ name = safe_malloc (g, keylen+1); + memcpy (name, key, keylen); + name[keylen] = '\0';
safe_strndup I think?
- num_fields = ntohl (*(uint32_t *) header_start); + num_fields = be32toh (*(uint32_t *) header_start);
I now see what's confusing me about this patch .. it seems to be applied on top of the existing patches? Best to post a completely new series against the current HEAD. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones New in Fedora 11: Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 70 libraries supprt'd http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw
(Resending to the list since I fat-fingered the address the first time...) Here's the series rebased and squashed down into one commit. Also I added in the strndup suggestion. Still ignoring the fact that it breaks the ABI for now until we decide what to do about that.
(Work in progress) For now this only works for RPM, the others just return an empty string. --- generator/actions.ml | 6 +++ generator/structs.ml | 1 + inspector/virt-inspector.c | 4 ++ src/inspect-apps.c | 111 +++++++++++++++++++++++++-------------------- 4 files changed, 73 insertions(+), 49 deletions(-) diff --git a/generator/actions.ml b/generator/actions.ml index 71aee37..3ad98fc 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -1571,6 +1571,12 @@ The release string of the application or package, for package managers that use this. If unavailable this is returned as an empty string C<\"\">. +=item C<app_arch> + +The architecture string of the application or package, for package +managers that use this. If unavailable this is returned as an empty +string C<\"\">. + =item C<app_install_path> The installation path of the application (on operating systems diff --git a/generator/structs.ml b/generator/structs.ml index d62fcc5..8ac9fc5 100644 --- a/generator/structs.ml +++ b/generator/structs.ml @@ -183,6 +183,7 @@ let structs = [ "app_epoch", FInt32; "app_version", FString; "app_release", FString; + "app_arch", FString; "app_install_path", FString; "app_trans_path", FString; "app_publisher", FString; diff --git a/inspector/virt-inspector.c b/inspector/virt-inspector.c index 8b06fa7..3ac14a7 100644 --- a/inspector/virt-inspector.c +++ b/inspector/virt-inspector.c @@ -747,6 +747,10 @@ output_applications (xmlTextWriterPtr xo, char *root) XMLERROR (-1, xmlTextWriterWriteElement (xo, BAD_CAST "release", BAD_CAST apps->val[i].app_release)); + if (apps->val[i].app_arch && apps->val[i].app_arch[0]) + XMLERROR (-1, + xmlTextWriterWriteElement (xo, BAD_CAST "arch", + BAD_CAST apps->val[i].app_arch)); if (apps->val[i].app_install_path && apps->val[i].app_install_path[0]) XMLERROR (-1, xmlTextWriterWriteElement (xo, BAD_CAST "install_path", diff --git a/src/inspect-apps.c b/src/inspect-apps.c index f65c70a..e272513 100644 --- a/src/inspect-apps.c +++ b/src/inspect-apps.c @@ -46,7 +46,7 @@ static struct guestfs_application_list *list_applications_rpm (guestfs_h *g, str #endif static struct guestfs_application_list *list_applications_deb (guestfs_h *g, struct inspect_fs *fs); static struct guestfs_application_list *list_applications_windows (guestfs_h *g, struct inspect_fs *fs); -static void add_application (guestfs_h *g, struct guestfs_application_list *, const char *name, const char *display_name, int32_t epoch, const char *version, const char *release, const char *install_path, const char *publisher, const char *url, const char *description); +static void add_application (guestfs_h *g, struct guestfs_application_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_application_list *); /* Unlike the simple inspect-get-* calls, this one assumes that the @@ -125,7 +125,7 @@ guestfs__inspect_list_applications (guestfs_h *g, const char *root) #ifdef DB_DUMP /* This data comes from the Name database, and contains the application - * names and the first 4 bytes of the link field. + * names and the first 4 bytes of each link field. */ struct rpm_names_list { struct rpm_name *names; @@ -161,26 +161,62 @@ read_rpm_name (guestfs_h *g, void *listv) { struct rpm_names_list *list = listv; + const unsigned char *link_p; char *name; /* Ignore bogus entries. */ if (keylen == 0 || valuelen < 4) return 0; - /* The name (key) field won't be NUL-terminated, so we must do that. */ - name = safe_malloc (g, keylen+1); - memcpy (name, key, keylen); - name[keylen] = '\0'; - - list->names = safe_realloc (g, list->names, - (list->len + 1) * sizeof (struct rpm_name)); - list->names[list->len].name = name; - memcpy (list->names[list->len].link, value, 4); - list->len++; + /* A name entry will have as many links as installed instances of + * that pacakge. For example, if glibc.i686 and glibc.x86_64 are + * both installed, then there will be a link for each Packages + * entry. Add an entry onto list for all installed instances. + */ + for (link_p = value; link_p < value + valuelen; link_p += 8) { + name = safe_strndup (g, key, keylen); + + list->names = safe_realloc (g, list->names, + (list->len + 1) * sizeof (struct rpm_name)); + list->names[list->len].name = name; + memcpy (list->names[list->len].link, link_p, 4); + list->len++; + } return 0; } +/* tag constants, see rpmtag.h in RPM for complete list */ +#define RPMTAG_VERSION 1001 +#define RPMTAG_RELEASE 1002 +#define RPMTAG_ARCH 1022 + +static char * +get_rpm_header_tag (guestfs_h *g, const void *header_start, size_t header_len, uint32_t tag) +{ + uint32_t num_fields, offset; + const void *cursor = header_start + 8, *store; + + /* This function parses the RPM header structure to pull out various + * tag strings (version, release, arch, etc.). For more detail on the + * header format, see: + * http://www.rpm.org/max-rpm/s1-rpm-file-format-rpm-file-format.html#S2-RPM-FI... + */ + + num_fields = be32toh (*(uint32_t *) header_start); + store = header_start + 8 + (16 * num_fields); + + while (cursor < store && cursor < header_start + header_len) { + if (be32toh (*(uint32_t *) cursor) == tag){ + offset = be32toh(*(uint32_t *) (cursor + 8)); + return safe_strdup(g, store + offset); + } + cursor += 16; + } + + return NULL; +} + struct read_package_data { struct rpm_names_list *list; struct guestfs_application_list *apps; @@ -194,16 +230,13 @@ read_package (guestfs_h *g, { struct read_package_data *data = datav; struct rpm_name nkey, *entry; - char *p; - size_t len; - ssize_t max; - char *nul_name_nul, *version, *release; + char *version, *release, *arch; /* This function reads one (key, value) pair from the Packages * database. The key is the link field (see struct rpm_name). The - * value is a long binary string, but we can extract the version - * number from it as below. First we have to look up the link field - * in the list of links (which is sorted by link field). + * value is a long binary string, but we can extract the header data + * from it as below. First we have to look up the link field in the + * list of links (which is sorted by link field). */ /* Ignore bogus entries. */ @@ -219,41 +252,20 @@ read_package (guestfs_h *g, /* We found a matching link entry, so that gives us the application * name (entry->name). Now we can get other data for this - * application out of the binary value string. XXX This is a real - * hack. + * application out of the binary value string. */ - /* Look for \0<name>\0 */ - len = strlen (entry->name); - nul_name_nul = safe_malloc (g, len + 2); - nul_name_nul[0] = '\0'; - memcpy (&nul_name_nul[1], entry->name, len); - nul_name_nul[len+1] = '\0'; - p = memmem (value, valuelen, nul_name_nul, len+2); - free (nul_name_nul); - if (!p) - return 0; - - /* Following that are \0-delimited version and release fields. */ - p += len + 2; /* Note we have to skip \0 + name + \0. */ - max = valuelen - (p - (char *) value); - if (max < 0) - max = 0; - version = safe_strndup (g, p, max); - - len = strlen (version); - p += len + 1; - max = valuelen - (p - (char *) value); - if (max < 0) - max = 0; - release = safe_strndup (g, p, max); + version = get_rpm_header_tag(g, value, valuelen, RPMTAG_VERSION); + release = get_rpm_header_tag(g, value, valuelen, RPMTAG_RELEASE); + arch = get_rpm_header_tag(g, value, valuelen, RPMTAG_ARCH); /* Add the application and what we know. */ add_application (g, data->apps, entry->name, "", 0, version, release, - "", "", "", ""); + arch ? arch : "", "", "", "", ""); free (version); free (release); + free (arch); return 0; } @@ -378,7 +390,7 @@ list_applications_deb (guestfs_h *g, struct inspect_fs *fs) else if (STREQ (line, "")) { if (installed_flag && name && version) add_application (g, apps, name, "", 0, version, release ? : "", - "", "", "", ""); + "", "", "", "", ""); free (name); free (version); free (release); @@ -519,7 +531,7 @@ list_applications_windows_from_path (guestfs_h *g, add_application (g, apps, name, display_name, 0, version ? : "", - "", + "", "", install_path ? : "", publisher ? : "", url ? : "", @@ -542,7 +554,7 @@ list_applications_windows_from_path (guestfs_h *g, static void add_application (guestfs_h *g, struct guestfs_application_list *apps, const char *name, const char *display_name, int32_t epoch, - const char *version, const char *release, + const char *version, const char *release, const char *arch, const char *install_path, const char *publisher, const char *url, const char *description) @@ -555,6 +567,7 @@ add_application (guestfs_h *g, struct guestfs_application_list *apps, apps->val[apps->len-1].app_epoch = epoch; apps->val[apps->len-1].app_version = safe_strdup (g, version); apps->val[apps->len-1].app_release = safe_strdup (g, release); + apps->val[apps->len-1].app_arch = safe_strdup (g, arch); apps->val[apps->len-1].app_install_path = safe_strdup (g, install_path); /* XXX Translated path is not implemented yet. */ apps->val[apps->len-1].app_trans_path = safe_strdup (g, ""); -- 1.7.11.7
--- inspector/virt-inspector.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/inspector/virt-inspector.c b/inspector/virt-inspector.c index 8b06fa7..3ac14a7 100644 --- a/inspector/virt-inspector.c +++ b/inspector/virt-inspector.c @@ -747,6 +747,10 @@ output_applications (xmlTextWriterPtr xo, char *root) XMLERROR (-1, xmlTextWriterWriteElement (xo, BAD_CAST "release", BAD_CAST apps->val[i].app_release)); + if (apps->val[i].app_arch && apps->val[i].app_arch[0]) + XMLERROR (-1, + xmlTextWriterWriteElement (xo, BAD_CAST "arch", + BAD_CAST apps->val[i].app_arch)); if (apps->val[i].app_install_path && apps->val[i].app_install_path[0]) XMLERROR (-1, xmlTextWriterWriteElement (xo, BAD_CAST "install_path", -- 1.7.11.4
participants (2)
-
John Eckersberg -
Richard W.M. Jones