Skip to contentSkip to conversation

qga: Add guest-fsfreeze-freeze-list command

PATCH Superseded Tomoki Sekiyama 2 participants 9 messages Jun 5, 2014 – Jun 20, 2014

v4 of 5 Superseded Latest is v5 →

lore
Share & tools
lore
#0 lore raw

v4 cover qga: Add guest-fsfreeze-freeze-list command

Hi,

This is v4 patch for qemu-ga to add functions to freeze specific file systems mounted in a guest.

PATCH 1 adds a guest-fsfreeze-freeze-list command, which takes an additional argument to specify which filesystems to be frozen, derived from guest-fsfreeze-freeze command.

PATCH 2 adds a guest-get-fsinfo command to get a list of mounted file systems in the guest with disk devices information to resolve backend disk images for a management layer such as libvirt.

Changes since v3:
- PATCH 1: make it a new command which accepts 'mountpoints' argument in order to discover optional params are accepted or not.
- PATCH 2: code cleanups...
(v3: http://lists.gnu.org/archive/html/qemu-devel/2014-05/msg04571.html)

---
Tomoki Sekiyama (2):
qga: Add guest-fsfreeze-freeze-list command
qga: Add guest-get-fsinfo command

 qga/commands-posix.c |  454 ++++++++++++++++++++++++++++++++++++++++++++++++++
 qga/commands-win32.c |   15 ++
 qga/qapi-schema.json |   96 +++++++++++
 3 files changed, 563 insertions(+), 2 deletions(-)
Signature · 2 lines
Regards,
Tomoki Sekiyama
#1 lore raw

PATCH v4 1/2 qga: Add guest-fsfreeze-freeze-list command

If an array of mount point paths is specified as 'mountpoints' argument of guest-fsfreeze-freeze-list, qemu-ga will only freeze the file systems mounted on specified paths in Linux guests. Otherwise, it works as the same way as guest-fsfreeze-freeze.
This would be useful when the host wants to create partial disk snapshots.

---
 qga/commands-posix.c |   32 +++++++++++++++++++++++++++++++-
 qga/commands-win32.c |    9 +++++++++
 qga/qapi-schema.json |   17 +++++++++++++++++
 3 files changed, 57 insertions(+), 1 deletion(-)
qga/commands-posix.c +31 -1
index 34ddba0..212988f 100644
@@ -710,13 +710,21 @@ GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **errp)
     return GUEST_FSFREEZE_STATUS_THAWED;
 }
 
+int64_t qmp_guest_fsfreeze_freeze(Error **errp)
+{
+    return qmp_guest_fsfreeze_freeze_list(false, NULL, errp);
+}
+
 /*
  * Walk list of mounted file systems in the guest, and freeze the ones which
  * are real local file systems.
  */
-int64_t qmp_guest_fsfreeze_freeze(Error **errp)
+int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints,
+                                       strList *mountpoints,
+                                       Error **errp)
 {
     int ret = 0, i = 0;
+    strList *list;
     FsMountList mounts;
     struct FsMount *mount;
     Error *local_err = NULL;
@@ -741,6 +749,19 @@ int64_t qmp_guest_fsfreeze_freeze(Error **errp)
     ga_set_frozen(ga_state);
 
     QTAILQ_FOREACH_REVERSE(mount, &mounts, FsMountList, next) {
+        /* To issue fsfreeze in the reverse order of mounts, check if the
+         * mount is listed in the list here */
+        if (has_mountpoints) {
+            for (list = mountpoints; list; list = list->next) {
+                if (strcmp(list->value, mount->dirname) == 0) {
+                    break;
+                }
+            }
+            if (!list) {
+                continue;
+            }
+        }
+
         fd = qemu_open(mount->dirname, O_RDONLY);
         if (fd == -1) {
             error_setg_errno(errp, errno, "failed to open %s", mount->dirname);
@@ -1474,6 +1495,15 @@ int64_t qmp_guest_fsfreeze_freeze(Error **errp)
     return 0;
 }
 
+int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints,
+                                       strList *mountpoints,
+                                       Error **errp)
+{
+    error_set(errp, QERR_UNSUPPORTED);
+
+    return 0;
+}
+
 int64_t qmp_guest_fsfreeze_thaw(Error **errp)
 {
     error_set(errp, QERR_UNSUPPORTED);
qga/commands-win32.c +9 -0
index d793dd0..6184d05 100644
@@ -204,6 +204,15 @@ error:
     return 0;
 }
 
+int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints,
+                                       strList *mountpoints,
+                                       Error **errp)
+{
+    error_set(errp, QERR_UNSUPPORTED);
+
+    return 0;
+}
+
 /*
  * Thaw local file systems using Volume Shadow-copy Service.
  */
qga/qapi-schema.json +17 -0
index a8cdcb3..caa4612 100644
@@ -387,6 +387,23 @@
   'returns': 'int' }
 
 ##
+# @guest-fsfreeze-freeze-list:
+#
+# Sync and freeze specified guest filesystems
+#
+# @mountpoints: #optional an array of mountpoints of filesystems to be frozen.
+#               If omitted, every mounted filesystem is frozen.
+#
+# Returns: Number of file systems currently frozen. On error, all filesystems
+# will be thawed.
+#
+# Since: 2.1
+##
+{ 'command': 'guest-fsfreeze-freeze-list',
+  'data':    { '*mountpoints': ['str'] },
+  'returns': 'int' }
+
+##
 # @guest-fsfreeze-thaw:
 #
 # Unfreeze all frozen guest filesystems
Reviewed-by on v4 1/2
#2 lore raw
Quoted 6 lines · Tomoki Sekiyama

If an array of mount point paths is specified as 'mountpoints' argument of guest-fsfreeze-freeze-list, qemu-ga will only freeze the file systems mounted on specified paths in Linux guests. Otherwise, it works as the same way as guest-fsfreeze-freeze.
This would be useful when the host wants to create partial disk snapshots.

Quoted 6 lines
---
 qga/commands-posix.c |   32 +++++++++++++++++++++++++++++++-
 qga/commands-win32.c |    9 +++++++++
 qga/qapi-schema.json |   17 +++++++++++++++++
 3 files changed, 57 insertions(+), 1 deletion(-)
Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
#3 lore raw

PATCH v4 2/2 qga: Add guest-get-fsinfo command

In reply to Tomoki Sekiyama

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:

    NAME           MAJ:MIN RM  SIZE RO TYPE MOUNTPOINT
    sdb              8:16   0    1G  0 disk
    `-sdb1           8:17   0 1024M  0 part
      `-vg0-lv0    253:1    0  1.4G  0 lvm  /mnt/test
    sdc              8:32   0    1G  0 disk
    `-sdc1           8:33   0  512M  0 part
      `-vg0-lv0    253:1    0  1.4G  0 lvm  /mnt/test
    vda            252:0    0   25G  0 disk
    `-vda1         252:1    0   25G  0 part /

where sdb is a SCSI disk with PCI controller 0000:00:0a.0 and ID=1,
sdc is an IDE disk with PCI controller 0000:00:01.1, and vda is a virtio-blk disk with PCI device 0000:00:06.0,

guest-get-fsinfo command will return the following result:

    {"return":
     [{"name":"dm-1",
       "mountpoint":"/mnt/test",
       "disk":[
        {"bus-type":"scsi","bus":0,"unit":1,"target":0,
         "pci-controller":{"bus":0,"slot":10,"domain":0,"function":0}},
        {"bus-type":"ide","bus":0,"unit":0,"target":0,
         "pci-controller":{"bus":0,"slot":1,"domain":0,"function":1}}],
       "type":"xfs"},
      {"name":"vda1", "mountpoint":"/",
       "disk":[
        {"bus-type":"virtio","bus":0,"unit":0,"target":0,
         "pci-controller":{"bus":0,"slot":6,"domain":0,"function":0}}],
       "type":"ext4"}]}

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.

---
 qga/commands-posix.c |  422 ++++++++++++++++++++++++++++++++++++++++++++++++++
 qga/commands-win32.c |    6 +
 qga/qapi-schema.json |   79 +++++++++
 3 files changed, 506 insertions(+), 1 deletion(-)
qga/commands-posix.c +421 -1
index 212988f..b1756b3 100644
@@ -18,6 +18,7 @@
 #include <unistd.h>
 #include <errno.h>
 #include <fcntl.h>
+#include <dirent.h>
 #include <stdio.h>
 #include <string.h>
 #include <sys/stat.h>
@@ -575,6 +576,7 @@ static void guest_file_init(void)
 typedef struct FsMount {
     char *dirname;
     char *devtype;
+    unsigned int devmajor, devminor;
     QTAILQ_ENTRY(FsMount) next;
 } FsMount;
 
@@ -596,15 +598,40 @@ static void free_fs_mount_list(FsMountList *mounts)
      }
 }
 
+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);
+        return 0;
+    }
+    return -1;
+}
+
 /*
  * Walk the mount table and build a list of local file systems
  */
-static void build_fs_mount_list(FsMountList *mounts, Error **errp)
+static void build_fs_mount_list_from_mtab(FsMountList *mounts, Error **errp)
 {
     struct mntent *ment;
     FsMount *mount;
     char const *mtab = "/proc/self/mounts";
     FILE *fp;
+    unsigned int devmajor, devminor;
 
     fp = setmntent(mtab, "r");
     if (!fp) {
@@ -624,20 +651,407 @@ static void build_fs_mount_list(FsMountList *mounts, Error **errp)
             (strcmp(ment->mnt_type, "cifs") == 0)) {
             continue;
         }
+        if (dev_major_minor(ment->mnt_fsname, &devmajor, &devminor) == -2) {
+            /* Skip bind mounts */
+            continue;
+        }
 
         mount = g_malloc0(sizeof(FsMount));
         mount->dirname = g_strdup(ment->mnt_dir);
         mount->devtype = g_strdup(ment->mnt_type);
+        mount->devmajor = devmajor;
+        mount->devminor = devminor;
 
         QTAILQ_INSERT_TAIL(mounts, mount, next);
     }
 
     endmntent(fp);
 }
+
+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') {
+            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 {
+            name[j++] = name[i];
+        }
+    }
+}
+
+static void build_fs_mount_list(FsMountList *mounts, Error **errp)
+{
+    FsMount *mount;
+    char const *mountinfo = "/proc/self/mountinfo";
+    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,
+                     &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);
+        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);
+        mount->devmajor = devmajor;
+        mount->devminor = devminor;
+
+        QTAILQ_INSERT_TAIL(mounts, mount, next);
+    }
+    free(line);
+
+    fclose(fp);
+}
 #endif
 
 #if defined(CONFIG_FSFREEZE)
 
+static char *get_pci_driver(char const *syspath, int pathlen, Error **errp)
+{
+    char *path;
+    char *dpath;
+    char *driver = NULL;
+    char buf[PATH_MAX];
+    ssize_t len;
+
+    path = g_strndup(syspath, pathlen);
+    dpath = g_strdup_printf("%s/driver", path);
+    len = readlink(dpath, buf, sizeof(buf)-1);
+    if (len != -1) {
+        buf[len] = 0;
+        driver = g_strdup(basename(buf));
+    }
+    g_free(dpath);
+    g_free(path);
+    return driver;
+}
+
+static int compare_uint(const void *_a, const void *_b)
+{
+    unsigned int a = *(unsigned int *)_a;
+    unsigned int b = *(unsigned int *)_b;
+
+    return a < b ? -1 : a > b ? 1 : 0;
+}
+
+/* Walk the specified sysfs path and build a sorted list of ata port numbers */
+static int build_ata_ports(char const *syspath, char const *ata,
+                           unsigned int *ports, int ports_max, Error **errp)
+{
+    char *path;
+    DIR *dir;
+    struct dirent *entry;
+    int i = 0;
+
+    path = g_strndup(syspath, ata - syspath);
+    dir = opendir(path);
+    if (!dir) {
+        error_setg_errno(errp, errno, "opendir(\"%s\")", path);
+        g_free(path);
+        return -1;
+    }
+
+    while (i < ports_max) {
+        entry = readdir(dir);
+        if (!entry) {
+            break;
+        }
+        if (sscanf(entry->d_name, "ata%d", ports+i) == 1) {
+            ++i;
+        }
+    }
+
+    qsort(ports, i, sizeof(ports[0]), compare_uint);
+
+    g_free(path);
+    closedir(dir);
+    return i;
+}
+
+/* Store disk device info specified by @sysfs into @fs */
+static void __build_guest_fsinfo(char const *syspath,
+                                 GuestFilesystemInfo *fs, Error **errp)
+{
+    unsigned int pci[4], ata, tgt[3], ports[8];
+    int i, nports = 0, pcilen;
+    GuestDiskAddress *disk;
+    GuestPCIAddress *pciaddr;
+    GuestDiskAddressList *list = NULL;
+    bool has_ata = false, has_tgt = false;
+    char *p, *driver = NULL;
+
+    p = strstr(syspath, "/devices/pci");
+    if (!p || sscanf(p+12, "%*x:%*x/%x:%x:%x.%x%n",
+                     pci, pci+1, pci+2, pci+3, &pcilen) < 4) {
+        g_debug("only pci device is supported: sysfs path \"%s\"", syspath);
+        return;
+    }
+
+    driver = get_pci_driver(syspath, (p+12+pcilen)-syspath, errp);
+    if (!driver) {
+        goto cleanup;
+    }
+
+    p = strstr(syspath, "/target");
+    if (p && sscanf(p+7, "%*u:%*u:%*u/%*u:%u:%u:%u", tgt, tgt+1, tgt+2) == 3) {
+        has_tgt = true;
+    }
+
+    p = strstr(syspath, "/ata");
+    if (p && sscanf(p+4, "%u", &ata) == 1) {
+        has_ata = true;
+        nports = build_ata_ports(syspath, p, ports,
+                                 sizeof(ports)/sizeof(ports[0]), errp);
+        if (nports < 0) {
+            goto cleanup;
+        }
+    }
+
+    pciaddr = g_malloc0(sizeof(*pciaddr));
+    pciaddr->domain = pci[0];
+    pciaddr->bus = pci[1];
+    pciaddr->slot = pci[2];
+    pciaddr->function = pci[3];
+
+    disk = g_malloc0(sizeof(*disk));
+    disk->pci_controller = pciaddr;
+
+    list = g_malloc0(sizeof(*list));
+    list->value = disk;
+
+    if (strcmp(driver, "ata_piix") == 0) {
+        /* an ata port per ide bus, target*:0:<unit>:0 */
+        if (!has_ata || !has_tgt) {
+            g_debug("invalid sysfs path '%s' (driver '%s')", syspath, driver);
+            goto cleanup;
+        }
+        for (i = 0; i < nports; i++) {
+            if (ata == ports[i]) {
+                disk->bus_type = GUEST_DISK_BUS_TYPE_IDE;
+                disk->bus = i;
+                disk->unit = tgt[1];
+                break;
+            }
+        }
+        if (i >= nports) {
+            g_debug("no ata port for '%s' (driver '%s')", syspath, driver);
+            goto cleanup;
+        }
+    } else if (strcmp(driver, "sym53c8xx") == 0) {
+        /* scsi(LSI Logic): target*:0:<unit>:0 */
+        if (!has_tgt) {
+            g_debug("invalid sysfs path '%s' (driver '%s')", syspath, driver);
+            goto cleanup;
+        }
+        disk->bus_type = GUEST_DISK_BUS_TYPE_SCSI;
+        disk->unit = tgt[1];
+    } else if (strcmp(driver, "virtio-pci") == 0) {
+        if (has_tgt) {
+            /* virtio-scsi: target*:0:0:<unit> */
+            disk->bus_type = GUEST_DISK_BUS_TYPE_SCSI;
+            disk->unit = tgt[2];
+        } else {
+            /* virtio-blk: 1 disk per 1 device */
+            disk->bus_type = GUEST_DISK_BUS_TYPE_VIRTIO;
+        }
+    } else if (strcmp(driver, "ahci") == 0) {
+        /* ahci: an ata port per unit */
+        if (!has_ata || !has_tgt) {
+            g_debug("invalid sysfs path '%s' (driver '%s')", syspath, driver);
+            goto cleanup;
+        }
+        for (i = 0; i < nports; i++) {
+            if (ata == ports[i]) {
+                disk->unit = i;
+                disk->bus_type = GUEST_DISK_BUS_TYPE_SATA;
+                break;
+            }
+        }
+        if (i >= nports) {
+            g_debug("no ata port for '%s' (driver '%s')", syspath, driver);
+            goto cleanup;
+        }
+    } else {
+        g_debug("unknown driver '%s' (sysfs path '%s')", driver, syspath);
+        goto cleanup;
+    }
+
+    list->next = fs->disk;
+    fs->disk = list;
+    g_free(driver);
+    return;
+
+cleanup:
+    if (list) {
+        qapi_free_GuestDiskAddressList(list);
+    }
+    g_free(driver);
+}
+
+static void _build_guest_fsinfo(char const *dirpath,
+                                GuestFilesystemInfo *fs, Error **errp);
+
+/* Store a list of slave devices of virtual volume specified by @syspath into
+ * @fs */
+static void __build_guest_fsinfo_virtual(char const *syspath,
+                                         GuestFilesystemInfo *fs,
+                                         Error **errp)
+{
+    DIR *dir;
+    char *dirpath;
+    struct dirent entry, *result;
+
+    dirpath = g_strdup_printf("%s/slaves", syspath);
+    dir = opendir(dirpath);
+    if (!dir) {
+        error_setg_errno(errp, errno, "opendir(\"%s\")", dirpath);
+        g_free(dirpath);
+        return;
+    }
+    g_free(dirpath);
+
+    for (;;) {
+        if (readdir_r(dir, &entry, &result) != 0) {
+            error_setg_errno(errp, errno, "readdir_r(\"%s\")", dirpath);
+            break;
+        }
+        if (!result) {
+            break;
+        }
+
+        if (entry.d_type == DT_LNK) {
+            g_debug(" slave device '%s'", entry.d_name);
+            dirpath = g_strdup_printf("%s/slaves/%s", syspath, entry.d_name);
+            _build_guest_fsinfo(dirpath, fs, errp);
+            g_free(dirpath);
+
+            if (*errp) {
+                break;
+            }
+        }
+    }
+
+    closedir(dir);
+}
+
+static void _build_guest_fsinfo(char const *dirpath,
+                                GuestFilesystemInfo *fs, Error **errp)
+{
+    char *syspath = realpath(dirpath, NULL);
+
+    if (!syspath) {
+        error_setg_errno(errp, errno, "realpath(\"%s\")", dirpath);
+        return;
+    }
+
+    if (!fs->name) {
+        fs->name = g_strdup(basename(syspath));
+    }
+
+    g_debug("  parse sysfs path '%s'", syspath);
+
+    if (strstr(syspath, "/devices/virtual/block/")) {
+        __build_guest_fsinfo_virtual(syspath, fs, errp);
+    } else {
+        __build_guest_fsinfo(syspath, fs, errp);
+    }
+
+    free(syspath);
+}
+
+/* Return a list of the disk device(s)' info which @mount lies on */
+static GuestFilesystemInfo *build_guest_fsinfo(struct FsMount *mount,
+                                               Error **errp)
+{
+    GuestFilesystemInfo *fs = g_malloc0(sizeof(*fs));
+    char *dirpath = g_strdup_printf("/sys/dev/block/%u:%u",
+                                    mount->devmajor, mount->devminor);
+
+    fs->mountpoint = g_strdup(mount->dirname);
+    fs->type = g_strdup(mount->devtype);
+    _build_guest_fsinfo(dirpath, fs, errp);
+
+    g_free(dirpath);
+    return fs;
+}
+
+GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
+{
+    FsMountList mounts;
+    struct FsMount *mount;
+    GuestFilesystemInfoList *new, *ret = NULL;
+    Error *local_err = NULL;
+
+    QTAILQ_INIT(&mounts);
+    build_fs_mount_list(&mounts, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return NULL;
+    }
+
+    QTAILQ_FOREACH(mount, &mounts, next) {
+        g_debug("Building guest fsinfo for '%s'", mount->dirname);
+
+        new = g_malloc0(sizeof(*ret));
+        new->value = build_guest_fsinfo(mount, &local_err);
+        new->next = ret;
+        ret = new;
+        if (local_err) {
+            error_propagate(errp, local_err);
+            qapi_free_GuestFilesystemInfoList(ret);
+            ret = NULL;
+            break;
+        }
+    }
+
+    free_fs_mount_list(&mounts);
+    return ret;
+}
+
+
 typedef enum {
     FSFREEZE_HOOK_THAW = 0,
     FSFREEZE_HOOK_FREEZE,
@@ -1481,6 +1895,12 @@ int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
 
 #if !defined(CONFIG_FSFREEZE)
 
+GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
+{
+    error_set(errp, QERR_UNSUPPORTED);
+    return NULL;
+}
+
 GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **errp)
 {
     error_set(errp, QERR_UNSUPPORTED);
qga/commands-win32.c +6 -0
index 6184d05..8c9ec34 100644
@@ -150,6 +150,12 @@ void qmp_guest_file_flush(int64_t handle, Error **errp)
     error_set(errp, QERR_UNSUPPORTED);
 }
 
+GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
+{
+    error_set(errp, QERR_UNSUPPORTED);
+    return NULL;
+}
+
 /*
  * Return status of freeze/thaw
  */
qga/qapi-schema.json +79 -0

82 lines not on this page · raw message

#4 lore raw

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

Quoted 7 lines · Tomoki Sekiyama

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>

Quoted 22 lines

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.

---
 qga/commands-posix.c |  422 ++++++++++++++++++++++++++++++++++++++++++++++++++
 qga/commands-win32.c |    6 +
 qga/qapi-schema.json |   79 +++++++++
 3 files changed, 506 insertions(+), 1 deletion(-)
qga/commands-posix.c +9 -0
+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) {

qga/commands-posix.c static void free_fs_mount_list(FsMountList *mounts) · line 619

Quoted 10 lines
qga/commands-posix.c +10 -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.

qga/commands-posix.c static void build_fs_mount_list(FsMountList *mounts, Error **errp) · line 680

Quoted 11 lines
qga/commands-posix.c +11 -0
+
+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 '+'

qga/commands-posix.c static void build_fs_mount_list(FsMountList *mounts, Error **errp) · line 692

Quoted 12 lines
qga/commands-posix.c +12 -0
+            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?

qga/commands-posix.c static void build_fs_mount_list(FsMountList *mounts, Error **errp) · line 701

Quoted 9 lines
qga/commands-posix.c +9 -0
+            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?

Quoted 7 lines
qga/commands-posix.c +7 -0
+    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;
+

qga/commands-posix.c static void build_fs_mount_list(FsMountList *mounts, Error **errp) · line 718

Quoted 10 lines
qga/commands-posix.c +10 -0
+    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.

qga/commands-posix.c static void build_fs_mount_list(FsMountList *mounts, Error **errp) · line 727

Quoted 9 lines
qga/commands-posix.c +9 -0
+                     &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 '-'

qga/commands-posix.c static void build_fs_mount_list(FsMountList *mounts, Error **errp) · line 738

Quoted 11 lines
qga/commands-posix.c +11 -0
+        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 '+'

qga/commands-posix.c static void build_fs_mount_list(FsMountList *mounts, Error **errp) · line 816

Quoted 4 lines
qga/commands-posix.c +4 -0
+
+/* 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.

Review

Quoted 2 lines
(quoted diff) +2 -0
+
+    driver = get_pci_driver(syspath, (p+12+pcilen)-syspath, errp);

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

Review

Quoted 2 lines
(quoted diff) +2 -0
+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?

Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
#5 lore raw

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

Hi Eric,

Thank you for the review.

Quoted 41 lines · Eric Blake
Quoted 7 lines · Tomoki Sekiyama

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>

Quoted 31 lines

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.

---
 qga/commands-posix.c |  422
++++++++++++++++++++++++++++++++++++++++++++++++++
 qga/commands-win32.c |    6 +
 qga/qapi-schema.json |   79 +++++++++
 3 files changed, 506 insertions(+), 1 deletion(-)
(quoted diff) +2 -0
+static int dev_major_minor(const char *devpath,
+                           unsigned int *devmajor, unsigned int

*devminor)

qga/commands-posix.c +8 -0
+{
+    struct stat st;
+
+    *devmajor = 0;
+    *devminor = 0;
+
+    if (stat(devpath, &st) < 0) {
+        slog("failed to stat device file '%s': %s", devpath,

strerror(errno));

qga/commands-posix.c +5 -0
+        return -1;
+    }
+    if (S_ISDIR(st.st_mode)) {
+        /* It is bind mount */
+        return -2;
Quoted 8 lines

qga/commands-posix.c static void free_fs_mount_list(FsMountList *mounts) · line 619

Quoted 4 lines
qga/commands-posix.c +4 -0
+    }
+    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.

Quoted 6 lines
Quoted 6 lines
qga/commands-posix.c +6 -0
+
+static void decode_mntname(char *name, int len)
+{
+    int i, j = 0;
+    for (i = 0; i <= len; i++) {
+        if (name[i] != '\\') {
Quoted 8 lines
Quoted 6 lines
qga/commands-posix.c +5 -0
+            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.

Quoted 16 lines
Quoted 12 lines
qga/commands-posix.c +3 -0
+            name[j++] = ' ';
+            i += 3;
+        } else if (name[i+1] == '0' && name[i+2] == '1' && name[i+3]

== '1') {

qga/commands-posix.c +3 -0
+            name[j++] = '\t';
+            i += 3;
+        } else if (name[i+1] == '0' && name[i+2] == '1' && name[i+3]

== '2') {

qga/commands-posix.c +3 -0
+            name[j++] = '\n';
+            i += 3;
+        } else if (name[i+1] == '1' && name[i+2] == '3' && name[i+3]

== '4') {

qga/commands-posix.c static void build_fs_mount_list(FsMountList *mounts, Error **errp) · line 692

Quoted 3 lines
qga/commands-posix.c +3 -0
+            name[j++] = '\\';
+            i += 3;
+        } else {
Quoted 4 lines

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.

Quoted 5 lines
Quoted 5 lines
qga/commands-posix.c +5 -0
+            name[j++] = name[i];
+        }
+    }
+}
+
Quoted 8 lines

qga/commands-posix.c static void build_fs_mount_list(FsMountList *mounts, Error **errp) · line 701

Quoted 4 lines
qga/commands-posix.c +4 -0
+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.

Quoted 16 lines
Quoted 16 lines
qga/commands-posix.c +16 -0
+    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
Quoted 8 lines
Quoted 3 lines

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

Quoted 4 lines
Quoted 4 lines
qga/commands-posix.c +4 -0
+                     &dev_s, &dev_e, &check);
+        if (ret < 3) {
+            continue;
+        }

<snip>

Quoted 10 lines

qga/commands-posix.c static void build_fs_mount_list(FsMountList *mounts, Error **errp) · line 816

Quoted 5 lines
qga/commands-posix.c +4 -0
+
+/* 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.

Quoted 5 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+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

#6 lore raw

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

Quoted 5 lines · Tomoki Sekiyama
Quoted 5 lines
Quoted 4 lines
qga/commands-posix.c +4 -0
+    }
+    if (S_ISBLK(st.st_mode)) {
+        *devmajor = major(st.st_rdev);
+        *devminor = minor(st.st_rdev);
Quoted 7 lines
Quoted 3 lines

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.

Quoted 12 lines
Quoted 11 lines
Quoted 6 lines
qga/commands-posix.c +3 -0
+    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.

Quoted 4 lines

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.

Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
#7 lore raw

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

Quoted 15 lines · Eric Blake
Quoted 5 lines · Tomoki Sekiyama
Quoted 5 lines
Quoted 4 lines
qga/commands-posix.c +4 -0
+    }
+    if (S_ISBLK(st.st_mode)) {
+        *devmajor = major(st.st_rdev);
+        *devminor = minor(st.st_rdev);
Quoted 7 lines
Quoted 3 lines

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.

Quoted 6 lines

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

Quoted 17 lines
Quoted 12 lines
Quoted 11 lines
Quoted 6 lines
qga/commands-posix.c +3 -0
+    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.

Quoted 4 lines

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?

Quoted 6 lines

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

#8 lore raw
In reply to Tomoki Sekiyama

Any comments for this patch?

Signature · 1 line
Tomoki Sekiyama
Quoted 36 lines · Tomoki Sekiyama

Hi,

This is v4 patch for qemu-ga to add functions to freeze specific file systems
mounted in a guest.

PATCH 1 adds a guest-fsfreeze-freeze-list command, which takes an additional
argument to specify which filesystems to be frozen, derived from guest-fsfreeze-freeze command.

PATCH 2 adds a guest-get-fsinfo command to get a list of mounted file systems in the guest with disk devices information to resolve backend disk images for a management layer such as libvirt.

Changes since v3:
- PATCH 1: make it a new command which accepts 'mountpoints' argument in order to discover optional params are accepted or not.
- PATCH 2: code cleanups...
(v3: http://lists.gnu.org/archive/html/qemu-devel/2014-05/msg04571.html)

---
Tomoki Sekiyama (2):
qga: Add guest-fsfreeze-freeze-list command
qga: Add guest-get-fsinfo command

qga/commands-posix.c | 454
++++++++++++++++++++++++++++++++++++++++++++++++++

qga/commands-win32.c |   15 ++
qga/qapi-schema.json |   96 +++++++++++
3 files changed, 563 insertions(+), 2 deletions(-)
Signature · 2 lines
Regards,
Tomoki Sekiyama