[libvirt] [PATCH v3 6/9] qemu: add support for new fields in FSInfo
Michal Privoznik
mprivozn at redhat.com
Mon Aug 26 15:29:27 UTC 2019
On 8/23/19 6:31 PM, Jonathon Jongsma wrote:
> 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 at redhat.com>
> ---
> 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;
> +
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.
> + 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);
No need for this prototype if you just switch the order.
> 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]);
Dont' forget to free @info_ret itself.
> + }
> + 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++) {
Ugrh. Gross. Not your fault, but I'm moving this into a separate
function, because this has grown over bearable limit.
> + 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"};
Again, not your fault, but this looks very ugly to me. Will fix it though.
Michal
More information about the libvir-list
mailing list