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

[Markus Armbruster]

Eric Blake <address@hidden> 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 <address@hidden>
>> ---
>>  qemu-config.c   |   15 +++++++++++++--
>>  qemu-options.hx |    3 ++-
>>  2 files changed, 15 insertions(+), 3 deletions(-)
>
> Reviewed-by: Eric Blake <address@hidden>
>
>> 
>> 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.