Re: [Qemu-devel] [PATCH] READCONFIG: Allow reading the configuration from a pre-existing filedescriptor

[Daniel P. Berrange]

On Thu, Jan 26, 2012 at 08:40:02AM +0100, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> > On 01/25/2012 03:23 PM, Ronnie Sahlberg wrote:
> >> diff --git a/qemu-config.c b/qemu-config.c
> >> index b030205..c12c5eb 100644
> >> --- a/qemu-config.c
> >> +++ b/qemu-config.c
> >> @@ -770,8 +770,19 @@ out:
> >>  
> >>  int qemu_read_config_file(const char *filename)
> >>  {
> >> -    FILE *f = fopen(filename, "r");
> >> -    int ret;
> >> +    FILE *f;
> >> +    int ret, fd;
> >> +
> >> +    if (strncmp(filename, "fd:", 3)) {
> >> +        f = fopen(filename, "r");
> >> +    } else {
> >> +        errno = 0;
> >> +        fd = strtol(filename + 3, NULL, 10);


> >> +        if (errno != 0) {
> >> +            return -errno;
> >> +        }
> >
> > POSIX says that strtol("", NULL, 10) may return 0 without setting errno
> > (that is, you can't rely on EINVAL in that case).  That's another
> > argument for _always_ passing a non-NULL pointer and to see if you
> > accidentally parsed an empty string, since you don't want to have
> > another FILE* competing with stdin.  [Libvirt forbids the direct use of
> > strtol and friends, and instead provides wrapper functions that take
> > care of the sanity checking that is not mandated by POSIX; it may be
> > worth introducing a qemu_strtol that does likewise, but that's a
> > different cleanup project with wider scope.]
> 
> strtol() should be used like this:
> 
>     errno = 0;
>     val = strtol(str, &end, base);
>     if (end == str || || *end != 0 || errno != 0) {
>       // conversion failed
>     }
> 
> Use "*end in follow set" instead of "*end != 0" when the number needn't
> be null terminated.
> 
> Plenty of bad examples to be found in qemu, I'm afraid.

The rules for correct strtol() usage are so bizarre that, IMHO, you are
doomed to always have plenty of bad usage in any non-trivial code. As Eric
mentions, after fighting bad usage for a while in libvirt, we eventually
banned all direct usage of strtol() in favour of a saner helper API, which
has really helped our code quality in this area. The most important aspect
of the saner API is the avoidance of overloading the return value to contain
both the error status & and parsed value. We ended up using this (and some
other variants for different sized integers):

/* Like strtol, but produce an "int" result, and check more carefully.
   Return 0 upon success;  return -1 to indicate failure.
   When END_PTR is NULL, the byte after the final valid digit must be NUL.
   Otherwise, it's like strtol and lets the caller check any suffix for
   validity.  This function is careful to return -1 when the string S
   represents a number that is not representable as an "int". */
int
virStrToLong_i(char const *s, char **end_ptr, int base, int *result)
{
    long int val;
    char *p;
    int err;

    errno = 0;
    val = strtol(s, &p, base);
    err = (errno || (!end_ptr && *p) || p == s || (int) val != val);
    if (end_ptr)
        *end_ptr = p;
    if (err)
        return -1;
    *result = val;
    return 0;
}

For extra paranoia you could also annotate the header file declaration
with __attribute__((return_check)) to force callers to always check the
error return status.  I'd heartily encourage QEMU folks to similarly
ban any use of strtol() (or the similarly badly designed glib equivalent)
in favour of saner helper APIs.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|