[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2 3/5] qemu: Implement the qemu driver for virDomainGetFSInfo




On 11/17/2014 06:27 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 hds com>
> ---
>  src/conf/domain_conf.c   |   71 ++++++++++++++++++
>  src/conf/domain_conf.h   |    6 ++
>  src/libvirt_private.syms |    1 
>  src/qemu/qemu_agent.c    |  178 ++++++++++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_agent.h    |    2 +
>  src/qemu/qemu_driver.c   |   48 ++++++++++++
>  6 files changed, 306 insertions(+)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 5f4b9f6..5972d7a 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -11114,6 +11114,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)
> @@ -11403,6 +11457,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 530a3ca..6164588 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2461,6 +2461,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);
> @@ -2529,6 +2533,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 0864618..16d4311 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 8df1330..f6432cc 100644
> --- a/src/qemu/qemu_agent.c
> +++ b/src/qemu/qemu_agent.c
> @@ -1777,3 +1777,181 @@ 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;
> +
> +    if (!(data = virJSONValueObjectGet(reply, "return"))) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("'%s' missing in reply of guest-get-fsinfo"),
> +                           "return");

Not exactly what I had in mind, but I'll fix these... There's also 
an indent issue here (cut-n-paste from some other error message I assume)

There was also a check-syntax error or two with those multiline messages...

> +        goto cleanup;
> +    }
> +
> +    if (data->type != VIR_JSON_TYPE_ARRAY) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("guest-get-fsinfo '%s' data was not an array"),
> +                       "return");
> +        goto cleanup;
> +    }
> +
> +    ndata = virJSONValueArraySize(data);
> +    if (!ndata) {
> +        ret = 0;
> +        goto cleanup;
> +    }
> +    if (VIR_ALLOC_N(info_ret, ndata) < 0)
> +        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,
> +                           _("array element '%zd' of '%d' missing in"
> +                             "guest-get-fsinfo '%s' data"),
> +                           i, ndata, "return");
> +            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' missing in reply of guest-get-fsinfo"),
> +                           "mountpoint");
> +            goto cleanup;
> +        }
> +
> +        if (VIR_STRDUP(info_ret[i]->name,
> +                       virJSONValueObjectGetString(entry, "name")) < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("'%s' missing in reply of guest-get-fsinfo"),
> +                           "name");
> +            goto cleanup;
> +        }
> +
> +        if (VIR_STRDUP(info_ret[i]->type,
> +                       virJSONValueObjectGetString(entry, "type")) < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("'%s' missing in reply of guest-get-fsinfo"),
> +                           "type");
> +            goto cleanup;
> +        }
> +
> +        if (!(entry = virJSONValueObjectGet(entry, "disk"))) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("'%s' missing in reply of guest-get-fsinfo"),
> +                           "disk");
> +            goto cleanup;
> +        }
> +
> +        if (entry->type != VIR_JSON_TYPE_ARRAY) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("guest-get-fsinfo '%s' data was not an array"),
> +                           "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,
> +                               _("array element '%zd' of '%d' missing in"
> +                                 "guest-get-fsinfo '%s' data"),
> +                               j, ndisk, "disk");
> +                goto cleanup;
> +            }
> +
> +            if (!(pci = virJSONValueObjectGet(disk, "pci-controller"))) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("'%s' missing in guest-get-fsinfo "
> +                                 "'disk' data"), "pci-controller");
> +                goto cleanup;
> +            }
> +
> +            for (k = 0; k < 3; k++) {
> +                if (virJSONValueObjectGetNumberInt(
> +                        disk, diskaddr_comp[k], &diskaddr[k]) < 0) {
> +                    virReportError(VIR_ERR_INTERNAL_ERROR,
> +                                   _("'%s' missing in guest-get-fsinfo "
> +                                     "'disk' data"), diskaddr_comp[k]);
> +                    goto cleanup;
> +                }
> +            }
> +            for (k = 0; k < 4; k++) {
> +                if (virJSONValueObjectGetNumberInt(
> +                        pci, pciaddr_comp[k], &pciaddr[k]) < 0) {
> +                    virReportError(VIR_ERR_INTERNAL_ERROR,
> +                                   _("'%s' missing in guest-get-fsinfo "
> +                                     "'pci-address' data"), pciaddr_comp[k]);
> +                    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;
> +    info_ret = NULL;
> +    ret = ndata;
> +
> + cleanup:
> +    if (info_ret) {
> +        for (i = 0; i < ndata; i++)
> +            virDomainFSInfoFree(info_ret[i]);
> +        VIR_FREE(info_ret);
> +    }
> +    virJSONValueFree(cmd);
> +    virJSONValueFree(reply);
> +    return ret;
> +}

I'll squash the following in (hopefully I got the cut-n-paste right):

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index f6432cc..dcbeee8 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -1802,16 +1802,15 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr 
         goto cleanup;
 
     if (!(data = virJSONValueObjectGet(reply, "return"))) {
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("'%s' missing in reply of guest-get-fsinfo"),
-                           "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,
-                       _("guest-get-fsinfo '%s' data was not an array"),
-                       "return");
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("guest-get-fsinfo return information was not "
+                         "an array"));
         goto cleanup;
     }
 
@@ -1830,8 +1829,8 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **
         if (!entry) {
             virReportError(VIR_ERR_INTERNAL_ERROR,

-                           _("array element '%zd' of '%d' missing in"
-                             "guest-get-fsinfo '%s' data"),
-                           i, ndata, "return");
+                           _("array element '%zd' of '%d' missing in "
+                             "guest-get-fsinfo return data"),
+                           i, ndata);
             goto cleanup;
         }
 
@@ -1840,39 +1839,35 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr 
 
         if (VIR_STRDUP(info_ret[i]->mountpoint,
                        virJSONValueObjectGetString(entry, "mountpoint")) < 0) {
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("'%s' missing in reply of guest-get-fsinfo"),
-                           "mountpoint");
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("'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' missing in reply of guest-get-fsinfo"),
-                           "name");
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("'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' missing in reply of guest-get-fsinfo"),
-                           "type");
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("'type' missing in reply of guest-get-fsinfo"));
             goto cleanup;
         }
 
         if (!(entry = virJSONValueObjectGet(entry, "disk"))) {
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("'%s' missing in reply of guest-get-fsinfo"),
-                           "disk");
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("'disk' missing in reply of guest-get-fsinfo"));
             goto cleanup;
         }
 
         if (entry->type != VIR_JSON_TYPE_ARRAY) {
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("guest-get-fsinfo '%s' data was not an array"),
-                           "disk");
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("guest-get-fsinfo 'disk' data was not an array"));
             goto cleanup;
         }
 
@@ -1893,15 +1888,15 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr 
             if (!disk) {
                 virReportError(VIR_ERR_INTERNAL_ERROR,
-                               _("array element '%zd' of '%d' missing in"
-                                 "guest-get-fsinfo '%s' data"),
-                               j, ndisk, "disk");
+                               _("array element '%zd' of '%d' missing in "
+                                 "guest-get-fsinfo 'disk' data"),
+                               j, ndisk);
                 goto cleanup;
             }
 
             if (!(pci = virJSONValueObjectGet(disk, "pci-controller"))) {
-                virReportError(VIR_ERR_INTERNAL_ERROR,
-                               _("'%s' missing in guest-get-fsinfo "
-                                 "'disk' data"), "pci-controller");
+                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                               _("'pci-controller' missing in guest-get-fsinfo 
+                                 "'disk' data"));
                 goto cleanup;
             }
 

> 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 a84fd47..f655302 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -18767,6 +18767,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)
> +        goto cleanup;

Again - this is a QUERY not a MODIFY as far as I understand things, correct?

remote_protocol.x has:

+    /**
+     * @generate: none
+     * @acl: domain:read
+     */
+    REMOTE_PROC_DOMAIN_GET_FSINFO = 348
 
Although everything that happens here is not my specialty, so hopefully
someone else reads this review and can comment...


> +
> +    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 virHypervisorDriver qemuDriver = {
>      .no = VIR_DRV_QEMU,
>      .name = QEMU_DRIVER_NAME,
> @@ -18967,6 +19014,7 @@ static virHypervisorDriver qemuDriver = {
>      .connectGetDomainCapabilities = qemuConnectGetDomainCapabilities, /* 1.2.7 */
>      .connectGetAllDomainStats = qemuConnectGetAllDomainStats, /* 1.2.8 */
>      .nodeAllocPages = qemuNodeAllocPages, /* 1.2.9 */
> +    .domainGetFSInfo = qemuDomainGetFSInfo, /* 1.2.11 */
>  };
>  
>  
> 

I'll squash the following in:
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9f7d951..2263b59 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18787,7 +18787,7 @@ qemuDomainGetFSInfo(virDomainPtr dom,
 
     priv = vm->privateData;
 
-    if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
+    if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
         goto cleanup;
 
     if (!virDomainObjIsActive(vm)) {



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]