[Libguestfs] [PATCH v2] inspector: recognize ppc64 and ppc64le archs (RHBZ#1211996)

[Maros Zatko]

On 04/17/2015 03:26 PM, Pino Toscano wrote:
> On Friday 17 April 2015 15:15:26 Maros Zatko wrote:
>> On 04/17/2015 10:56 AM, Pino Toscano wrote:
>>> On Thursday 16 April 2015 16:51:48 Maros Zatko wrote:
>>>> ---
>>>>    src/filearch.c | 18 +++++++++++++-----
>>>>    1 file changed, 13 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/src/filearch.c b/src/filearch.c
>>>> index df65c98..5de2959 100644
>>>> --- a/src/filearch.c
>>>> +++ b/src/filearch.c
>>>> @@ -59,8 +59,9 @@ 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, "MSB.*64.*PowerPC", 0)
>>>> +COMPILE_REGEXP (re_elf_ppc64le, "LSB.*64.*PowerPC", 0)
>>>>    
>>>>    /* Convert output from 'file' command on ELF files to the canonical
>>>>     * architecture string.  Caller must free the result.
>>>> @@ -87,6 +88,8 @@ canonical_elf_arch (guestfs_h *g, const char *elf_arch)
>>>>        r = "ia64";
>>>>      else if (match (g, elf_arch, re_elf_ppc64))
>>>>        r = "ppc64";
>>>> +  else if (match (g, elf_arch, re_elf_ppc64le))
>>>> +    r = "ppc64le";
>>>>      else if (strstr (elf_arch, "PowerPC"))
>>>>        r = "ppc";
>>>>      else if (strstr (elf_arch, "ARM aarch64"))
>>>> @@ -114,6 +117,7 @@ magic_for_file (guestfs_h *g, const char *filename, bool *loading_ok,
>>>>    {
>>>>      int flags;
>>>>      CLEANUP_MAGIC_T_FREE magic_t m = NULL;
>>>> +  CLEANUP_FREE char *tmp = NULL;
>>>>      const char *line;
>>>>      char *elf_arch;
>>>>    
>>>> @@ -145,7 +149,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, &tmp, &elf_arch);
>>>>      if (elf_arch == NULL) {
>>> Better switch the checking from elf_arch (which is not touched if
>>> match2 fails, so being an uninitialized pointer) to the return value
>>> of match2.
>>>
>>>>        error (g, "no re_file_elf match in '%s'", line);
>>>>        return NULL;
>>>> @@ -315,6 +319,8 @@ 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;
>>>> +  CLEANUP_FREE char *end_elf_arch = NULL;
>>>>      char *ret = NULL;
>>>>    
>>>>      /* Get the output of the "file" command.  Note that because this
>>>> @@ -324,8 +330,10 @@ 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) {
>>>> +    end_elf_arch = guestfs_int_safe_asprintf (g, "%s %s", endianness, elf_arch);
>>>> +    ret = canonical_elf_arch (g, end_elf_arch);
>>>> +  }
>>> I still do think this approach, i.e. match MSB|LSB, create a new string
>>> with it and have it matched again, is doing the same thing twice for no
>>> reason.
>>>
>>> As I suggested in the v1 review, please do pass the result of the
>>> MSB|LSB match as new parameter for canonical_elf_arch, checking it
>>> when the architecture is ppc64. This will also avoid having a new regex,
>>> re_elf_ppc64le, and leave re_elf_ppc64 as it is currently:
>>>
>>> char *
>>> canonical_elf_arch (guestfs_h *g, const char *endianness,
>>>                       const char *elf_arch)
>>> {
>>>     ...
>>>       else if (match (g, elf_arch, re_elf_ppc64)) {
>>>         if (STREQ (endianness, "LSB")
>>>           r = "ppc64le";
>>>         else
>>>           r = "ppc64";
>>>       }
>>> }
>>>
>>> No need for a new regex, and reuses the new MSB|LSB match as it is.
>> I don't like this because it creates more structured if/elseif that
>> needed and becomes more cluttered.
> Considering the proposed changes to re_file_elf would already give
> a MSB|LSB match, what would be the usefulness of forging a new
> architecture string, different than either the file/magic output and
> where you need to look for MSB|LSB *again*?
>
> Passing the MSB|LSB match would allow also to check for different
> architectures with different endianness (e.g. mips/mipsel), without
> adding new regexps.
>
you won. are you happy?