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

On Thu, 2019-08-22 at 19:02 -0300, Daniel Henrique Barboza wrote: [...] Hmm, sorry for the sloppiness. I ran these tests multiple times, but apparently after a final rebase, I introduced something that I failed to catch before submitting. Thanks for testing it out.

Jonathon > > 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); >