READCONFIG: Allow reading the configuration from a pre-existing filedescriptor

Update the readconfig filename parsing to allow specifying an existing, inherited, filedescriptor as 'fd:<n>'
This is useful when you want to pass potentially sensitive onfiguration data to qemu without having it hit the filesystem/stable-storage

Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
---
 qemu-config.c   |   15 +++++++++++++--
 qemu-options.hx |    3 ++-
 2 files changed, 15 insertions(+), 3 deletions(-)

On 01/25/2012 03:23 PM, Ronnie Sahlberg wrote:
> Update the readconfig filename parsing to allow specifying an existing, inherited, filedescriptor as 'fd:<n>'
> This is useful when you want to pass potentially sensitive onfiguration data to qemu without having it hit the filesystem/stable-storage
> 
> Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> ---
>  qemu-config.c   |   15 +++++++++++++--
>  qemu-options.hx |    3 ++-
>  2 files changed, 15 insertions(+), 3 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> 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);

This means

-readconfig fd:4junk

will read fd 4.  I don't know if there is a policy on ignoring vs.
rejecting ill-formed command line, which is why I'm okay with the patch
as-is if it meets project policy; but you might want to consider passing
a non-NULL pointer for the second argument and rejecting an empty string
or trailing junk.

> +        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.]

Eric Blake <eblake@redhat.com> writes:

> On 01/25/2012 03:23 PM, Ronnie Sahlberg wrote:
>> Update the readconfig filename parsing to allow specifying an existing, inherited, filedescriptor as 'fd:<n>'
>> This is useful when you want to pass potentially sensitive onfiguration data to qemu without having it hit the filesystem/stable-storage
>> 
>> Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>> ---
>>  qemu-config.c   |   15 +++++++++++++--
>>  qemu-options.hx |    3 ++-
>>  2 files changed, 15 insertions(+), 3 deletions(-)
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
>> 
>> 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);

The user needs to escape filenames starting with fd by prefixing "./".

Makes -readconfig yet another special case.  Well, one more won't kill
us.

Still, it would be nice to factor out the common code, so that other
instances can share it (and thus use the same syntax).

For full generality, make it a function taking (const char
*filename_or_fd, int flags) and returning the open file descriptor.  If
filename_or_fd refers to a file descriptor, it must be open and
compatible with flags.

Cleaner than this ad hoc overloading of the filename argument would be:

* Separate option for fd: -readconfig-fd FD

* Use the general syntax for NAME=VALUE arguments: -readconfig
  path=FNAME -readonfig fd=FD

  With QemuOpts, can make "path" the implied_opt_name so that
  -readconfig FNAME still works.  Need to escape ',' and '=' in
  filenames, but that's common in qemu command syntax.

> This means
>
> -readconfig fd:4junk
>
> will read fd 4.  I don't know if there is a policy on ignoring vs.
> rejecting ill-formed command line, which is why I'm okay with the patch
> as-is if it meets project policy; but you might want to consider passing
> a non-NULL pointer for the second argument and rejecting an empty string
> or trailing junk.
>
>> +        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.

On Thu, Jan 26, 2012 at 08:40:02AM +0100, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> 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

On Thu, Jan 26, 2012 at 08:40:02AM +0100, Markus Armbruster wrote:
> Cleaner than this ad hoc overloading of the filename argument would be:
> 
> * Separate option for fd: -readconfig-fd FD
> 
> * Use the general syntax for NAME=VALUE arguments: -readconfig
>   path=FNAME -readonfig fd=FD
> 
>   With QemuOpts, can make "path" the implied_opt_name so that
>   -readconfig FNAME still works.  Need to escape ',' and '=' in
>   filenames, but that's common in qemu command syntax.

I really think this option is better and cleaner. There's no reason to
invent a new mini-language for argument syntax if we already have one.