[Qemu-devel] [PATCH v4 0/2] qga: Add guest-fsfreeze-freeze-list command

[Qemu-devel] [PATCH v4 0/2] qga: Add guest-fsfreeze-freeze-list command

[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(-)

-- 
Regards,
Tomoki Sekiyama

[Qemu-devel] [PATCH v4 1/2] qga: Add guest-fsfreeze-freeze-list command

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

Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com>
---
 qga/commands-posix.c |   32 +++++++++++++++++++++++++++++++-
 qga/commands-win32.c |    9 +++++++++
 qga/qapi-schema.json |   17 +++++++++++++++++
 3 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 34ddba0..212988f 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -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);
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index d793dd0..6184d05 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -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.
  */
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index a8cdcb3..caa4612 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -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

Re: [Qemu-devel] [PATCH v4 1/2] qga: Add guest-fsfreeze-freeze-list command

[Eric Blake]

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

On 06/05/2014 08:57 AM, Tomoki Sekiyama wrote:
> 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.
> 
> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com>
> ---
>  qga/commands-posix.c |   32 +++++++++++++++++++++++++++++++-
>  qga/commands-win32.c |    9 +++++++++
>  qga/qapi-schema.json |   17 +++++++++++++++++
>  3 files changed, 57 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

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

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

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

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

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 212988f..b1756b3 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -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);
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 6184d05..8c9ec34 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -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
  */
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index caa4612..8604a68 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -659,3 +659,82 @@
 { 'command': 'guest-set-vcpus',
   'data':    {'vcpus': ['GuestLogicalProcessor'] },
   'returns': 'int' }
+
+##
+# @GuestDiskBusType
+#
+# An enumeration of bus type of disks
+#
+# @ide: IDE disks
+# @fdc: floppy disks
+# @scsi: SCSI disks
+# @virtio: virtio disks
+# @xen: Xen disks
+# @usb: USB disks
+# @uml: UML disks
+# @sata: SATA disks
+# @sd: SD cards
+#
+# Since: 2.1
+##
+{ 'enum': 'GuestDiskBusType',
+  'data': [ 'ide', 'fdc', 'scsi', 'virtio', 'xen', 'usb', 'uml', 'sata',
+            'sd' ] }
+
+##
+# @GuestPCIAddress:
+#
+# @domain: domain id
+# @bus: bus id
+# @slot: slot id
+# @function: function id
+#
+# Since: 2.1
+##
+{ 'type': 'GuestPCIAddress',
+  'data': {'domain': 'int', 'bus': 'int',
+           'slot': 'int', 'function': 'int'} }
+
+##
+# @GuestDiskAddress:
+#
+# @pci-controller: controller's PCI address
+# @type: bus type
+# @bus: bus id
+# @target: target id
+# @unit: unit id
+#
+# Since: 2.1
+##
+{ 'type': 'GuestDiskAddress',
+  'data': {'pci-controller': 'GuestPCIAddress',
+           'bus-type': 'GuestDiskBusType',
+           'bus': 'int', 'target': 'int', 'unit': 'int'} }
+
+##
+# @GuestFilesystemInfo
+#
+# @name: disk name
+# @mountpoint: mount point path
+# @type: file system type string
+# @disk: an array of disk hardware information that the volume lies on,
+#        which may be empty if the disk type is not supported
+#
+# Since: 2.1
+##
+{ 'type': 'GuestFilesystemInfo',
+  'data': {'name': 'str', 'mountpoint': 'str', 'type': 'str',
+           'disk': ['GuestDiskAddress']} }
+
+##
+# @guest-get-fsinfo:
+#
+# Returns: The list of filesystems information mounted in the guest.
+#          The returned mountpoints may be specified to
+#          @guest-fsfreeze-freeze-list.
+#          Network filesystems (such as CIFS and NFS) are not listed.
+#
+# Since: 2.1
+##
+{ 'command': 'guest-get-fsinfo',
+  'returns': ['GuestFilesystemInfo'] }

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

[Eric Blake]

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

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

[Tomoki Sekiyama]

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

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

[Eric Blake]

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

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

[Tomoki Sekiyama]

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

Re: [Qemu-devel] [PATCH v4 0/2] qga: Add guest-fsfreeze-freeze-list command

[Tomoki Sekiyama]

Any comments for this patch?

-- 
Tomoki Sekiyama



On 6/5/14 10:57 , "Tomoki Sekiyama" <tomoki.sekiyama@hds.com> wrote:

>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(-)
>
>-- 
>Regards,
>Tomoki Sekiyama