6:42 a.m.
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