[Libguestfs] [PATCH FOR DISCUSSION ONLY] Rewrite libguestfs-supermin-helper in C.

[Richard W.M. Jones]

On Fri, Mar 12, 2010 at 12:06:24PM +0100, Jim Meyering wrote:
> This looks nice.
> Mostly nit-picky robustness and style-related suggestions below:

Thanks for the "nit-picky" review :-)

> IMHO, it's very often better to use asprintf.
> Two reasons:
>   - eliminates wasteful (and not-so-portable) [PATH_MAX] use.
>   - eliminates risk of misbehavior if/when some combination of inputs
>       leads to a truncated buffer.

Yup, much better.  I changed all these snprintfs, strdups, mallocs,
and reallocs, to use the gnulib x-* versions as you noted.

> > +static void
> > +add_string (char ***argv, size_t *size, size_t *alloc, const char *str)
> > +{
> 
> For a variable that counts items as this "size" does,
> I'd prefer to use a name like n_used, n_items or even just "n",
> since "size" tends to evoke "buffer size", which might be misleading here.
>
> Similar, s/alloc/n_alloc/ would make me slightly happier ;-)

Ok, replaced all those with n_used / n_alloc as suggested.

> For extra credit, see xalloc.h's comment with sample use of x2nrealloc.
> Using that would let you remove the += 64 code above,
> and make this function even more concise.

Yup, much improved.

> > +  struct dirent *d;
> > +  for (;;) {
> > +    errno = 0;
> > +    d = readdir (dir);
> 
> It'd be nice to move the declaration of "d" down into the loop.
> That'd save a line and ensure that there is no accidental use
> outside of the scope where it's intended to be used.
> 
>      for (;;) {
>        errno = 0;
>        struct dirent *d = readdir (dir);

Ok, done.

> > +    if (d == NULL) {
> > +      if (errno != 0) {
> > +        /* But if it fails here, after opening and potentially reading
> > +         * part of the directory, that's a proper failure - inform the
> > +         * user and exit.
> > +         */
> > +        perror (name);
> > +        exit (EXIT_FAILURE);
> 
> Personally, I have a bias for using the "error" function to report diagnostics.
> Then, they always start with "program_name: ", and that makes it easier
> to attribute a diagnostic to its source.
> 
> Another advantage is readability: it combines the common, 2-line fprintf;exit
> idiom into a single statement, so you often end up being able to avoid
> the curly braces, too.

OK, I replaced every use of perror with error.

> The string, "libguestfs-supermin-helper" appears enough that I'd
> factor it out.  But if you go with using the "error" function,
> you'll get that for free.

Also used error here.

> > +/* Copy contents of buffer to out_fd and keep out_offset correct. */
> > +static void
> > +copy_to_fd (const void *buffer, size_t len)
> 
> You might prefer write_to_fd, since "write" is usually what you
> do when sending the contents of a buffer to an output stream/device.

Agreed, done.

> > +{
> > +  if (full_write (out_fd, buffer, len) != len) {
> > +    perror ("libguestfs-supermin-helper: write");
> > +    exit (EXIT_FAILURE);
> > +  }
> > +  out_offset += len;
> > +}
> > +
> > +/* Copy contents of file to out_fd. */
> > +static void
> > +copy_file_to_fd (const char *filename)
> > +{
> > +  char buffer[BUFFER_SIZE];
> > +  int fd2;
> > +  ssize_t r;
> 
> You know my bias is to move declarations, like the above two,
> "down" to first use, whenever possible.

OK I moved these down a bit.

> > +      fprintf (stderr, "libguestfs-supermin-helper: copy_file_len_to_fd: %s: file has increased in size\n", filename);
> 
> long lines

Mostly fixed with the error() change.

> > +    else if (S_ISLNK (statbuf->st_mode)) {
> > +      char tmp[PATH_MAX];
> 
> Why bother reading the symlink dest. name?
> It might well be yet another symlink.

Not quite sure what you meant here -- I need to read the symlink
in order to write it out to the cpio file.

> > +    /* /lib/modules/<version> */
> > +    char *modpath = malloc (strlen (MODULESDIR) + strlen (version) + 2);
> > +    if (modpath == NULL) {
> > +      perror ("malloc");
> > +      exit (1);
> > +    }
> > +    sprintf (modpath, MODULESDIR "/%s", version);
> 
> This is the classic case in which using asprintf is better.
> Or actually, better still, xasprintf, since you want to exit upon failure.
> You can replace the above with this one line:
>
>     char *modpath = xasprintf (MODULESDIR "/%s", version);

Done.

> > +static void
> > +print_timestamped_message (const char *fs, ...)
> > +{
> > +  struct timeval tv;
> > +  gettimeofday (&tv, NULL);
> 
> Technically, gettimeofday *may* fail.  Solaris documents it:

Hmmm ... it's only informational anyway.

I'll post a new patch in a minute when I've tested it a bit.

Thanks,

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