Re: [Qemu-devel] [PATCH v4 2/2] qga: Add guest-get-fsinfo command

[Tomoki Sekiyama <tomoki.sekiyama@hds.com>]

On 6/20/14 11:21 , "Eric Blake" <eblake@redhat.com> wrote:

>On 06/19/2014 06:34 PM, Tomoki Sekiyama wrote:
>
>>>> +    }
>>>> +    if (S_ISBLK(st.st_mode)) {
>>>> +        *devmajor = major(st.st_rdev);
>>>> +        *devminor = minor(st.st_rdev);
>>>
>>> major() and minor() are not POSIX functions.  While they work on Linux,
>>> and appear to have BSD origins, I still wonder if you need to be more
>>> careful on guarding their use.
>> 
>> This function is guarded by '#if defined(__linux__)' (and also
>> '#if defined(CONFIG_FSFREEZE) || defined(CONFIG_FSTRIM)' ),
>> so I believe it is ok here.
>
>I take it the function gracefully fails on non-Linux.  But that's not
>very nice to management functions - they learn that the function exists,
>but have to call it to see if it actually works.  It might be nicer to
>conditionally expose the command only if it will work, so that querying
>the list of commands makes it obvious whether the agent supports subset
>freezing.

I think hiding unsupported commands from 'guest-info' is reasonable.
Currently the list is auto-generated from qapi-schema.json and all
commands are listed even though some of them just returns QERR_
UNSUPPORTED on some platforms(like guest-fstrim and guest-network-
get-interfaces etc. on non-Linux platform).
We may need a new mechanism to unregister them from the list,
or to mark them "unsupported", or to extend the generator so that
we can omit them from the list on unsupported platforms.
(And that should be done in another patch series.)

>>>> +    while (getline(&line, &n, fp) != -1) {
>>>> +        ret = sscanf(line,
>>>> +                     "%*u %*u %u:%u %*s %n%*s%n %*s %*s %*s %n%*s%n
>>>> %n%*s%n%c",
>>>> +                     &devmajor, &devminor, &dir_s, &dir_e, &type_s,
>>>> &type_e,
>>>
>>> I'm not a huge fan of sscanf("%u") - it has undefined behavior on
>>> integer overflow.  But the alternative of avoiding sscanf and
>>> open-coding your parser is so much bulkier, that I tend to look the
>>> other way.
>> 
>> Hmm, '%*u' can be simply replaced by '%*s' then.
>> For '%u:%u', maybe we can catch this part with '%s' or '%n%*s%n'
>> and convert them into integers later by strtoul().
>> Does it sound good to you?
>
>As I said, the cost of properly parsing a row of integers explodes into
>so much more code that I'm just fine looking the other way if you want
>to use sscanf for compactness - after all, this is a kernel file we're
>supposed to be reading, and if that interface has been corrupted to give
>us bogus data that doesn't parse correctly, then we're probably already
>hurting in other ways.

Right... then I'm going to keep sscanf here then.

Thanks,
Tomoki Sekiyama