Flagged a couple of typos that can be fixed by the maintainer when pushing upstream or by in a later version, if a new version is needed. Aside for the typos, LGTM: Reviewed-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; Tested-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; On 8/23/19 1:31 PM, Jonathon Jongsma wrote: s/ispired/inspired Capital "T" after the period. ". The information ..." s/strign/string s/inforamtion/information

On 8/23/19 1:31 PM, Jonathon Jongsma wrote: Reviewed-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; Tested-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt;

This function fetches the list of logged-in users from the qemu agent and adds them to a list of typed parameters so that they can be used internally in libvirt. Also add some basic tests for the function. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- src/qemu/qemu_agent.c | 91 +++++++++++++++++++++++ src/qemu/qemu_agent.h | 2 + tests/qemuagenttest.c | 168 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 261 insertions(+) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 361db299a5..963a4b9359 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -2240,3 +2240,94 @@ qemuAgentSetUserPassword(qemuAgentPtr mon, VIR_FREE(password64); return ret; } + +int +qemuAgentGetUsers(qemuAgentPtr mon, + virTypedParameterPtr *params, + int *nparams, + int *maxparams) +{ + int ret = -1; + size_t i; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + virJSONValuePtr data = NULL; + size_t ndata; + const char *strvalue; + + if (!(cmd = qemuAgentMakeCommand("guest-get-users", NULL))) + return -1; + + if (qemuAgentCommand(mon, cmd, &reply, true, + VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0) + goto cleanup; + + if (!(data = virJSONValueObjectGetArray(reply, "return"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest-get-users reply was missing return data")); + goto cleanup; + } + + if (!virJSONValueIsArray(data)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Malformed guest-get-users data array")); + goto cleanup; + } + + ndata = virJSONValueArraySize(data); + + if (virTypedParamsAddUInt(params, nparams, maxparams, + "user.count", ndata) < 0) + goto cleanup; + + for (i = 0; i < ndata; i++) { + char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; + virJSONValuePtr entry = virJSONValueArrayGet(data, i); + + if (!entry) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("array element missing in guest-get-users return " + "value")); + goto cleanup; + } + + if (!(strvalue = virJSONValueObjectGetString(entry, "user"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("'user' missing in reply of guest-get-users")); + goto cleanup; + } + + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, "user.%zu.name", i); + if (virTypedParamsAddString(params, nparams, maxparams, + param_name, strvalue) < 0) + goto cleanup; + + /* 'domain' is only present for windows guests */ + if ((strvalue = virJSONValueObjectGetString(entry, "domain"))) { + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, + "user.%zu.domain", i); + if (virTypedParamsAddString(params, nparams, maxparams, + param_name, strvalue) < 0) + goto cleanup; + } + + double logintime; + if (virJSONValueObjectGetNumberDouble(entry, "login-time", &logintime) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("'login-time' missing in reply of guest-get-users")); + goto cleanup; + } + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, + "user.%zu.login-time", i); + if (virTypedParamsAddULLong(params, nparams, maxparams, + param_name, logintime * 1000) < 0) + goto cleanup; + } + + ret = ndata; + + cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h index 6ae9fe54da..05621b521a 100644 --- a/src/qemu/qemu_agent.h +++ b/src/qemu/qemu_agent.h @@ -120,3 +120,5 @@ int qemuAgentSetUserPassword(qemuAgentPtr mon, const char *user, const char *password, bool crypted); + +int qemuAgentGetUsers(qemuAgentPtr mon, virTypedParameterPtr *params, int *nparams, int *maxparams); diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c index 2f79986207..f2936a59f0 100644 --- a/tests/qemuagenttest.c +++ b/tests/qemuagenttest.c @@ -902,6 +902,173 @@ testQemuAgentGetInterfaces(const void *data) return ret; } +static const char testQemuAgentUsersResponse[] = + "{\"return\": " + " [" + " {\"user\": \"test\"," + " \"login-time\": 1561739203.584038" + " }," + " {\"user\": \"test2\"," + " \"login-time\": 1561739229.190697" + " }" + " ]" + "}"; + +static const char testQemuAgentUsersResponse2[] = + "{\"return\": " + " [" + " {\"user\": \"test\"," + " \"domain\": \"DOMAIN\"," + " \"login-time\": 1561739203.584038" + " }" + " ]" + "}"; + +static int getUserInfo(virTypedParameterPtr params, int nparams, size_t nth, + const char **username, const char **domain, + unsigned long long *logintime) +{ + char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; + + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, + "user.%zu.name", nth); + if (username && + virTypedParamsGetString(params, nparams, param_name, username) < 0) + return -1; + + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, + "user.%zu.domain", nth); + if (domain && + virTypedParamsGetString(params, nparams, param_name, domain) < 0) + return -1; + + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, + "user.%zu.login-time", nth); + if (logintime && + virTypedParamsGetULLong(params, nparams, param_name, logintime) < 0) + return -1; + + return 0; +} + +static int +testQemuAgentUsers(const void *data) +{ + virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; + qemuMonitorTestPtr test = qemuMonitorTestNewAgent(xmlopt); + virTypedParameterPtr params = NULL; + int nparams = 0; + int maxparams = 0; + int ret = -1; + const char *username = NULL; + const char *domain = NULL; + unsigned long long logintime = 0; + unsigned int count; + + if (!test) + return -1; + + if (qemuMonitorTestAddAgentSyncResponse(test) < 0) + goto cleanup; + + if (qemuMonitorTestAddItem(test, "guest-get-users", + testQemuAgentUsersResponse) < 0) + goto cleanup; + + /* get users */ + if (qemuAgentGetUsers(qemuMonitorTestGetAgent(test), + &params, &nparams, &maxparams) < 0) + goto cleanup; + + if (virTypedParamsGetUInt(params, nparams, "user.count", &count) < 0) + goto cleanup; + if (count != 2) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "Expected '2' users, got '%u'", count); + goto cleanup; + } + + getUserInfo(params, nparams, 0, &username, NULL, &logintime); + if (!username) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", "Missing username"); + goto cleanup; + } + if (STRNEQ(username, "test")) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "Expected user name 'test', got '%s'", username); + goto cleanup; + } + if (logintime != 1561739203584) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "Expected login time of '1561739203584', got '%llu'", + logintime); + goto cleanup; + } + + getUserInfo(params, nparams, 1, &username, NULL, &logintime); + if (STRNEQ(username, "test2")) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "Expected user name 'test2', got '%s'", username); + goto cleanup; + } + if (logintime != 1561739229190) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "Expected login time of '1561739229190', got '%llu'", + logintime); + goto cleanup; + } + + if (qemuMonitorTestAddAgentSyncResponse(test) < 0) + goto cleanup; + + if (qemuMonitorTestAddItem(test, "guest-get-users", + testQemuAgentUsersResponse2) < 0) + goto cleanup; + + virTypedParamsFree(params, nparams); + params = NULL; + nparams = 0; + maxparams = 0; + + /* get users with domain */ + if (qemuAgentGetUsers(qemuMonitorTestGetAgent(test), + &params, &nparams, &maxparams) < 0) + goto cleanup; + + if (virTypedParamsGetUInt(params, nparams, "user.count", &count) < 0) + goto cleanup; + if (count != 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "Expected '1' user, got '%u'", count); + goto cleanup; + } + + getUserInfo(params, nparams, 0, &username, &domain, &logintime); + if (STRNEQ(username, "test")) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "Expected user name 'test', got '%s'", username); + goto cleanup; + } + if (logintime != 1561739203584) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "Expected login time of '1561739203584', got '%llu'", + logintime); + goto cleanup; + } + if (STRNEQ(domain, "DOMAIN")) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "Expected domain 'DOMAIN', got '%s'", domain); + goto cleanup; + } + ret = 0; + + cleanup: + virTypedParamsFree(params, nparams); + qemuMonitorTestFree(test); + return ret; +} + + static int mymain(void) { @@ -931,6 +1098,7 @@ mymain(void) DO_TEST(CPU); DO_TEST(ArbitraryCommand); DO_TEST(GetInterfaces); + DO_TEST(Users); DO_TEST(Timeout); /* Timeout should always be called last */ -- 2.21.0

On 8/23/19 6:31 PM, Jonathon Jongsma wrote: We can use VIR_AUTOPTR() and drop the explicit virJSONValueFree() calls at the cleanup label which then in turn gets needless as we can 'return -1' directly instead of 'goto cleanup'. Very loooong line ;-) This function can be renamed to checkUserInfo() and it can check the values directly. It saves us couple of more lines. Michal

This function queries the guest operating system information and adds the returned information to an array of typed parameters with field names intended to be returned in virDomainGetGuestInfo(). Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- src/qemu/qemu_agent.c | 53 ++++++++++++++++++ src/qemu/qemu_agent.h | 1 + tests/qemuagenttest.c | 122 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 176 insertions(+) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 963a4b9359..0fcc3fdabd 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -2331,3 +2331,56 @@ qemuAgentGetUsers(qemuAgentPtr mon, virJSONValueFree(reply); return ret; } + +int +qemuAgentGetOSInfo(qemuAgentPtr mon, + virTypedParameterPtr *params, + int *nparams, + int *maxparams) +{ + int ret = -1; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + virJSONValuePtr data = NULL; + const char *result; + + if (!(cmd = qemuAgentMakeCommand("guest-get-osinfo", NULL))) + return -1; + + if (qemuAgentCommand(mon, cmd, &reply, true, + VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0) + goto cleanup; + + if (!(data = virJSONValueObjectGetObject(reply, "return"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest-get-osinfo reply was missing return data")); + goto cleanup; + } + +#define OSINFO_ADD_PARAM(agent_string_, param_string_) \ + do { \ + if ((result = virJSONValueObjectGetString(data, agent_string_))) { \ + if (virTypedParamsAddString(params, nparams, maxparams, \ + param_string_, result) < 0) { \ + goto cleanup; \ + } \ + } \ + } while (0) + OSINFO_ADD_PARAM("id", "os.id"); + OSINFO_ADD_PARAM("name", "os.name"); + OSINFO_ADD_PARAM("pretty-name", "os.pretty-name"); + OSINFO_ADD_PARAM("version", "os.version"); + OSINFO_ADD_PARAM("version-id", "os.version-id"); + OSINFO_ADD_PARAM("machine", "os.machine"); + OSINFO_ADD_PARAM("variant", "os.variant"); + OSINFO_ADD_PARAM("variant-id", "os.variant-id"); + OSINFO_ADD_PARAM("kernel-release", "os.kernel-release"); + OSINFO_ADD_PARAM("kernel-version", "os.kernel-version"); + + ret = 0; + + cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h index 05621b521a..ee019455e5 100644 --- a/src/qemu/qemu_agent.h +++ b/src/qemu/qemu_agent.h @@ -122,3 +122,4 @@ int qemuAgentSetUserPassword(qemuAgentPtr mon, bool crypted); int qemuAgentGetUsers(qemuAgentPtr mon, virTypedParameterPtr *params, int *nparams, int *maxparams); +int qemuAgentGetOSInfo(qemuAgentPtr mon, virTypedParameterPtr *params, int *nparams, int *maxparams); diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c index f2936a59f0..489b77d403 100644 --- a/tests/qemuagenttest.c +++ b/tests/qemuagenttest.c @@ -1068,6 +1068,127 @@ testQemuAgentUsers(const void *data) return ret; } +static const char testQemuAgentOSInfoResponse[] = + "{\"return\": " + " {\"name\":\"CentOS Linux\", " + " \"kernel-release\":\"3.10.0-862.14.4.el7.x86_64\", " + " \"version\":\"7 (Core)\", " + " \"pretty-name\":\"CentOS Linux 7 (Core)\", " + " \"version-id\":\"7\", " + " \"kernel-version\":\"#1 SMP Wed Sep 26 15:12:11 UTC 2018\", " + " \"machine\":\"x86_64\", " + " \"id\":\"centos\"} " + "}"; + +static const char testQemuAgentOSInfoResponse2[] = + "{\"return\": " + " {\"name\":\"Microsoft Windows\", " + " \"kernel-release\":\"7601\", " + " \"version\":\"Microsoft Windows 77\", " + " \"variant\":\"client\", " + " \"pretty-name\":\"Windows 7 Professional\", " + " \"version-id\":\"\", " + " \"variant-id\":\"client\", " + " \"kernel-version\":\"6.1\", " + " \"machine\":\"x86_64\", " + " \"id\":\"mswindows\"} " + "}"; + +static int +testQemuAgentOSInfo(const void *data) +{ + virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; + qemuMonitorTestPtr test = qemuMonitorTestNewAgent(xmlopt); + virTypedParameterPtr params = NULL; + int nparams = 0; + int maxparams = 0; + int ret = -1; + + if (!test) + return -1; + + if (qemuMonitorTestAddAgentSyncResponse(test) < 0) + goto cleanup; + + if (qemuMonitorTestAddItem(test, "guest-get-osinfo", + testQemuAgentOSInfoResponse) < 0) + goto cleanup; + + /* get osinfo */ + if (qemuAgentGetOSInfo(qemuMonitorTestGetAgent(test), + &params, &nparams, &maxparams) < 0) + goto cleanup; + + if (nparams != 8) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "Expected 8 params, got %d", nparams); + goto cleanup; + } +#define VALIDATE_PARAM(param_name_, expected_) \ + do { \ + const char *value_ = NULL; \ + if (virTypedParamsGetString(params, nparams, param_name_, &value_) < 0 || \ + value_ == NULL) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, "missing param '%s'", param_name_); \ + goto cleanup; \ + } \ + if (STRNEQ(value_, expected_)) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + "Expected name '%s', got '%s'", expected_, value_); \ + goto cleanup; \ + } \ + } while (0) + + VALIDATE_PARAM("os.id", "centos"); + VALIDATE_PARAM("os.name", "CentOS Linux"); + VALIDATE_PARAM("os.version", "7 (Core)"); + VALIDATE_PARAM("os.version-id", "7"); + VALIDATE_PARAM("os.pretty-name", "CentOS Linux 7 (Core)"); + VALIDATE_PARAM("os.kernel-release", "3.10.0-862.14.4.el7.x86_64"); + VALIDATE_PARAM("os.kernel-version", "#1 SMP Wed Sep 26 15:12:11 UTC 2018"); + VALIDATE_PARAM("os.machine", "x86_64"); + virTypedParamsFree(params, nparams); + params = NULL; + nparams = 0; + maxparams = 0; + + if (qemuMonitorTestAddAgentSyncResponse(test) < 0) + goto cleanup; + + if (qemuMonitorTestAddItem(test, "guest-get-osinfo", + testQemuAgentOSInfoResponse2) < 0) + goto cleanup; + + /* get users with domain */ + if (qemuAgentGetOSInfo(qemuMonitorTestGetAgent(test), + &params, &nparams, &maxparams) < 0) + goto cleanup; + + if (nparams != 10) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "Expected 10 params, got %d", nparams); + goto cleanup; + } + + VALIDATE_PARAM("os.id", "mswindows"); + VALIDATE_PARAM("os.name", "Microsoft Windows"); + VALIDATE_PARAM("os.pretty-name", "Windows 7 Professional"); + VALIDATE_PARAM("os.version", "Microsoft Windows 77"); + VALIDATE_PARAM("os.version-id", ""); + VALIDATE_PARAM("os.variant", "client"); + VALIDATE_PARAM("os.variant-id", "client"); + VALIDATE_PARAM("os.kernel-release", "7601"); + VALIDATE_PARAM("os.kernel-version", "6.1"); + VALIDATE_PARAM("os.machine", "x86_64"); + virTypedParamsFree(params, nparams); + + ret = 0; + + cleanup: + qemuMonitorTestFree(test); + return ret; +} + static int mymain(void) @@ -1099,6 +1220,7 @@ mymain(void) DO_TEST(ArbitraryCommand); DO_TEST(GetInterfaces); DO_TEST(Users); + DO_TEST(OSInfo); DO_TEST(Timeout); /* Timeout should always be called last */ -- 2.21.0

On 8/23/19 6:31 PM, Jonathon Jongsma wrote: Again, VIR_AUTOPTR() is your friend ;-) Long story short, VIR_AUTOFREE/VIR_AUTOPTR/VIR_AUTOUNREF()/... is our use of attribute(__cleanup__) which is a way of telling compiler to call a function once the associated variable goes out of scope. In general, the function can be anything, but we use it for automagic free/unref. This way, we can drop explicit calls to virXXXFree() if the variable is marked as VIR_AUTO*** and thus save up a few lines. Also, the code is more robust as there won't be any hidden path this forgets to 'goto cleanup' and thus causes a memleak. It takes some time to get used to it and we still haven't converted our code fully (as you realized by now for sure). Michal

On 8/23/19 1:31 PM, Jonathon Jongsma wrote: I spotted two missing indentations down there, but other than that, LGTM. Reviewed-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; Tested-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; "goto cleanup" should be indented 4 spaces after the preceeding 'if' "goto cleanup" should be indented 4 spaces after the preceeding 'if'

Since version 3.0, qemu has returned disk usage statistics in guest-get-fsinfo. And since 3.1, it has returned information about the disk serial number and device node of disks that are targeted by the filesystem. Unfortunately, the public API virDomainGetFSInfo() returns the filesystem info using a virDomainFSInfo struct, and due to API/ABI guaranteeds it cannot be extended. So this new information cannot easily be added to the public API. However, it is possible to add this new filesystem information to a new virDomainGetGuestInfo() API which will be based on typed parameters and is thus more extensible. In order to support these two use cases, I added an internal struct which the agent code uses to return all of the new data fields. This internal struct can be converted to the public struct at a cost of some extra memory allocation. In a following commit, this additional information will be used within virDomainGetGuestInfo(). Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- src/qemu/qemu_agent.c | 203 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 182 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index e986204162..8d54979f89 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1827,19 +1827,144 @@ qemuAgentSetTime(qemuAgentPtr mon, return ret; } +typedef struct _qemuAgentDiskInfo qemuAgentDiskInfo; +typedef qemuAgentDiskInfo *qemuAgentDiskInfoPtr; +struct _qemuAgentDiskInfo { + char *alias; + char *serial; + char *devnode; +}; + +typedef struct _qemuAgentFSInfo qemuAgentFSInfo; +typedef qemuAgentFSInfo *qemuAgentFSInfoPtr; +struct _qemuAgentFSInfo { + char *mountpoint; /* path to mount point */ + char *name; /* device name in the guest (e.g. "sda1") */ + char *fstype; /* filesystem type */ + long long total_bytes; + long long used_bytes; + size_t ndisks; + qemuAgentDiskInfoPtr *disks; +}; + +static void +qemuAgentDiskInfoFree(qemuAgentDiskInfoPtr info) +{ + if (!info) + return; + + VIR_FREE(info->serial); + VIR_FREE(info->alias); + VIR_FREE(info->devnode); + VIR_FREE(info); +} + +static void +qemuAgentFSInfoFree(qemuAgentFSInfoPtr info) +{ + size_t i; + + if (!info) + return; + + VIR_FREE(info->mountpoint); + VIR_FREE(info->name); + VIR_FREE(info->fstype); + + for (i = 0; i < info->ndisks; i++) + qemuAgentDiskInfoFree(info->disks[i]); + VIR_FREE(info->disks); + + VIR_FREE(info); +} + +static virDomainFSInfoPtr +qemuAgentFSInfoToPublic(qemuAgentFSInfoPtr agent) +{ + virDomainFSInfoPtr ret = NULL; + size_t n; + + if (VIR_ALLOC(ret) < 0) + return NULL; + if (VIR_STRDUP(ret->name, agent->name) < 0) + goto error; + if (VIR_STRDUP(ret->mountpoint, agent->mountpoint) < 0) + goto error; + if (VIR_STRDUP(ret->fstype, agent->fstype) < 0) + goto error; + + ret->ndevAlias = agent->ndisks; + + if (ret->ndevAlias == 0) + return ret; + + if (VIR_ALLOC_N(ret->devAlias, ret->ndevAlias) < 0) + goto error; + + for (n = 0; n < ret->ndevAlias; n++) { + if (VIR_STRDUP(ret->devAlias[n], agent->disks[n]->alias) < 0) + goto error; + } + + return ret; + + error: + virDomainFSInfoFree(ret); + return NULL; +} + +static int +qemuAgentGetFSInfoInternal(qemuAgentPtr mon, qemuAgentFSInfoPtr **info, + virDomainDefPtr vmdef); int qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info, virDomainDefPtr vmdef) +{ + int ret = -1; + qemuAgentFSInfoPtr *agentinfo = NULL; + virDomainFSInfoPtr *info_ret = NULL; + size_t i; + int nfs; + + nfs = qemuAgentGetFSInfoInternal(mon, &agentinfo, vmdef); + if (nfs < 0) + return ret; + if (VIR_ALLOC_N(info_ret, nfs) < 0) + goto cleanup; + + for (i = 0; i < nfs; i++) { + if (!(info_ret[i] = qemuAgentFSInfoToPublic(agentinfo[i]))) + goto cleanup; + } + + *info = info_ret; + info_ret = NULL; + ret = nfs; + + cleanup: + for (i = 0; i < nfs; i++) { + qemuAgentFSInfoFree(agentinfo[i]); + /* if there was an error, free any memory we've allocated for the + * return value */ + if (info_ret) + virDomainFSInfoFree(info_ret[i]); + } + return ret; +} + + +static int +qemuAgentGetFSInfoInternal(qemuAgentPtr mon, qemuAgentFSInfoPtr **info, + virDomainDefPtr vmdef) { size_t i, j, k; int ret = -1; - size_t ndata = 0, ndisk; - char **alias; + size_t ndata = 0; virJSONValuePtr cmd; virJSONValuePtr reply = NULL; virJSONValuePtr data; - virDomainFSInfoPtr *info_ret = NULL; + qemuAgentFSInfoPtr *info_ret = NULL; virPCIDeviceAddress pci_address; const char *result = NULL; @@ -1915,6 +2040,33 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info, if (VIR_STRDUP(info_ret[i]->fstype, result) < 0) goto cleanup; + + /* 'used-bytes' and 'total-bytes' were added in qemu-ga 3.0 */ + unsigned long long bytes_val; + if (virJSONValueObjectHasKey(entry, "used-bytes")) { + if (virJSONValueObjectGetNumberUlong(entry, "used-bytes", + &bytes_val) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Error getting 'used-bytes' in reply of guest-get-fsinfo")); + goto cleanup; + } + info_ret[i]->used_bytes = bytes_val; + } else { + info_ret[i]->used_bytes = -1; + } + + if (virJSONValueObjectHasKey(entry, "total-bytes")) { + if (virJSONValueObjectGetNumberUlong(entry, "total-bytes", + &bytes_val) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Error getting 'total-bytes' in reply of guest-get-fsinfo")); + goto cleanup; + } + info_ret[i]->total_bytes = bytes_val; + } else { + info_ret[i]->total_bytes = -1; + } + if (!(entry = virJSONValueObjectGet(entry, "disk"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("'disk' missing in reply of guest-get-fsinfo")); @@ -1927,31 +2079,45 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info, goto cleanup; } - ndisk = virJSONValueArraySize(entry); - if (ndisk == 0) + info_ret[i]->ndisks = virJSONValueArraySize(entry); + if (info_ret[i]->ndisks == 0) continue; - if (VIR_ALLOC_N(info_ret[i]->devAlias, ndisk) < 0) + if (VIR_ALLOC_N(info_ret[i]->disks, info_ret[i]->ndisks) < 0) goto cleanup; - alias = info_ret[i]->devAlias; - info_ret[i]->ndevAlias = 0; - for (j = 0; j < ndisk; j++) { - virJSONValuePtr disk = virJSONValueArrayGet(entry, j); + for (j = 0; j < info_ret[i]->ndisks; j++) { + virJSONValuePtr jsondisk = virJSONValueArrayGet(entry, j); virJSONValuePtr pci; int diskaddr[3], pciaddr[4]; const char *diskaddr_comp[] = {"bus", "target", "unit"}; const char *pciaddr_comp[] = {"domain", "bus", "slot", "function"}; + const char *val; virDomainDiskDefPtr diskDef; - if (!disk) { + if (VIR_ALLOC(info_ret[i]->disks[j]) < 0) + goto cleanup; + + qemuAgentDiskInfoPtr disk = info_ret[i]->disks[j]; + + if (!jsondisk) { virReportError(VIR_ERR_INTERNAL_ERROR, _("array element '%zd' of '%zd' missing in " "guest-get-fsinfo 'disk' data"), - j, ndisk); + j, info_ret[i]->ndisks); goto cleanup; } - if (!(pci = virJSONValueObjectGet(disk, "pci-controller"))) { + if ((val = virJSONValueObjectGetString(jsondisk, "serial"))) { + if (VIR_STRDUP(disk->serial, val) < 0) + goto cleanup; + } + + if ((val = virJSONValueObjectGetString(jsondisk, "dev"))) { + if (VIR_STRDUP(disk->devnode, val) < 0) + goto cleanup; + } + + if (!(pci = virJSONValueObjectGet(jsondisk, "pci-controller"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("'pci-controller' missing in guest-get-fsinfo " "'disk' data")); @@ -1960,7 +2126,7 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info, for (k = 0; k < 3; k++) { if (virJSONValueObjectGetNumberInt( - disk, diskaddr_comp[k], &diskaddr[k]) < 0) { + jsondisk, diskaddr_comp[k], &diskaddr[k]) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("'%s' missing in guest-get-fsinfo " "'disk' data"), diskaddr_comp[k]); @@ -1986,13 +2152,8 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info, diskaddr[0], diskaddr[1], diskaddr[2]))) continue; - if (VIR_STRDUP(*alias, diskDef->dst) < 0) + if (VIR_STRDUP(disk->alias, diskDef->dst) < 0) goto cleanup; - - if (*alias) { - alias++; - info_ret[i]->ndevAlias++; - } } } @@ -2003,7 +2164,7 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info, cleanup: if (info_ret) { for (i = 0; i < ndata; i++) - virDomainFSInfoFree(info_ret[i]); + qemuAgentFSInfoFree(info_ret[i]); VIR_FREE(info_ret); } virJSONValueFree(cmd); -- 2.21.0

On 8/23/19 6:31 PM, Jonathon Jongsma wrote: This is not safe. You set ret->ndevAlias before allocating ret->devAlias. Just imagine this VIR_ALLOC_N() fails so that the control jumps over to error label where virDomainFSInfoFree() is called which sees nonzero info->ndevAlias and thus enters its loop and derefs info->devAlias which is NULL. No need for this prototype if you just switch the order. Dont' forget to free @info_ret itself. Ugrh. Gross. Not your fault, but I'm moving this into a separate function, because this has grown over bearable limit. Again, not your fault, but this looks very ugly to me. Will fix it though. Michal

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(a)redhat.com&gt; --- 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; + 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); -- 2.21.0

Iimplements the new guest information API by querying requested information via the guest agent. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 110 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 110 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1b051a9424..446266e66b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -23190,6 +23190,115 @@ qemuDomainGetLaunchSecurityInfo(virDomainPtr domain, return ret; } +static unsigned int supportedGuestInfoTypes = + VIR_DOMAIN_GUEST_INFO_USERS | + VIR_DOMAIN_GUEST_INFO_OS | + VIR_DOMAIN_GUEST_INFO_TIMEZONE | + VIR_DOMAIN_GUEST_INFO_HOSTNAME | + VIR_DOMAIN_GUEST_INFO_FILESYSTEM; + +static void +qemuDomainGetGuestInfoCheckSupport(unsigned int *types) +{ + if (*types == 0) + *types = supportedGuestInfoTypes; + + *types = *types & supportedGuestInfoTypes; +} + +static int +qemuDomainGetGuestInfo(virDomainPtr dom, + unsigned int types, + virTypedParameterPtr *params, + int *nparams, + unsigned int flags) +{ + virQEMUDriverPtr driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + qemuAgentPtr agent; + int ret = -1; + int rv = -1; + int maxparams = 0; + char *hostname = NULL; + virDomainDefPtr def = NULL; + virCapsPtr caps = NULL; + unsigned int supportedTypes = types; + + virCheckFlags(0, ret); + qemuDomainGetGuestInfoCheckSupport(&supportedTypes); + + if (!(vm = qemuDomObjFromDomain(dom))) + goto cleanup; + + if (virDomainGetGuestInfoEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_QUERY) < 0) + goto cleanup; + + if (!qemuDomainAgentAvailable(vm, true)) + goto endjob; + + agent = qemuDomainObjEnterAgent(vm); + + /* Although the libvirt qemu driver supports all of these guest info types, + * some guest agents might be too old to support these commands. If these + * info categories were explicitly requested (i.e. 'types' is non-zero), + * abort and report an error on any failures, otherwise continue and return + * as much info as is supported by the guest agent. */ + if (supportedTypes & VIR_DOMAIN_GUEST_INFO_USERS) { + if (qemuAgentGetUsers(agent, params, nparams, &maxparams) < 0 && + types != 0) + goto exitagent; + } + if (supportedTypes & VIR_DOMAIN_GUEST_INFO_OS) { + if (qemuAgentGetOSInfo(agent, params, nparams, &maxparams) < 0 + && types != 0) + goto exitagent; + } + if (supportedTypes & VIR_DOMAIN_GUEST_INFO_TIMEZONE) { + if (qemuAgentGetTimezone(agent, params, nparams, &maxparams) < 0 + && types != 0) + goto exitagent; + } + if (supportedTypes & VIR_DOMAIN_GUEST_INFO_HOSTNAME) { + if (qemuAgentGetHostname(agent, &hostname) < 0) { + if (types != 0) + goto exitagent; + } else { + if (virTypedParamsAddString(params, nparams, &maxparams, "hostname", + hostname) < 0) + goto exitagent; + } + } + if (supportedTypes & VIR_DOMAIN_GUEST_INFO_FILESYSTEM) { + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto exitagent; + + if (!(def = virDomainDefCopy(vm->def, caps, driver->xmlopt, NULL, false))) + goto exitagent; + + if (qemuAgentGetFSInfoParams(agent, params, nparams, &maxparams, def) < 0 && + types != 0) + goto exitagent; + } + + rv = 0; + + exitagent: + qemuDomainObjExitAgent(vm, agent); + + endjob: + qemuDomainObjEndAgentJob(vm); + + cleanup: + virDomainObjEndAPI(&vm); + virDomainDefFree(def); + virObjectUnref(caps); + VIR_FREE(hostname); + return rv; +} + static virHypervisorDriver qemuHypervisorDriver = { .name = QEMU_DRIVER_NAME, .connectURIProbe = qemuConnectURIProbe, @@ -23425,6 +23534,7 @@ static virHypervisorDriver qemuHypervisorDriver = { .domainCheckpointLookupByName = qemuDomainCheckpointLookupByName, /* 5.6.0 */ .domainCheckpointGetParent = qemuDomainCheckpointGetParent, /* 5.6.0 */ .domainCheckpointDelete = qemuDomainCheckpointDelete, /* 5.6.0 */ + .domainGetGuestInfo = qemuDomainGetGuestInfo, /* 5.6.0 */ }; -- 2.21.0

On 8/23/19 6:31 PM, Jonathon Jongsma wrote: This can be const since it's never modified. Huh, this @rv and @ret are used a bit misleadinly :-D VIR_AUTOFREE() This might not do what you think it does. qemuAgentGetUsers() (and the rest of qemuAgentXXX() for that matter) can fail because of variety of reasons. We want to continue if and only if command was not found and halt in all other cases. But I'll leave this as is for now and probably fix this in a follow up patch. Unless you want to work on that. If you do, then you might want to take a look how we do that in qemu monitor code: qemuMonitorJSONHasError(reply, "CommandNotFound")) No need to create the copy of the domain def (which is really expensive). s/6/7/ Michal

On 8/23/19 1:31 PM, Jonathon Jongsma wrote: Reviewed-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; Tested-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt;