[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libguestfs] [PATCH] Replace shell_quote function with %Q and %R printf specifiers.



On 27/07/09 22:42, Richard W.M. Jones wrote:
At the moment the daemon code contains an incredibly hairy function
called shell_quote for safely quoting strings passed to the shell.

The patch replaces that with a glibc custom printf format (actually
two, but very closely related), %Q and %R.

%Q is like %s but it safely shell quotes the string.

%R is like %Q but it prefixes the path with /sysroot.


Nice.

There's a danger here, though, of increasing the barrier to understandability. I can imagine looking at a printf containing %Q or %R and wasting a huge amount of time on google fruitlessly trying to work out what on earth it's on about. With that in mind I'd definitely add something to HACKING. If it weren't for the nowarn wrapper happening to provide a weirdness hook, I might also put add a comment above every printf which uses it. I would, however, add a much bigger comment to asprintf_nowarn, pointing to the implementations.

I also don't think %R is what this feature is about. It seems too trivial for a fairly off-the-wall trick. You can just as easily write:

#define SYSROOT "/sysroot"

snprintf (cmd, sizeof cmd, "cat " SYSROOT "%s", path);

It's only slightly longer, and a whole lot more readable to the vast majority of C programmers.

Lastly, avoid using inline. The compiler will inline a function if it makes sense.

Matt
--
Matthew Booth, RHCA, RHCSS
Red Hat Engineering, Virtualisation Team

M:       +44 (0)7977 267231
GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]