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

[Eric Blake <eblake@redhat.com>]

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

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.

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

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

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

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

> +                     &dev_s, &dev_e, &check);
> +        if (ret < 3) {
> +            continue;
> +        }
> +        line[dir_e] = 0;
> +        line[type_e] = 0;
> +        line[dev_e] = 0;
> +        decode_mntname(line+dir_s, dir_e-dir_s);
> +        decode_mntname(line+dev_s, dev_e-dev_s);

Space around binary '+' and '-'

> +        if (devmajor == 0) {
> +            /* btrfs reports major number = 0 */
> +            if (strcmp("btrfs", line+type_s) != 0 ||
> +                dev_major_minor(line+dev_s, &devmajor, &devminor) < 0) {
> +                continue;
> +            }
> +        }
> +
> +        mount = g_malloc0(sizeof(FsMount));
> +        mount->dirname = g_strdup(line+dir_s);
> +        mount->devtype = g_strdup(line+type_s);

Space around '+'

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

> +
> +    driver = get_pci_driver(syspath, (p+12+pcilen)-syspath, errp);

Spaces around operators (I'll quit pointing it out).

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


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