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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2119 bytes --]

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.


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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]