[PATCH v3 0/5] Inspection code in C
The first three patches were posted previously: https://www.redhat.com/archives/libguestfs/2010-July/msg00082.html The last two patches in this series change guestfish -i to use this new code. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming blog: http://rwmj.wordpress.com Fedora now supports 80 OCaml packages (the OPEN alternative to F#) http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora
-- 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
On 02/08/10 18:12, Richard W.M. Jones wrote:
From 647be56b2d8b171a33608b36e0b7557d94db3c96 Mon Sep 17 00:00:00 2001 From: Richard Jones<rjones@redhat.com> Date: Wed, 28 Jul 2010 15:38:57 +0100 Subject: [PATCH 1/5] New API: file-architecture
This change simply converts the existing Perl-only function file_architecture into a core API call. The core API call is written in C and available in all languages and from guestfish. --- README | 2 + configure.ac | 9 ++ perl/lib/Sys/Guestfs/Lib.pm | 147 +----------------------- perl/t/510-lib-file-arch.t | 70 ----------- po/POTFILES.in | 1 + src/Makefile.am | 3 +- src/generator.ml | 128 +++++++++++++++++++++ src/inspect.c | 266 +++++++++++++++++++++++++++++++++++++++++++ 8 files changed, 411 insertions(+), 215 deletions(-) delete mode 100644 perl/t/510-lib-file-arch.t create mode 100644 src/inspect.c
diff --git a/src/inspect.c b/src/inspect.c new file mode 100644 index 0000000..8e28d8a --- /dev/null +++ b/src/inspect.c @@ -0,0 +1,266 @@ +/* libguestfs + * Copyright (C) 2010 Red Hat Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#include<config.h> + +#include<stdio.h> +#include<stdlib.h> +#include<stdint.h> +#include<inttypes.h> +#include<unistd.h> +#include<string.h> +#include<sys/stat.h> + +#include<pcre.h> + +#include "ignore-value.h" + +#include "guestfs.h" +#include "guestfs-internal.h" +#include "guestfs-internal-actions.h" +#include "guestfs_protocol.h" + +/* Compile all the regular expressions once when the shared library is + * loaded. PCRE is thread safe so we're supposedly OK here if + * multiple threads call into the libguestfs API functions below + * simultaneously. + */ +static pcre *re_file_elf; +static pcre *re_file_win64; +static pcre *re_elf_ppc64; + +static void compile_regexps (void) __attribute__((constructor)); +static void +compile_regexps (void) +{ + const char *err; + int offset; + +#define COMPILE(re,pattern,options) \ + do { \ + re = pcre_compile ((pattern), (options),&err,&offset, NULL); \ + if (re == NULL) { \ + ignore_value (write (2, err, strlen (err))); \ + abort (); \ + } \ + } while (0) + + COMPILE (re_file_elf, + "ELF.*(?:executable|shared object|relocatable), (.+?),", 0); + COMPILE (re_file_win64, "PE32\\+ executable", 0); + COMPILE (re_elf_ppc64, "64.*PowerPC", 0); +} + +/* Match a regular expression which contains no captures. Returns + * true if it matches or false if it doesn't. + */ +static int +match (const char *str, const pcre *re) +{ + size_t len = strlen (str); + int vec[30], r; + + r = pcre_exec (re, NULL, str, len, 0, 0, vec, 30);
You can ditch vec here and pass in NULL, 0. None of the above REs use back references, so this shouldn't be an issue. Aside: using a portion of a passed-in vector for slack space under certain circumstances is just hideous!
+ if (r == PCRE_ERROR_NOMATCH) + return 0; + if (r != 1) { + /* Internal error -- should not happen. */ + fprintf (stderr, "libguestfs: %s: %s: internal error: pcre_exec returned unexpected error code %d when matching against the string \"%s\"\n", + __FILE__, __func__, r, str); + return 0; + } + + return 1; +} + +/* Match a regular expression which contains exactly one capture. If + * the string matches, return the capture, otherwise return NULL. The + * caller must free the result. + */ +static char * +match1 (const char *str, const pcre *re) +{ + size_t len = strlen (str); + int vec[30], r; + + r = pcre_exec (re, NULL, str, len, 0, 0, vec, 30);
vec here could be of size 9 (need 0, 1, 2, 3; round up to multiple of 3 == 6; * 1.5 = 9). Whatever you set it to, though, please don't hard-code the size twice. Please use sizeof(vec)/sizeof(vec[0]) in the function call.
+ if (r == PCRE_ERROR_NOMATCH) + return NULL; + if (r != 2) { + /* Internal error -- should not happen. */ + fprintf (stderr, "libguestfs: %s: %s: internal error: pcre_exec returned unexpected error code %d when matching against the string \"%s\"\n", + __FILE__, __func__, r, str); + return NULL; + } + + return strndup (&str[vec[2]], vec[3]-vec[2]);
Is there a safe_ version of that? If not, you probably ought to check for NULL return for consistency.
+} + +/* 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) +{ + const char *r; + + if (strstr (elf_arch, "Intel 80386")) + r = "i386"; + else if (strstr (elf_arch, "Intel 80486")) + r = "i486"; + else if (strstr (elf_arch, "x86-64")) + r = "x86_64"; + else if (strstr (elf_arch, "AMD x86-64")) + r = "x86_64"; + else if (strstr (elf_arch, "SPARC32")) + r = "sparc"; + else if (strstr (elf_arch, "SPARC V9")) + r = "sparc64"; + else if (strstr (elf_arch, "IA-64")) + r = "ia64"; + else if (match (elf_arch, re_elf_ppc64)) + r = "ppc64"; + else if (strstr (elf_arch, "PowerPC")) + r = "ppc"; + else + r = elf_arch; + + char *ret = safe_strdup (g, r); + return ret; +} + +static int +is_regular_file (const char *filename) +{ + struct stat statbuf; + + return lstat (filename,&statbuf) == 0&& S_ISREG (statbuf.st_mode); +} + +/* Download and uncompress the cpio file to find binaries within. */ +#define INITRD_BINARIES1 "bin/ls bin/rm bin/modprobe sbin/modprobe bin/sh bin/bash bin/dash bin/nash" +#define INITRD_BINARIES2 {"bin/ls", "bin/rm", "bin/modprobe", "sbin/modprobe", "bin/sh", "bin/bash", "bin/dash", "bin/nash"}
Eww.
+ +static char * +cpio_arch (guestfs_h *g, const char *file, const char *path) +{
This function is really the architecture of an initrd. It should probably be called initrd_arch.
+ char *ret = NULL; + + const char *method; + if (strstr (file, "gzip")) + method = "zcat"; + else if (strstr (file, "bzip2")) + method = "bzcat"; + else + method = "cat"; + + char dir[] = "/tmp/initrd.XXXXXX"; +#define dir_len 18
This should be strlen(dir). The compiler will recognise this as a constant.
+ if (mkdtemp (dir) == NULL) { + perrorf (g, "mkdtemp"); + goto out; + } + + char dir_initrd[dir_len + 16]; + snprintf (dir_initrd, dir_len + 16, "%s/initrd", dir); + if (guestfs_download (g, path, dir_initrd) == -1) + goto out; + + char cmd[dir_len + 256]; + snprintf (cmd, dir_len + 256, + "cd %s&& %s initrd | cpio --quiet -id " INITRD_BINARIES1, + dir, method); + int r = system (cmd); + if (r == -1 || WEXITSTATUS (r) != 0) { + perrorf (g, "cpio command failed"); + goto out; + } + + char bin[dir_len + 32];
You've placed a limit here of 31 bytes on the contents of any member of INITRD_BINARIES2, which is defined above. You need a comment next to the definition of INITRD_BINARIES2 about this. Or remove the limit...
+ const char *bins[] = INITRD_BINARIES2; + size_t i; + for (i = 0; i< sizeof bins / sizeof bins[0]; ++i) { + snprintf (bin, dir_len + 32, "%s/%s", dir, bins[i]);
Why not just use asprintf, or whatever the equivalent is in gnulib?
+ + if (is_regular_file (bin)) { + snprintf (cmd, dir_len + 256, "file %s", bin);
Reusing a magic number from above in a different context. Magic numbers are getting a bit hard to track at this point. Could do with cmd_len for the size of cmd. Stepping back slightly, you could more cleanly use libmagic here instead of invoking file. It would be less code, and wouldn't encode length limits which aren't defined anywhere.
+ FILE *fp = popen (cmd, "r"); + if (!fp) { + perrorf (g, "popen: %s", cmd); + goto out; + } + + char line[1024]; + if (!fgets (line, sizeof line, fp)) { + perrorf (g, "fgets"); + fclose (fp); + goto out; + } + pclose (fp);
^^^ This chunk could go.
+ + char *elf_arch; + if ((elf_arch = match1 (line, re_file_elf)) != NULL) { + ret = canonical_elf_arch (g, elf_arch); + free (elf_arch); + goto out; + } + } + } + error (g, "file_architecture: could not determine architecture of cpio archive"); + + out: + /* Free up the temporary directory. Note the directory name cannot + * contain shell meta-characters because of the way it was + * constructed above. + */ + snprintf (cmd, dir_len + 256, "rm -rf %s", dir);
cmd_len Is there really no widely available C implementation of this?
+ ignore_value (system (cmd)); + + return ret; +#undef dir_len +} + +char * +guestfs__file_architecture (guestfs_h *g, const char *path) +{ + char *file = NULL; + char *elf_arch = NULL; + char *ret = NULL; + + /* Get the output of the "file" command. Note that because this + * runs in the daemon, LANG=C so it's in English. + */ + file = guestfs_file (g, path); + if (file == NULL) + return NULL; + + if ((elf_arch = match1 (file, re_file_elf)) != NULL) + ret = canonical_elf_arch (g, elf_arch); + else if (strstr (file, "PE32 executable")) + ret = safe_strdup (g, "i386"); + else if (match (file, re_file_win64))
Why not strstr(file, "PE32+ executable") ?
+ ret = safe_strdup (g, "x86_64"); + else if (strstr (file, "cpio archive")) + ret = cpio_arch (g, file, path); + else + error (g, "file_architecture: unknown architecture: %s", path); + + free (file); + free (elf_arch); + return ret; /* caller frees */ +} -- 1.7.1
API is fine. I think we need to think hard, though, about how to do safe string manipulation in libguestfs. In the above code, it's hard to be sure that you're not leaking or overflowing anything. Perhaps using a string library would be in order. Matt -- Matthew Booth, RHCA, RHCSS Red Hat Engineering, Virtualisation Team GPG ID: D33C3490 GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
On Mon, Aug 09, 2010 at 05:08:06PM +0100, Matthew Booth wrote:
On 02/08/10 18:12, Richard W.M. Jones wrote:
From 647be56b2d8b171a33608b36e0b7557d94db3c96 Mon Sep 17 00:00:00 2001 From: Richard Jones<rjones@redhat.com> Date: Wed, 28 Jul 2010 15:38:57 +0100 Subject: [PATCH 1/5] New API: file-architecture
This change simply converts the existing Perl-only function file_architecture into a core API call. The core API call is written in C and available in all languages and from guestfish. --- README | 2 + configure.ac | 9 ++ perl/lib/Sys/Guestfs/Lib.pm | 147 +----------------------- perl/t/510-lib-file-arch.t | 70 ----------- po/POTFILES.in | 1 + src/Makefile.am | 3 +- src/generator.ml | 128 +++++++++++++++++++++ src/inspect.c | 266 +++++++++++++++++++++++++++++++++++++++++++ 8 files changed, 411 insertions(+), 215 deletions(-) delete mode 100644 perl/t/510-lib-file-arch.t create mode 100644 src/inspect.c
diff --git a/src/inspect.c b/src/inspect.c new file mode 100644 index 0000000..8e28d8a --- /dev/null +++ b/src/inspect.c @@ -0,0 +1,266 @@ +/* libguestfs + * Copyright (C) 2010 Red Hat Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#include<config.h> + +#include<stdio.h> +#include<stdlib.h> +#include<stdint.h> +#include<inttypes.h> +#include<unistd.h> +#include<string.h> +#include<sys/stat.h> + +#include<pcre.h> + +#include "ignore-value.h" + +#include "guestfs.h" +#include "guestfs-internal.h" +#include "guestfs-internal-actions.h" +#include "guestfs_protocol.h" + +/* Compile all the regular expressions once when the shared library is + * loaded. PCRE is thread safe so we're supposedly OK here if + * multiple threads call into the libguestfs API functions below + * simultaneously. + */ +static pcre *re_file_elf; +static pcre *re_file_win64; +static pcre *re_elf_ppc64; + +static void compile_regexps (void) __attribute__((constructor)); +static void +compile_regexps (void) +{ + const char *err; + int offset; + +#define COMPILE(re,pattern,options) \ + do { \ + re = pcre_compile ((pattern), (options),&err,&offset, NULL); \ + if (re == NULL) { \ + ignore_value (write (2, err, strlen (err))); \ + abort (); \ + } \ + } while (0) + + COMPILE (re_file_elf, + "ELF.*(?:executable|shared object|relocatable), (.+?),", 0); + COMPILE (re_file_win64, "PE32\\+ executable", 0); + COMPILE (re_elf_ppc64, "64.*PowerPC", 0); +} + +/* Match a regular expression which contains no captures. Returns + * true if it matches or false if it doesn't. + */ +static int +match (const char *str, const pcre *re) +{ + size_t len = strlen (str); + int vec[30], r; + + r = pcre_exec (re, NULL, str, len, 0, 0, vec, 30);
You can ditch vec here and pass in NULL, 0. None of the above REs use back references, so this shouldn't be an issue.
My reading of the pcre_exec docs suggested otherwise. However I will need to try it when I get back from the conference.
Aside: using a portion of a passed-in vector for slack space under certain circumstances is just hideous!
+ if (r == PCRE_ERROR_NOMATCH) + return 0; + if (r != 1) { + /* Internal error -- should not happen. */ + fprintf (stderr, "libguestfs: %s: %s: internal error: pcre_exec returned unexpected error code %d when matching against the string \"%s\"\n", + __FILE__, __func__, r, str); + return 0; + } + + return 1; +} + +/* Match a regular expression which contains exactly one capture. If + * the string matches, return the capture, otherwise return NULL. The + * caller must free the result. + */ +static char * +match1 (const char *str, const pcre *re) +{ + size_t len = strlen (str); + int vec[30], r; + + r = pcre_exec (re, NULL, str, len, 0, 0, vec, 30);
vec here could be of size 9 (need 0, 1, 2, 3; round up to multiple of 3 == 6; * 1.5 = 9). Whatever you set it to, though, please don't hard-code the size twice. Please use sizeof(vec)/sizeof(vec[0]) in the function call.
Yes, good idea.
+ if (r == PCRE_ERROR_NOMATCH) + return NULL; + if (r != 2) { + /* Internal error -- should not happen. */ + fprintf (stderr, "libguestfs: %s: %s: internal error: pcre_exec returned unexpected error code %d when matching against the string \"%s\"\n", + __FILE__, __func__, r, str); + return NULL; + } + + return strndup (&str[vec[2]], vec[3]-vec[2]);
Is there a safe_ version of that? If not, you probably ought to check for NULL return for consistency.
Urr yes you are right, I thought I'd caught all instances of this ...
+} + +/* 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) +{ + const char *r; + + if (strstr (elf_arch, "Intel 80386")) + r = "i386"; + else if (strstr (elf_arch, "Intel 80486")) + r = "i486"; + else if (strstr (elf_arch, "x86-64")) + r = "x86_64"; + else if (strstr (elf_arch, "AMD x86-64")) + r = "x86_64"; + else if (strstr (elf_arch, "SPARC32")) + r = "sparc"; + else if (strstr (elf_arch, "SPARC V9")) + r = "sparc64"; + else if (strstr (elf_arch, "IA-64")) + r = "ia64"; + else if (match (elf_arch, re_elf_ppc64)) + r = "ppc64"; + else if (strstr (elf_arch, "PowerPC")) + r = "ppc"; + else + r = elf_arch; + + char *ret = safe_strdup (g, r); + return ret; +} + +static int +is_regular_file (const char *filename) +{ + struct stat statbuf; + + return lstat (filename,&statbuf) == 0&& S_ISREG (statbuf.st_mode); +} + +/* Download and uncompress the cpio file to find binaries within. */ +#define INITRD_BINARIES1 "bin/ls bin/rm bin/modprobe sbin/modprobe bin/sh bin/bash bin/dash bin/nash" +#define INITRD_BINARIES2 {"bin/ls", "bin/rm", "bin/modprobe", "sbin/modprobe", "bin/sh", "bin/bash", "bin/dash", "bin/nash"}
Eww.
Right, but it does make sense in context of the later code ... If C had LISP-like macros ...
+ +static char * +cpio_arch (guestfs_h *g, const char *file, const char *path) +{
This function is really the architecture of an initrd. It should probably be called initrd_arch.
Yup, although we can only handle cpio-format (see documentation). Very old distros (RHEL 3 and earlier IIRC) used a compressed ext2 disk image, and we cannot handle that at the moment.
+ char *ret = NULL; + + const char *method; + if (strstr (file, "gzip")) + method = "zcat"; + else if (strstr (file, "bzip2")) + method = "bzcat"; + else + method = "cat"; + + char dir[] = "/tmp/initrd.XXXXXX"; +#define dir_len 18
This should be strlen(dir). The compiler will recognise this as a constant.
OK.
+ if (mkdtemp (dir) == NULL) { + perrorf (g, "mkdtemp"); + goto out; + } + + char dir_initrd[dir_len + 16]; + snprintf (dir_initrd, dir_len + 16, "%s/initrd", dir); + if (guestfs_download (g, path, dir_initrd) == -1) + goto out; + + char cmd[dir_len + 256]; + snprintf (cmd, dir_len + 256, + "cd %s&& %s initrd | cpio --quiet -id " INITRD_BINARIES1, + dir, method); + int r = system (cmd); + if (r == -1 || WEXITSTATUS (r) != 0) { + perrorf (g, "cpio command failed"); + goto out; + } + + char bin[dir_len + 32];
You've placed a limit here of 31 bytes on the contents of any member of INITRD_BINARIES2, which is defined above. You need a comment next to the definition of INITRD_BINARIES2 about this. Or remove the limit...
Yup.
+ const char *bins[] = INITRD_BINARIES2; + size_t i; + for (i = 0; i< sizeof bins / sizeof bins[0]; ++i) { + snprintf (bin, dir_len + 32, "%s/%s", dir, bins[i]);
Why not just use asprintf, or whatever the equivalent is in gnulib?
Because we'd have to free it up. I think there is an alloca-variant in gnulib, but I think the static array should be OK if we document it.
+ + if (is_regular_file (bin)) { + snprintf (cmd, dir_len + 256, "file %s", bin);
Reusing a magic number from above in a different context. Magic numbers are getting a bit hard to track at this point. Could do with cmd_len for the size of cmd.
Stepping back slightly, you could more cleanly use libmagic here instead of invoking file. It would be less code, and wouldn't encode length limits which aren't defined anywhere.
Yes, that would make a lot of sense.
+ FILE *fp = popen (cmd, "r"); + if (!fp) { + perrorf (g, "popen: %s", cmd); + goto out; + } + + char line[1024]; + if (!fgets (line, sizeof line, fp)) { + perrorf (g, "fgets"); + fclose (fp); + goto out; + } + pclose (fp);
^^^ This chunk could go.
+ + char *elf_arch; + if ((elf_arch = match1 (line, re_file_elf)) != NULL) { + ret = canonical_elf_arch (g, elf_arch); + free (elf_arch); + goto out; + } + } + } + error (g, "file_architecture: could not determine architecture of cpio archive"); + + out: + /* Free up the temporary directory. Note the directory name cannot + * contain shell meta-characters because of the way it was + * constructed above. + */ + snprintf (cmd, dir_len + 256, "rm -rf %s", dir);
cmd_len
Is there really no widely available C implementation of this?
Not that I'm aware of ...?
+ ignore_value (system (cmd)); + + return ret; +#undef dir_len +} + +char * +guestfs__file_architecture (guestfs_h *g, const char *path) +{ + char *file = NULL; + char *elf_arch = NULL; + char *ret = NULL; + + /* Get the output of the "file" command. Note that because this + * runs in the daemon, LANG=C so it's in English. + */ + file = guestfs_file (g, path); + if (file == NULL) + return NULL; + + if ((elf_arch = match1 (file, re_file_elf)) != NULL) + ret = canonical_elf_arch (g, elf_arch); + else if (strstr (file, "PE32 executable")) + ret = safe_strdup (g, "i386"); + else if (match (file, re_file_win64))
Why not strstr(file, "PE32+ executable") ?
Yes, that's a mistake.
+ ret = safe_strdup (g, "x86_64"); + else if (strstr (file, "cpio archive")) + ret = cpio_arch (g, file, path); + else + error (g, "file_architecture: unknown architecture: %s", path); + + free (file); + free (elf_arch); + return ret; /* caller frees */ +} -- 1.7.1
API is fine. I think we need to think hard, though, about how to do safe string manipulation in libguestfs. In the above code, it's hard to be sure that you're not leaking or overflowing anything. Perhaps using a string library would be in order.
Use OCaml ... 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
On Tue, Aug 10, 2010 at 01:41:21PM +0100, Richard W.M. Jones wrote:
+ r = pcre_exec (re, NULL, str, len, 0, 0, vec, 30);
You can ditch vec here and pass in NULL, 0. None of the above REs use back references, so this shouldn't be an issue.
My reading of the pcre_exec docs suggested otherwise. However I will need to try it when I get back from the conference.
The docs say I can pass ovector as NULL, but in that case pcre_exec might have to malloc extra space (although only in the case where the regexp contains back-references -- ours don't *yet* but someone might add one in future). I think it's safer to just leave this. However I've changed all calls to use sizeof(vec)/sizeof(vec[0]) instead of the fixed integer.
+ char *ret = NULL; + + const char *method; + if (strstr (file, "gzip")) + method = "zcat"; + else if (strstr (file, "bzip2")) + method = "bzcat"; + else + method = "cat"; + + char dir[] = "/tmp/initrd.XXXXXX"; +#define dir_len 18
This should be strlen(dir). The compiler will recognise this as a constant.
OK.
Actually I used 'sizeof dir' instead here.
+ + if (is_regular_file (bin)) { + snprintf (cmd, dir_len + 256, "file %s", bin);
Reusing a magic number from above in a different context. Magic numbers are getting a bit hard to track at this point. Could do with cmd_len for the size of cmd.
Stepping back slightly, you could more cleanly use libmagic here instead of invoking file. It would be less code, and wouldn't encode length limits which aren't defined anywhere.
Yes, that would make a lot of sense.
The code isn't shorter, but I changed it to use libmagic anyway. I'm going to post the whole series again, since it got longer ... Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://et.redhat.com/~rjones/libguestfs/ See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html
-- 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 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
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://et.redhat.com/~rjones/libguestfs/ See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://et.redhat.com/~rjones/libguestfs/ See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html
participants (2)
-
Matthew Booth -
Richard W.M. Jones