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

This patch fails to compile in my env throwing this error: CC qemuagenttest.o qemuagenttest.c: In function 'testQemuAgentGetFSInfoCommon.constprop': qemuagenttest.c:242:5: error: 'ret_def' may be used uninitialized in this function [-Werror=maybe-uninitialized] 242 | virDomainDefFree(ret_def); | ^~~~~~~~~~~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors make[2]: *** [Makefile:5856: qemuagenttest.o] Error 1 I attempted to fix it by doing: ---- diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c index be53f7e61a..00dcf26681 100644 --- a/tests/qemuagenttest.c +++ b/tests/qemuagenttest.c @@ -175,7 +175,7 @@ testQemuAgentGetFSInfoCommon(virDomainXMLOptionPtr xmlopt, int ret = -1; char *domain_filename = NULL; qemuMonitorTestPtr ret_test; - virDomainDefPtr ret_def; + virDomainDefPtr ret_def = NULL; if (!test || !def) return -1; ------ I was able to compile it, but then 'make check' got rekt: ---- [...] PASS: qemudomainsnapshotxml2xmltest PASS: qemucommandutiltest PASS: virsh-cpuset ../build-aux/test-driver: line 107: 19017 Segmentation fault (core dumped) "$@" > $log_file 2>&1 FAIL: qemuagenttest PASS: qemumigparamstest [...] ===================================================================== ======= Testsuite summary for libvirt 5.7.0 ===================================================================== ======= # TOTAL: 129 # PASS: 127 # SKIP: 1 # XFAIL: 0 # FAIL: 1 # XPASS: 0 # ERROR: 0 ----- Not sure if there is something wrong with the patch, or with the fix I made (seems unlikely, but ...) or perhaps something else missing in my env (is there any external libs needed for this?). Thanks, DHB

On 8/21/19 7:15 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() > --- > src/qemu/qemu_agent.c | 89 ++++++++++++++++++ > src/qemu/qemu_agent.h | 5 + > tests/qemuagenttest.c | 210 > +++++++++++++++++++++++++++++++++++++++--- > 3 files changed, 291 insertions(+), 13 deletions(-) > > diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c > index d5519cb243..c101805b23 100644 > --- a/src/qemu/qemu_agent.c > +++ b/src/qemu/qemu_agent.c > @@ -1953,6 +1953,95 @@ 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; > + > + nfs = qemuAgentGetFSInfoInternal(mon, &fsinfo, vmdef); > + > + 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 aa1e993649..cf80711e95 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; > + virDomainDefPtr ret_def; > > - 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; > + goto cleanup; > + } > + const char *name, *mountpoint, *fstype, *alias, *serial; > + unsigned int diskcount; > + unsigned long long bytesused, bytestotal; > + 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; > + 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);