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.

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.