Libguestfs - May 2015 - [PATCH v4 0/2] inspector: recognize ppc64 and ppc64le archs (RHBZ#1211996)

Patch extracts MSB/LSB info from /bin/file output and passes it as separate
parameret from elf_arch. Then it is sent to impl_file_architecture and checked/

Modified magic_for_file to handle regex changes.

Now with tests for file_architecture.

Fixes: RHBZ#1211996

Maros Zatko (2):
  inspector: recognize ppc64 and ppc64le archs (RHBZ#1211996)
  tests: ppc64 and ppc64le for file_architecture

 generator/actions.ml |  8 ++++++++
 src/filearch.c       | 24 +++++++++++++++---------
 2 files changed, 23 insertions(+), 9 deletions(-)

-- 
1.9.3

---
 src/filearch.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/src/filearch.c b/src/filearch.c
index 8fb1acf..ce9d0a1 100644
--- a/src/filearch.c
+++ b/src/filearch.c
@@ -59,14 +59,14 @@ cleanup_magic_t_free (void *ptr)
 # endif
 
 COMPILE_REGEXP (re_file_elf,
-                "ELF.*(?:executable|shared object|relocatable),
(.+?),", 0)
-COMPILE_REGEXP (re_elf_ppc64, "64.*PowerPC", 0)
+                "ELF.*(MSB|LSB).*(?:executable|shared object|relocatable),
(.+?),", 0)
+COMPILE_REGEXP (re_elf_ppc64, ".*64.*PowerPC", 0)
 
 /* Convert output from 'file' command on ELF files to the canonical
  * architecture string.  Caller must free the result.
  */
 static char *
-canonical_elf_arch (guestfs_h *g, const char *elf_arch)
+canonical_elf_arch (guestfs_h *g, const char *endianness, const char *elf_arch)
 {
   const char *r;
   char *ret;
@@ -85,8 +85,12 @@ canonical_elf_arch (guestfs_h *g, const char *elf_arch)
     r = "sparc64";
   else if (strstr (elf_arch, "IA-64"))
     r = "ia64";
-  else if (match (g, elf_arch, re_elf_ppc64))
-    r = "ppc64";
+  else if (match (g, elf_arch, re_elf_ppc64)) {
+    if (strstr (endianness, "MSB"))
+      r = "ppc64";
+    else if (strstr (endianness, "LSB"))
+      r = "ppc64le";
+  }
   else if (strstr (elf_arch, "PowerPC"))
     r = "ppc";
   else if (strstr (elf_arch, "ARM aarch64"))
@@ -116,6 +120,7 @@ magic_for_file (guestfs_h *g, const char *filename, bool
*loading_ok,
   CLEANUP_MAGIC_T_FREE magic_t m = NULL;
   const char *line;
   CLEANUP_FREE char *elf_arch = NULL;
+  CLEANUP_FREE char *endianness = NULL;
 
   flags = g->verbose ? MAGIC_DEBUG : 0;
   flags |= MAGIC_ERROR | MAGIC_RAW;
@@ -145,7 +150,7 @@ magic_for_file (guestfs_h *g, const char *filename, bool
*loading_ok,
   if (loading_ok)
     *loading_ok = true;
 
-  elf_arch = match1 (g, line, re_file_elf);
+  match2 (g, line, re_file_elf, &endianness, &elf_arch);
   if (elf_arch == NULL) {
     error (g, "no re_file_elf match in '%s'", line);
     return NULL;
@@ -154,7 +159,7 @@ magic_for_file (guestfs_h *g, const char *filename, bool
*loading_ok,
   if (matched)
     *matched = true;
 
-  return canonical_elf_arch (g, elf_arch);
+  return canonical_elf_arch (g, endianness, elf_arch);
 }
 
 /* Download and uncompress the cpio file to find binaries within. */
@@ -315,6 +320,7 @@ guestfs_impl_file_architecture (guestfs_h *g, const char
*path)
 {
   CLEANUP_FREE char *file = NULL;
   CLEANUP_FREE char *elf_arch = NULL;
+  CLEANUP_FREE char *endianness = NULL;
   char *ret = NULL;
 
   /* Get the output of the "file" command.  Note that because this
@@ -324,8 +330,8 @@ guestfs_impl_file_architecture (guestfs_h *g, const char
*path)
   if (file == NULL)
     return NULL;
 
-  if ((elf_arch = match1 (g, file, re_file_elf)) != NULL)
-    ret = canonical_elf_arch (g, elf_arch);
+  if ((match2 (g, file, re_file_elf, &endianness, &elf_arch)) != 0)
+    ret = canonical_elf_arch (g, endianness, elf_arch);
   else if (strstr (file, "PE32 executable"))
     ret = safe_strdup (g, "i386");
   else if (strstr (file, "PE32+ executable"))
-- 
1.9.3

relates to RHBZ#1211996
---
 generator/actions.ml | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/generator/actions.ml b/generator/actions.ml
index 1a89869..cf3d525 100644
--- a/generator/actions.ml
+++ b/generator/actions.ml
@@ -788,6 +788,10 @@ to specify the QEMU interface emulation to use at run
time." };
       InitISOFS, Always, TestResultString (
         [["file_architecture"; "/bin-i586-dynamic"]],
"i386"), [];
       InitISOFS, Always, TestResultString (
+        [["file_architecture"; "/bin-ppc64-dynamic"]],
"ppc64"), [];
+      InitISOFS, Always, TestResultString (
+        [["file_architecture"; "/bin-ppc64le-dynamic"]],
"ppc64le"), [];
+      InitISOFS, Always, TestResultString (
         [["file_architecture"; "/bin-sparc-dynamic"]],
"sparc"), [];
       InitISOFS, Always, TestResultString (
         [["file_architecture"; "/bin-win32.exe"]],
"i386"), [];
@@ -802,6 +806,10 @@ to specify the QEMU interface emulation to use at run
time." };
       InitISOFS, Always, TestResultString (
         [["file_architecture"; "/lib-i586.so"]],
"i386"), [];
       InitISOFS, Always, TestResultString (
+        [["file_architecture"; "/lib-ppc64.so"]],
"ppc64"), [];
+      InitISOFS, Always, TestResultString (
+        [["file_architecture"; "/lib-ppc64le.so"]],
"ppc64le"), [];
+      InitISOFS, Always, TestResultString (
         [["file_architecture"; "/lib-sparc.so"]],
"sparc"), [];
       InitISOFS, Always, TestResultString (
         [["file_architecture"; "/lib-win32.dll"]],
"i386"), [];
-- 
1.9.3

On Tuesday 19 May 2015 20:23:37 Maros Zatko wrote:
> ---
>  src/filearch.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/src/filearch.c b/src/filearch.c
> index 8fb1acf..ce9d0a1 100644
> --- a/src/filearch.c
> +++ b/src/filearch.c
> @@ -59,14 +59,14 @@ cleanup_magic_t_free (void *ptr)
>  # endif
>  
>  COMPILE_REGEXP (re_file_elf,
> -                "ELF.*(?:executable|shared object|relocatable),
(.+?),", 0)
> -COMPILE_REGEXP (re_elf_ppc64, "64.*PowerPC", 0)
> +                "ELF.*(MSB|LSB).*(?:executable|shared
object|relocatable), (.+?),", 0)
> +COMPILE_REGEXP (re_elf_ppc64, ".*64.*PowerPC", 0)
>  
>  /* Convert output from 'file' command on ELF files to the
canonical
>   * architecture string.  Caller must free the result.
>   */
>  static char *
> -canonical_elf_arch (guestfs_h *g, const char *elf_arch)
> +canonical_elf_arch (guestfs_h *g, const char *endianness, const char
*elf_arch)
>  {
>    const char *r;
>    char *ret;
> @@ -85,8 +85,12 @@ canonical_elf_arch (guestfs_h *g, const char *elf_arch)
>      r = "sparc64";
>    else if (strstr (elf_arch, "IA-64"))
>      r = "ia64";
> -  else if (match (g, elf_arch, re_elf_ppc64))
> -    r = "ppc64";
> +  else if (match (g, elf_arch, re_elf_ppc64)) {
> +    if (strstr (endianness, "MSB"))
> +      r = "ppc64";
> +    else if (strstr (endianness, "LSB"))
> +      r = "ppc64le";
Can you please add an else branch signalling with error() the unknown
endianness string and returning NULL? Just a small safe guard.

-- 
Pino Toscano

On Tuesday 19 May 2015 20:23:36 Maros Zatko wrote:
> Patch extracts MSB/LSB info from /bin/file output and passes it as separate
> parameret from elf_arch. Then it is sent to impl_file_architecture and
checked/
> 
> Modified magic_for_file to handle regex changes.
> 
> Now with tests for file_architecture.
> 
> Fixes: RHBZ#1211996
> 
> Maros Zatko (2):
>   inspector: recognize ppc64 and ppc64le archs (RHBZ#1211996)
>   tests: ppc64 and ppc64le for file_architecture
Except one small note in patch 1, it seems generally good.

One thing though, could you please merge the tests into the main
commit, so it's more clear they go together?

Thanks for the work,
-- 
Pino Toscano