[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 7/9] qemu: add helper for getting full FSInfo





On 8/23/19 1:31 PM, Jonathon Jongsma wrote:
This function adds the complete filesystem information returned by the
qemu agent to an array of typed parameters with field names intended to
to be returned by virDomainGetGuestInfo()

Signed-off-by: Jonathon Jongsma <jjongsma redhat com>
---

Tested-by: Daniel Henrique Barboza <danielhb413 gmail com>


A few comments below:


  src/qemu/qemu_agent.c |  90 ++++++++++++++++++
  src/qemu/qemu_agent.h |   5 +
  tests/qemuagenttest.c | 210 +++++++++++++++++++++++++++++++++++++++---
  3 files changed, 292 insertions(+), 13 deletions(-)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index 8d54979f89..169ad74f6f 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -1953,6 +1953,96 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info,
      return ret;
  }
+int
+qemuAgentGetFSInfoParams(qemuAgentPtr mon,
+                         virTypedParameterPtr *params,
+                         int *nparams, int *maxparams,
+                         virDomainDefPtr vmdef)
+{
+    int ret = -1;
+    qemuAgentFSInfoPtr *fsinfo = NULL;
+    size_t i, j;
+    int nfs;
+
+    if ((nfs = qemuAgentGetFSInfoInternal(mon, &fsinfo, vmdef)) < 0)
+        return -1;
+
+    if (virTypedParamsAddUInt(params, nparams, maxparams,
+                              "fs.count", nfs) < 0)
+        goto cleanup;
+
+    for (i = 0; i < nfs; i++) {
+        char param_name[VIR_TYPED_PARAM_FIELD_LENGTH];
+        snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
+                 "fs.%zu.name", i);
+        if (virTypedParamsAddString(params, nparams, maxparams,
+                                    param_name, fsinfo[i]->name) < 0)
+            goto cleanup;
+        snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
+                 "fs.%zu.mountpoint", i);
+        if (virTypedParamsAddString(params, nparams, maxparams,
+                                    param_name, fsinfo[i]->mountpoint) < 0)
+            goto cleanup;
+        snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
+                 "fs.%zu.fstype", i);
+        if (virTypedParamsAddString(params, nparams, maxparams,
+                                    param_name, fsinfo[i]->fstype) < 0)
+            goto cleanup;
+
+        /* disk usage values are not returned by older guest agents, so
+         * only add the params if the value is set */
+        snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
+                 "fs.%zu.total-bytes", i);
+        if (fsinfo[i]->total_bytes != -1 &&
+            virTypedParamsAddULLong(params, nparams, maxparams,
+                                    param_name, fsinfo[i]->total_bytes) < 0)
+            goto cleanup;
+
+        snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
+                 "fs.%zu.used-bytes", i);
+        if (fsinfo[i]->used_bytes != -1 &&
+            virTypedParamsAddULLong(params, nparams, maxparams,
+                                    param_name, fsinfo[i]->used_bytes) < 0)
+            goto cleanup;
+
+        snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
+                 "fs.%zu.disk.count", i);
+        if (virTypedParamsAddUInt(params, nparams, maxparams,
+                                  param_name, fsinfo[i]->ndisks) < 0)
+            goto cleanup;
+        for (j = 0; j < fsinfo[i]->ndisks; j++) {
+            qemuAgentDiskInfoPtr d = fsinfo[i]->disks[j];
+            snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
+                     "fs.%zu.disk.%zu.alias", i, j);
+            if (d->alias &&
+                virTypedParamsAddString(params, nparams, maxparams,
+                                        param_name, d->alias) < 0)
+                goto cleanup;
+
+            snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
+                     "fs.%zu.disk.%zu.serial", i, j);
+            if (d->serial &&
+                virTypedParamsAddString(params, nparams, maxparams,
+                                        param_name, d->serial) < 0)
+                goto cleanup;
+
+            snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
+                     "fs.%zu.disk.%zu.device", i, j);
+            if (d->devnode &&
+                virTypedParamsAddString(params, nparams, maxparams,
+                                        param_name, d->devnode) < 0)
+                goto cleanup;
+        }
+    }
+    ret = nfs;
+
+ cleanup:
+    for (i = 0; i < nfs; i++)
+        qemuAgentFSInfoFree(fsinfo[i]);
+    VIR_FREE(fsinfo);
+
+    return ret;
+}
static int
  qemuAgentGetFSInfoInternal(qemuAgentPtr mon, qemuAgentFSInfoPtr **info,
diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h
index 69b0176855..f6d74a2603 100644
--- a/src/qemu/qemu_agent.h
+++ b/src/qemu/qemu_agent.h
@@ -74,6 +74,11 @@ int qemuAgentFSThaw(qemuAgentPtr mon);
  int qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info,
                         virDomainDefPtr vmdef);
+int qemuAgentGetFSInfoParams(qemuAgentPtr mon,
+                             virTypedParameterPtr *params,
+                             int *nparams, int *maxparams,
+                             virDomainDefPtr vmdef);
+
  int qemuAgentSuspend(qemuAgentPtr mon,
                       unsigned int target);
diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c
index 0912a5f29a..105b32017a 100644
--- a/tests/qemuagenttest.c
+++ b/tests/qemuagenttest.c
@@ -168,38 +168,45 @@ testQemuAgentFSTrim(const void *data)
static int
-testQemuAgentGetFSInfo(const void *data)
+testQemuAgentGetFSInfoCommon(virDomainXMLOptionPtr xmlopt,
+                             qemuMonitorTestPtr *test,
+                             virDomainDefPtr *def)
  {
-    virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data;
-    qemuMonitorTestPtr test = qemuMonitorTestNewAgent(xmlopt);
+    int ret = -1;
      char *domain_filename = NULL;
-    virDomainDefPtr def = NULL;
-    virDomainFSInfoPtr *info = NULL;
-    int ret = -1, ninfo = 0, i;
+    qemuMonitorTestPtr ret_test = NULL;
+    virDomainDefPtr ret_def = NULL;
- if (!test)
+    if (!test || !def)
+        return -1;
+
+    if (!(ret_test = qemuMonitorTestNewAgent(xmlopt)))
          return -1;
if (virAsprintf(&domain_filename, "%s/qemuagentdata/fsinfo.xml",
                      abs_srcdir) < 0)
          goto cleanup;
- if (!(def = virDomainDefParseFile(domain_filename, driver.caps, xmlopt,
-                                      NULL, VIR_DOMAIN_DEF_PARSE_INACTIVE)))
+    if (!(ret_def = virDomainDefParseFile(domain_filename, driver.caps, xmlopt,
+                                          NULL, VIR_DOMAIN_DEF_PARSE_INACTIVE)))
          goto cleanup;
- if (qemuMonitorTestAddAgentSyncResponse(test) < 0)
+    if (qemuMonitorTestAddAgentSyncResponse(ret_test) < 0)
          goto cleanup;
- if (qemuMonitorTestAddItem(test, "guest-get-fsinfo",
+    if (qemuMonitorTestAddItem(ret_test, "guest-get-fsinfo",
                                 "{\"return\": ["
                                 "  {\"name\": \"sda1\", \"mountpoint\": \"/\","
+                               "   \"total-bytes\":952840192,"
+                               "   \"used-bytes\":229019648,"
                                 "   \"disk\": ["
-                               "     {\"bus-type\": \"ide\","
+                               "     {\"serial\": \"ARBITRARYSTRING\","
+                               "      \"bus-type\": \"ide\","
                                 "      \"bus\": 1, \"unit\": 0,"
                                 "      \"pci-controller\": {"
                                 "        \"bus\": 0, \"slot\": 1,"
                                 "        \"domain\": 0, \"function\": 1},"
+                               "      \"dev\": \"/dev/sda1\","
                                 "      \"target\": 0}],"
                                 "   \"type\": \"ext4\"},"
                                 "  {\"name\": \"dm-1\","
@@ -221,6 +228,32 @@ testQemuAgentGetFSInfo(const void *data)
                                 "  {\"name\": \"sdb1\","
                                 "   \"mountpoint\": \"/mnt/disk\","
                                 "   \"disk\": [], \"type\": \"xfs\"}]}") < 0)
+                               goto cleanup;
+    *test = ret_test;
+    ret_test = NULL;
+    *def = ret_def;
+    ret_def = NULL;
+    ret = 0;
+
+ cleanup:
+    VIR_FREE(domain_filename);
+    if (ret_test)
+        qemuMonitorTestFree(ret_test);
+    virDomainDefFree(ret_def);
+
+    return ret;
+}
+
+static int
+testQemuAgentGetFSInfo(const void *data)
+{
+    virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data;
+    qemuMonitorTestPtr test = NULL;
+    virDomainDefPtr def = NULL;
+    virDomainFSInfoPtr *info = NULL;
+    int ret = -1, ninfo = 0, i;
+
+    if (testQemuAgentGetFSInfoCommon(xmlopt, &test, &def) < 0)
          goto cleanup;
if ((ninfo = qemuAgentGetFSInfo(qemuMonitorTestGetAgent(test),
@@ -295,7 +328,157 @@ testQemuAgentGetFSInfo(const void *data)
      for (i = 0; i < ninfo; i++)
          virDomainFSInfoFree(info[i]);
      VIR_FREE(info);
-    VIR_FREE(domain_filename);
+    virDomainDefFree(def);
+    qemuMonitorTestFree(test);
+    return ret;
+}
+
+static int
+testQemuAgentGetFSInfoParams(const void *data)
+{
+    virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data;
+    qemuMonitorTestPtr test = NULL;
+    virDomainDefPtr def = NULL;
+    virTypedParameterPtr params = NULL;
+    int nparams = 0, maxparams = 0;
+    int ret = -1;
+    unsigned int count;
+
+    if (testQemuAgentGetFSInfoCommon(xmlopt, &test, &def) < 0)
+        goto cleanup;
+
+    if (qemuAgentGetFSInfoParams(qemuMonitorTestGetAgent(test),
+                                 &params, &nparams, &maxparams, def) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       "Failed to execute qemuAgentGetFSInfoParams()");
+        goto cleanup;
+    }
+
+    if (virTypedParamsGetUInt(params, nparams, "fs.count", &count) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       "expected filesystem count");
+        goto cleanup;
+    }
+
+    if (count != 3) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       "expected 3 filesystems information, got %d", count);
+        ret = -1;

You already initialized 'ret' with '-1' up there. Over here (and in the other
instances in this function) you don't need to set it again.

+        goto cleanup;
+    }
+    const char *name, *mountpoint, *fstype, *alias, *serial;
+    unsigned int diskcount;
+    unsigned long long bytesused, bytestotal;

Even considering that variable declaration in the middle of the
function body is okay, a line break before and after the declarations
can enhance code readability IMO.

+    if (virTypedParamsGetString(params, nparams, "fs.2.name", &name) < 0 ||
+        virTypedParamsGetString(params, nparams, "fs.2.mountpoint", &mountpoint) < 0 ||
+        virTypedParamsGetString(params, nparams, "fs.2.fstype", &fstype) < 0 ||
+        virTypedParamsGetULLong(params, nparams, "fs.2.used-bytes", &bytesused) <= 0 ||
+        virTypedParamsGetULLong(params, nparams, "fs.2.total-bytes", &bytestotal) <= 0 ||
+        virTypedParamsGetUInt(params, nparams, "fs.2.disk.count", &diskcount) < 0 ||
+        virTypedParamsGetString(params, nparams, "fs.2.disk.0.alias", &alias) < 0 ||
+        virTypedParamsGetString(params, nparams, "fs.2.disk.0.serial", &serial) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+            "Missing an expected parameter for sda1 (%s,%s)",
+            name, alias);
+        ret = -1;
+        goto cleanup;
+    }
+    if (
+        STRNEQ(name, "sda1") ||
+        STRNEQ(mountpoint, "/") ||
+        STRNEQ(fstype, "ext4") ||
+        bytesused != 229019648 ||
+        bytestotal != 952840192 ||
+        diskcount != 1 ||
+        STRNEQ(alias, "hdc") ||
+        STRNEQ(serial, "ARBITRARYSTRING"))
+    {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+            "unexpected filesystems information returned for sda1 (%s,%s)",
+            name, alias);
+        ret = -1;
+        goto cleanup;
+    }
+
+    const char *alias2;

Same comment as above.

+    if (virTypedParamsGetString(params, nparams, "fs.1.name", &name) < 0 ||
+        virTypedParamsGetString(params, nparams, "fs.1.mountpoint", &mountpoint) < 0 ||
+        virTypedParamsGetString(params, nparams, "fs.1.fstype", &fstype) < 0 ||
+        virTypedParamsGetULLong(params, nparams, "fs.1.used-bytes", &bytesused) == 1 ||
+        virTypedParamsGetULLong(params, nparams, "fs.1.total-bytes", &bytestotal) == 1 ||
+        virTypedParamsGetUInt(params, nparams, "fs.1.disk.count", &diskcount) < 0 ||
+        virTypedParamsGetString(params, nparams, "fs.1.disk.0.alias", &alias) < 0 ||
+        virTypedParamsGetString(params, nparams, "fs.1.disk.1.alias", &alias2) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+            "Incorrect parameters for dm-1 (%s,%s)",
+            name, alias);
+        ret = -1;
+        goto cleanup;
+    }
+    if (STRNEQ(name, "dm-1") ||
+        STRNEQ(mountpoint, "/opt") ||
+        STRNEQ(fstype, "vfat") ||
+        diskcount != 2 ||
+        STRNEQ(alias, "vda") ||
+        STRNEQ(alias2, "vdb")) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+            "unexpected filesystems information returned for dm-1 (%s,%s)",
+            name, alias);
+        ret = -1;
+        goto cleanup;
+    }
+
+    alias = NULL;
+    if (virTypedParamsGetString(params, nparams, "fs.0.name", &name) < 0 ||
+        virTypedParamsGetString(params, nparams, "fs.0.mountpoint", &mountpoint) < 0 ||
+        virTypedParamsGetString(params, nparams, "fs.0.fstype", &fstype) < 0 ||
+        virTypedParamsGetULLong(params, nparams, "fs.0.used-bytes", &bytesused) == 1 ||
+        virTypedParamsGetULLong(params, nparams, "fs.0.total-bytes", &bytestotal) == 1 ||
+        virTypedParamsGetUInt(params, nparams, "fs.0.disk.count", &diskcount) < 0 ||
+        virTypedParamsGetString(params, nparams, "fs.0.disk.0.alias", &alias) == 1) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+            "Incorrect parameters for sdb1 (%s,%s)",
+            name, alias);
+        ret = -1;
+        goto cleanup;
+    }
+    if (STRNEQ(name, "sdb1") ||
+        STRNEQ(mountpoint, "/mnt/disk") ||
+        STRNEQ(fstype, "xfs") ||
+        diskcount != 0 ||
+        alias != NULL) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+            "unexpected filesystems information returned for sdb1 (%s,%s)",
+            name, alias);
+        ret = -1;
+        goto cleanup;
+    }
+
+    if (qemuMonitorTestAddAgentSyncResponse(test) < 0)
+        goto cleanup;
+
+    if (qemuMonitorTestAddItem(test, "guest-get-fsinfo",
+                               "{\"error\":"
+                               "    {\"class\":\"CommandDisabled\","
+                               "     \"desc\":\"The command guest-get-fsinfo "
+                                               "has been disabled for "
+                                               "this instance\","
+                               "     \"data\":{\"name\":\"guest-get-fsinfo\"}"
+                               "    }"
+                               "}") < 0)
+        goto cleanup;
+
+    if (qemuAgentGetFSInfoParams(qemuMonitorTestGetAgent(test), &params,
+                                 &nparams, &maxparams, def) != -1) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       "agent get-fsinfo command should have failed");
+        goto cleanup;
+    }
+
+    ret = 0;
+
+ cleanup:
+    virTypedParamsFree(params, nparams);
      virDomainDefFree(def);
      qemuMonitorTestFree(test);
      return ret;
@@ -1288,6 +1471,7 @@ mymain(void)
      DO_TEST(FSFreeze);
      DO_TEST(FSThaw);
      DO_TEST(FSTrim);
+    DO_TEST(GetFSInfoParams);
      DO_TEST(GetFSInfo);
      DO_TEST(Suspend);
      DO_TEST(Shutdown);


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]