[Libguestfs] [PATCH v3 1/5] New API: file-architecture

[Richard W.M. Jones]

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