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

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

Hi Eric,

Thank you for the review.

On 6/19/14 17:23 , "Eric Blake" <eblake@redhat.com> wrote:

>On 06/05/2014 08:57 AM, Tomoki Sekiyama wrote:
>> Add command to get mounted filesystems information in the guest.
>> The returned value contains a list of mountpoint paths and
>> corresponding disks info such as disk bus type, drive address,
>> and the disk controllers' PCI addresses, so that management layer
>> such as libvirt can resolve the disk backends.
>> 
>> For example, when `lsblk' result is:
><snip cool example>
>
>> 
>> In Linux guest, the disk information is resolved from sysfs. So far,
>> it only supports virtio-blk, virtio-scsi, IDE, SATA, SCSI disks on x86
>> hosts, and "disk" parameter may be empty for unsupported disk types.
>> 
>> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com>
>> ---
>>  qga/commands-posix.c |  422
>>++++++++++++++++++++++++++++++++++++++++++++++++++
>>  qga/commands-win32.c |    6 +
>>  qga/qapi-schema.json |   79 +++++++++
>>  3 files changed, 506 insertions(+), 1 deletion(-)
>> 
>
>> +static int dev_major_minor(const char *devpath,
>> +                           unsigned int *devmajor, unsigned int
>>*devminor)
>> +{
>> +    struct stat st;
>> +
>> +    *devmajor = 0;
>> +    *devminor = 0;
>> +
>> +    if (stat(devpath, &st) < 0) {
>> +        slog("failed to stat device file '%s': %s", devpath,
>>strerror(errno));
>> +        return -1;
>> +    }
>> +    if (S_ISDIR(st.st_mode)) {
>> +        /* It is bind mount */
>> +        return -2;
>> +    }
>> +    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.

>> +
>> +static void decode_mntname(char *name, int len)
>> +{
>> +    int i, j = 0;
>> +    for (i = 0; i <= len; i++) {
>> +        if (name[i] != '\\') {
>> +            name[j++] = name[i];
>> +        } else if (name[i+1] == '\\') {
>> +            name[j++] = '\\';
>> +            i++;
>> +        } else if (name[i+1] == '0' && name[i+2] == '4' && name[i+3]
>>== '0') {
>
>Spaces around binary '+'

OK, I will fix these, and also other binary operators.

>> +            name[j++] = ' ';
>> +            i += 3;
>> +        } else if (name[i+1] == '0' && name[i+2] == '1' && name[i+3]
>>== '1') {
>> +            name[j++] = '\t';
>> +            i += 3;
>> +        } else if (name[i+1] == '0' && name[i+2] == '1' && name[i+3]
>>== '2') {
>> +            name[j++] = '\n';
>> +            i += 3;
>> +        } else if (name[i+1] == '1' && name[i+2] == '3' && name[i+3]
>>== '4') {
>> +            name[j++] = '\\';
>> +            i += 3;
>> +        } else {
>
>This loop looks a bit hard-coded, in that it only recognizes certain
>escapes.  Wouldn't it be more generic to handle ALL instances of \
>followed by three octal digits, even if mount names tend not to encode
>that many characters as an escape?

I will replace this with more generic that covers every three octal
digits (\000 - \377). Mount names only escape above characters, but
anyway it is harmless to make it more generic.

>> +            name[j++] = name[i];
>> +        }
>> +    }
>> +}
>> +
>> +static void build_fs_mount_list(FsMountList *mounts, Error **errp)
>> +{
>> +    FsMount *mount;
>> +    char const *mountinfo = "/proc/self/mountinfo";
>
>The file /proc/self/mountinfo is Linux-specific, but you are in the file
>commands-posix.c.  Is this function properly guarded to not cause
>compilation issues on BSD?

Again, this is inside '#if defined(__linux__)', so it won't be compiled
on BSD.

>> +    FILE *fp;
>> +    char *line = NULL;
>> +    size_t n;
>> +    char check;
>> +    unsigned int devmajor, devminor;
>> +    int ret, dir_s, dir_e, type_s, type_e, dev_s, dev_e;
>> +
>> +    fp = fopen(mountinfo, "r");
>> +    if (!fp) {
>> +        build_fs_mount_list_from_mtab(mounts, errp);
>> +        return;
>> +    }
>> +
>> +    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?

>> +                     &dev_s, &dev_e, &check);
>> +        if (ret < 3) {
>> +            continue;
>> +        }

<snip>

>>
>> +
>> +/* Store disk device info specified by @sysfs into @fs */
>> +static void __build_guest_fsinfo(char const *syspath,
>> +                                 GuestFilesystemInfo *fs, Error **errp)
>
>Naming functions with leading __ is dangerous - it risks conflicts with
>system headers.  This function is static, so you don't need to munge the
>name.

>> +static void _build_guest_fsinfo(char const *dirpath,
>> +                                GuestFilesystemInfo *fs, Error **errp)
>
>Having both __build_guest_fsinfo and _build_guest_fsinfo in the same
>file is confusing.  Can you come up with better names?

I'll rename this series of functions to:
 __build_guest_fsinfo         -> build_guest_fsinfo_for_real_device
 __build_guest_fsinfo_virtual -> build_guest_fsinfo_for_virtual_device

 _build_guest_fsinfo          -> build_guest_fsinfo_for_device

 build_guest_fsinfo           -> build_guest_fsinfo   (unchanged)


Thanks,
Tomoki Sekiyama