[libvirt] [PATCH 3/5] qemu: Implement the qemu driver for virDomainGetFSInfo
John Ferlan
jferlan at redhat.com
Thu Oct 23 13:28:05 UTC 2014
On 09/30/2014 08:20 PM, Tomoki Sekiyama wrote:
> Get mounted filesystems list, which contains hardware info of disks and its
> controllers, from QEMU guest agent 2.2+. Then, convert the hardware info
> to corresponding device aliases for the disks.
>
> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama at hds.com>
> ---
> src/conf/domain_conf.c | 71 ++++++++++++++++++++
> src/conf/domain_conf.h | 6 ++
> src/libvirt_private.syms | 1
> src/qemu/qemu_agent.c | 165 ++++++++++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_agent.h | 2 +
> src/qemu/qemu_driver.c | 48 +++++++++++++
> 6 files changed, 293 insertions(+)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index b114737..f5c5e0c 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -10838,6 +10838,60 @@ virDomainHostdevFind(virDomainDefPtr def,
> return *found ? i : -1;
> }
>
> +static bool
> +virDomainDiskControllerMatch(int controller_type, int disk_bus)
> +{
> + if (controller_type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI &&
> + disk_bus == VIR_DOMAIN_DISK_BUS_SCSI)
> + return true;
> +
> + if (controller_type == VIR_DOMAIN_CONTROLLER_TYPE_FDC &&
> + disk_bus == VIR_DOMAIN_DISK_BUS_FDC)
> + return true;
> +
> + if (controller_type == VIR_DOMAIN_CONTROLLER_TYPE_IDE &&
> + disk_bus == VIR_DOMAIN_DISK_BUS_IDE)
> + return true;
> +
> + if (controller_type == VIR_DOMAIN_CONTROLLER_TYPE_SATA &&
> + disk_bus == VIR_DOMAIN_DISK_BUS_SATA)
> + return true;
> +
> + return false;
> +}
> +
> +int
> +virDomainDiskIndexByAddress(virDomainDefPtr def,
> + virDevicePCIAddressPtr pci_address,
> + unsigned int bus, unsigned int target,
> + unsigned int unit)
> +{
> + virDomainDiskDefPtr vdisk;
> + virDomainControllerDefPtr controller = NULL;
> + size_t i;
> + int cidx;
> +
> + if ((cidx = virDomainControllerFindByPCIAddress(def, pci_address)) >= 0)
> + controller = def->controllers[cidx];
> +
> + for (i = 0; i < def->ndisks; i++) {
> + vdisk = def->disks[i];
> + if (vdisk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
> + virDevicePCIAddressEqual(&vdisk->info.addr.pci, pci_address))
> + return i;
> + if (vdisk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) {
> + virDomainDeviceDriveAddressPtr drive = &vdisk->info.addr.drive;
> + if (controller &&
> + virDomainDiskControllerMatch(controller->type, vdisk->bus) &&
> + drive->controller == controller->idx &&
> + drive->bus == bus && drive->target == target &&
> + drive->unit == unit)
> + return i;
> + }
> + }
> + return -1;
> +}
> +
> int
> virDomainDiskIndexByName(virDomainDefPtr def, const char *name,
> bool allow_ambiguous)
> @@ -11127,6 +11181,23 @@ virDomainControllerFind(virDomainDefPtr def,
> return -1;
> }
>
> +int
> +virDomainControllerFindByPCIAddress(virDomainDefPtr def,
> + virDevicePCIAddressPtr addr)
> +{
> + size_t i;
> +
> + for (i = 0; i < def->ncontrollers; i++) {
> + virDomainDeviceInfoPtr info = &def->controllers[i]->info;
> +
> + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
> + virDevicePCIAddressEqual(&info->addr.pci, addr))
> + return i;
> + }
> +
> + return -1;
> +}
> +
> virDomainControllerDefPtr
> virDomainControllerRemove(virDomainDefPtr def, size_t i)
> {
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index ea201b3..5755eae 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2426,6 +2426,10 @@ int virDomainEmulatorPinDel(virDomainDefPtr def);
>
> void virDomainRNGDefFree(virDomainRNGDefPtr def);
>
> +int virDomainDiskIndexByAddress(virDomainDefPtr def,
> + virDevicePCIAddressPtr pci_controller,
> + unsigned int bus, unsigned int target,
> + unsigned int unit);
> int virDomainDiskIndexByName(virDomainDefPtr def, const char *name,
> bool allow_ambiguous);
> const char *virDomainDiskPathByName(virDomainDefPtr, const char *name);
> @@ -2493,6 +2497,8 @@ int virDomainControllerInsert(virDomainDefPtr def,
> void virDomainControllerInsertPreAlloced(virDomainDefPtr def,
> virDomainControllerDefPtr controller);
> int virDomainControllerFind(virDomainDefPtr def, int type, int idx);
> +int virDomainControllerFindByPCIAddress(virDomainDefPtr def,
> + virDevicePCIAddressPtr addr);
> virDomainControllerDefPtr virDomainControllerRemove(virDomainDefPtr def, size_t i);
>
> int virDomainLeaseIndex(virDomainDefPtr def,
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 2019ef5..db55abf 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -234,6 +234,7 @@ virDomainDiskGetDriver;
> virDomainDiskGetFormat;
> virDomainDiskGetSource;
> virDomainDiskGetType;
> +virDomainDiskIndexByAddress;
> virDomainDiskIndexByName;
> virDomainDiskInsert;
> virDomainDiskInsertPreAlloced;
> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
> index fe38f6d..5862c24 100644
> --- a/src/qemu/qemu_agent.c
> +++ b/src/qemu/qemu_agent.c
> @@ -1780,3 +1780,168 @@ qemuAgentSetTime(qemuAgentPtr mon,
> virJSONValueFree(reply);
> return ret;
> }
> +
> +
> +int
> +qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info,
> + virDomainDefPtr vmdef)
> +{
> + size_t i, j, k;
> + int ret = -1;
> + int ndata = 0, ndisk;
> + char **alias;
> + virJSONValuePtr cmd;
> + virJSONValuePtr reply = NULL;
> + virJSONValuePtr data;
> + virDomainFSInfoPtr *info_ret = NULL;
> + virDevicePCIAddress pci_address;
> +
> + cmd = qemuAgentMakeCommand("guest-get-fsinfo", NULL);
> + if (!cmd)
> + return ret;
> +
> + if (qemuAgentCommand(mon, cmd, &reply, true,
> + VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
> + goto cleanup;
Mainly curious - since it's stated that this returns data from qemu
guest agent 2.2+ - what happens when run on something earlier?
> +
> + if (!(data = virJSONValueObjectGet(reply, "return"))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("guest-get-fsinfo reply was missing return data"));
> + goto cleanup;
> + }
> +
> + if (data->type != VIR_JSON_TYPE_ARRAY) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("guest-get-fsinfo return data was not an array"));
> + goto cleanup;
> + }
> +
> + ndata = virJSONValueArraySize(data);
Is a zero return valid here?
> +
> + if (VIR_ALLOC_N(info_ret, ndata + 1) < 0)
Does any code actually make use of this empty entry? Since you return
the count callers should "honor" that instead (and it seems all your
callers do so). Be sure to adjust your comments everywhere accordingly,
such as patch 1 in the API comment.
> + goto cleanup;
> +
> + for (i = 0; i < ndata; i++) {
> + /* Reverse the order to arrange in mount order */
> + virJSONValuePtr entry = virJSONValueArrayGet(data, ndata - 1 - i);
> +
> + if (!entry) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("array element missing in guest-get-fsinfo data"));
> + goto cleanup;
> + }
> +
> + if (VIR_ALLOC(info_ret[i]) < 0)
> + goto cleanup;
> +
> + if (VIR_STRDUP(info_ret[i]->mountpoint,
> + virJSONValueObjectGetString(entry, "mountpoint")) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("guest-get-fsinfo entry was missing mountpoint "
> + "data"));
For fields that are expected put single quotes around them. To be
consistent with what I saw for guest-get-vcpus, consider:
"'mountpoint' missing in reply of guest-get-fsinfo"
> + goto cleanup;
> + }
> +
> + if (VIR_STRDUP(info_ret[i]->name,
> + virJSONValueObjectGetString(entry, "name")) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("guest-get-fsinfo entry was missing name data"));
likewise:
"'name' missing in reply of guest-get-fsinfo"
> + goto cleanup;
> + }
> +
> + if (VIR_STRDUP(info_ret[i]->type,
> + virJSONValueObjectGetString(entry, "type")) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("guest-get-fsinfo entry was missing type data"));
likewise:
"'type' missing in reply of guest-get-fsinfo"
> + goto cleanup;
> + }
> +
> + if (!(entry = virJSONValueObjectGet(entry, "disk"))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("guest-get-fsinfo entry was missing disk data"));
likewise:
"'disk' missing in reply of guest-get-fsinfo"
> + goto cleanup;
> + }
> +
> + if (entry->type != VIR_JSON_TYPE_ARRAY) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("guest-get-fsinfo disk data was not an array"));
s/disk/'disk'/
> + goto cleanup;
> + }
> +
> + ndisk = virJSONValueArraySize(entry);
> + if (!ndisk)
> + continue;
> + if (VIR_ALLOC_N(info_ret[i]->devAlias, ndisk + 1) < 0)
> + goto cleanup;
> +
> + alias = info_ret[i]->devAlias;
> + for (j = 0; j < ndisk; j++) {
> + virJSONValuePtr disk = virJSONValueArrayGet(entry, j);
> + virJSONValuePtr pci;
> + int diskaddr[3], pciaddr[4], idx;
> + const char *diskaddr_comp[] = {"bus", "target", "unit"};
> + const char *pciaddr_comp[] = {"domain", "bus", "slot", "function"};
> +
> + if (!disk) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("array element missing in guest-get-fsinfo "
> + "disk data"));
similar to earlier : s/disk/'disk'/
You may want to consider adding '%d' (j) and 'ndisk' to the output as
well as in "array elem 'j' of 'ndisk'"... Whether that'd be useful or
not if something go wrong.
> + goto cleanup;
> + }
> +
> + if (!(pci = virJSONValueObjectGet(disk, "pci-controller"))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("guest-get-fsinfo disk data was missing "
> + "pci-controller"));
likewise:
"'pci-controller' missing in reply of guest-get-fsinfo"
> + goto cleanup;
> + }
> +
> + for (k = 0; k < 3; k++) {
> + if (virJSONValueObjectGetNumberInt(
> + disk, diskaddr_comp[k], &diskaddr[k]) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("guest-get-fsinfo disk data was missing "
> + "%s"), diskaddr_comp[k]);
likewise:
"'%s' missing in reply of guest-get-fsinfo 'disk' address data"
> + goto cleanup;
> + }
> + }
> + for (k = 0; k < 4; k++) {
> + if (virJSONValueObjectGetNumberInt(
> + pci, pciaddr_comp[k], &pciaddr[k]) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("guest-get-fsinfo pci-address data was "
> + "missing %s"), pciaddr_comp[k]);
likewise:
"'%s' missing in reply of guest-get-fsinfo 'pci-address' data"
> + goto cleanup;
> + }
> + }
> +
> + pci_address.domain = pciaddr[0];
> + pci_address.bus = pciaddr[1];
> + pci_address.slot = pciaddr[2];
> + pci_address.function = pciaddr[3];
> + if ((idx = virDomainDiskIndexByAddress(
> + vmdef, &pci_address,
> + diskaddr[0], diskaddr[1], diskaddr[2])) < 0)
> + continue;
> +
> + if (VIR_STRDUP(*alias, vmdef->disks[idx]->dst) < 0)
> + goto cleanup;
> +
> + if (*alias)
> + alias++;
> + }
> + }
> +
> + *info = info_ret;
The "somewhat" standard way to avoid the unwanted free in cleanup is:
info_ret = NULL;
That also makes it so Coverity doesn't get confused over the two
conditions later on...
> + ret = ndata;
> +
> + cleanup:
> + if (ret < 0 && info_ret) {
Having two conditions confuses Coverity because it tries "both" paths
and although in the failure case, ret == -1 and we'd go thru here,
Coverity will generate the false positive and assume we can get here
with ret >= 0, so if we do the info_ret = NULL trick above, then we
don't need to check (ret < 0 &&), thus it just becomes:
if (info_ret)
> + for (i = 0; i < ndata; i++)
> + virDomainFSInfoFree(info_ret[i]);
> + VIR_FREE(info_ret);
> + }
> + virJSONValueFree(cmd);
> + virJSONValueFree(reply);
> + return ret;
> +}
> diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h
> index 6cd6b49..c983828 100644
> --- a/src/qemu/qemu_agent.h
> +++ b/src/qemu/qemu_agent.h
> @@ -73,6 +73,8 @@ int qemuAgentShutdown(qemuAgentPtr mon,
> int qemuAgentFSFreeze(qemuAgentPtr mon,
> const char **mountpoints, unsigned int nmountpoints);
> int qemuAgentFSThaw(qemuAgentPtr mon);
> +int qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info,
> + virDomainDefPtr vmdef);
>
> int qemuAgentSuspend(qemuAgentPtr mon,
> unsigned int target);
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index e873d45..68eb4b3 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -18211,6 +18211,53 @@ qemuNodeAllocPages(virConnectPtr conn,
> }
>
>
> +static int
> +qemuDomainGetFSInfo(virDomainPtr dom,
> + virDomainFSInfoPtr **info,
> + unsigned int flags)
> +{
> + virQEMUDriverPtr driver = dom->conn->privateData;
> + qemuDomainObjPrivatePtr priv;
> + virDomainObjPtr vm;
> + int ret = -1;
> +
> + virCheckFlags(0, ret);
> +
> + if (!(vm = qemuDomObjFromDomain(dom)))
> + return ret;
> +
> + if (virDomainGetFSInfoEnsureACL(dom->conn, vm->def) < 0)
> + goto cleanup;
> +
> + priv = vm->privateData;
> +
> + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
This is a QEMU_JOB_QUERY
John
> + goto cleanup;
> +
> + if (!virDomainObjIsActive(vm)) {
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + "%s", _("domain is not running"));
> + goto endjob;
> + }
> +
> + if (!qemuDomainAgentAvailable(priv, true))
> + goto endjob;
> +
> + qemuDomainObjEnterAgent(vm);
> + ret = qemuAgentGetFSInfo(priv->agent, info, vm->def);
> + qemuDomainObjExitAgent(vm);
> +
> + endjob:
> + if (!qemuDomainObjEndJob(driver, vm))
> + vm = NULL;
> +
> + cleanup:
> + if (vm)
> + virObjectUnlock(vm);
> + return ret;
> +}
> +
> +
> static virDriver qemuDriver = {
> .no = VIR_DRV_QEMU,
> .name = QEMU_DRIVER_NAME,
> @@ -18411,6 +18458,7 @@ static virDriver qemuDriver = {
> .connectGetDomainCapabilities = qemuConnectGetDomainCapabilities, /* 1.2.7 */
> .connectGetAllDomainStats = qemuConnectGetAllDomainStats, /* 1.2.8 */
> .nodeAllocPages = qemuNodeAllocPages, /* 1.2.9 */
> + .domainGetFSInfo = qemuDomainGetFSInfo, /* 1.2.10 */
> };
>
>
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
More information about the libvir-list
mailing list