[libvirt] [PATCH 0/2] Add and use for virDomainDiskIndexBy*
Sometimes the only thing we need is the pointer to virDomainDiskDef and having to call virDomainDiskIndexBy* APIs, storing the disk index, and looking it up in the disks array is ugly. After this patch, we can just call virDomainDiskBy* and get the pointer in one step. Jiri Denemark (2): Add wrappers for virDomainDiskIndexBy* Use virDomainDiskByName where appropriate src/conf/domain_conf.c | 20 +++++++++++++++ src/conf/domain_conf.h | 8 ++++++ src/libvirt_private.syms | 2 ++ src/libxl/libxl_driver.c | 4 +-- src/lxc/lxc_driver.c | 10 +++----- src/qemu/qemu_agent.c | 9 ++++--- src/qemu/qemu_blockjob.c | 5 ++-- src/qemu/qemu_driver.c | 62 ++++++++++++++++++----------------------------- src/qemu/qemu_migration.c | 6 ++--- src/qemu/qemu_process.c | 23 ++++-------------- 10 files changed, 72 insertions(+), 77 deletions(-) -- 2.4.1
Sometimes the only thing we need is the pointer to virDomainDiskDef and having to call virDomainDiskIndexBy* APIs, storing the disk index, and looking it up in the disks array is ugly. After this patch, we can just call virDomainDiskBy* and get the pointer in one step. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/conf/domain_conf.c | 20 ++++++++++++++++++++ src/conf/domain_conf.h | 8 ++++++++ src/libvirt_private.syms | 2 ++ 3 files changed, 30 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 394890e..892d7d7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12349,6 +12349,17 @@ virDomainDiskIndexByAddress(virDomainDefPtr def, return -1; } +virDomainDiskDefPtr +virDomainDiskByAddress(virDomainDefPtr def, + virDevicePCIAddressPtr pci_address, + unsigned int bus, + unsigned int target, + unsigned int unit) +{ + int idx = virDomainDiskIndexByAddress(def, pci_address, bus, target, unit); + return idx < 0 ? NULL : def->disks[idx]; +} + int virDomainDiskIndexByName(virDomainDefPtr def, const char *name, bool allow_ambiguous) @@ -12388,6 +12399,15 @@ virDomainDiskPathByName(virDomainDefPtr def, const char *name) return idx < 0 ? NULL : virDomainDiskGetSource(def->disks[idx]); } +virDomainDiskDefPtr +virDomainDiskByName(virDomainDefPtr def, + const char *name, + bool allow_ambiguous) +{ + int idx = virDomainDiskIndexByName(def, name, allow_ambiguous); + return idx < 0 ? NULL : def->disks[idx]; +} + int virDomainDiskInsert(virDomainDefPtr def, virDomainDiskDefPtr disk) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f93d73e..f27f2e4 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2685,8 +2685,16 @@ int virDomainDiskIndexByAddress(virDomainDefPtr def, virDevicePCIAddressPtr pci_controller, unsigned int bus, unsigned int target, unsigned int unit); +virDomainDiskDefPtr virDomainDiskByAddress(virDomainDefPtr def, + virDevicePCIAddressPtr pci_controller, + unsigned int bus, + unsigned int target, + unsigned int unit); int virDomainDiskIndexByName(virDomainDefPtr def, const char *name, bool allow_ambiguous); +virDomainDiskDefPtr virDomainDiskByName(virDomainDefPtr def, + const char *name, + bool allow_ambiguous); const char *virDomainDiskPathByName(virDomainDefPtr, const char *name); int virDomainDiskInsert(virDomainDefPtr def, virDomainDiskDefPtr disk) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 15e71d5..6a95fb9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -238,6 +238,8 @@ virDomainDeviceInfoCopy; virDomainDeviceInfoIterate; virDomainDeviceTypeToString; virDomainDiskBusTypeToString; +virDomainDiskByAddress; +virDomainDiskByName; virDomainDiskCacheTypeFromString; virDomainDiskCacheTypeToString; virDomainDiskDefAssignAddress; -- 2.4.1
On 05/21/2015 11:42 AM, Jiri Denemark wrote:
Sometimes the only thing we need is the pointer to virDomainDiskDef and having to call virDomainDiskIndexBy* APIs, storing the disk index, and looking it up in the disks array is ugly. After this patch, we can just call virDomainDiskBy* and get the pointer in one step.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/conf/domain_conf.c | 20 ++++++++++++++++++++ src/conf/domain_conf.h | 8 ++++++++ src/libvirt_private.syms | 2 ++ 3 files changed, 30 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 394890e..892d7d7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12349,6 +12349,17 @@ virDomainDiskIndexByAddress(virDomainDefPtr def, return -1; }
+virDomainDiskDefPtr +virDomainDiskByAddress(virDomainDefPtr def, + virDevicePCIAddressPtr pci_address, + unsigned int bus, + unsigned int target, + unsigned int unit) Shouldn't this new function be called virDomainDiskByPCIAddess(..)?
+{ + int idx = virDomainDiskIndexByAddress(def, pci_address, bus, target, unit); + return idx < 0 ? NULL : def->disks[idx]; +} + int virDomainDiskIndexByName(virDomainDefPtr def, const char *name, bool allow_ambiguous) @@ -12388,6 +12399,15 @@ virDomainDiskPathByName(virDomainDefPtr def, const char *name) return idx < 0 ? NULL : virDomainDiskGetSource(def->disks[idx]); }
+virDomainDiskDefPtr +virDomainDiskByName(virDomainDefPtr def, + const char *name, + bool allow_ambiguous) +{ + int idx = virDomainDiskIndexByName(def, name, allow_ambiguous); + return idx < 0 ? NULL : def->disks[idx]; +} + int virDomainDiskInsert(virDomainDefPtr def, virDomainDiskDefPtr disk) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f93d73e..f27f2e4 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2685,8 +2685,16 @@ int virDomainDiskIndexByAddress(virDomainDefPtr def, virDevicePCIAddressPtr pci_controller, unsigned int bus, unsigned int target, unsigned int unit); +virDomainDiskDefPtr virDomainDiskByAddress(virDomainDefPtr def, + virDevicePCIAddressPtr pci_controller, + unsigned int bus, + unsigned int target, + unsigned int unit); int virDomainDiskIndexByName(virDomainDefPtr def, const char *name, bool allow_ambiguous); +virDomainDiskDefPtr virDomainDiskByName(virDomainDefPtr def, + const char *name, + bool allow_ambiguous); const char *virDomainDiskPathByName(virDomainDefPtr, const char *name); int virDomainDiskInsert(virDomainDefPtr def, virDomainDiskDefPtr disk) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 15e71d5..6a95fb9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -238,6 +238,8 @@ virDomainDeviceInfoCopy; virDomainDeviceInfoIterate; virDomainDeviceTypeToString; virDomainDiskBusTypeToString; +virDomainDiskByAddress; +virDomainDiskByName; virDomainDiskCacheTypeFromString; virDomainDiskCacheTypeToString; virDomainDiskDefAssignAddress;
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Thu, May 21, 2015 at 13:53:16 +0200, Boris Fiuczynski wrote:
On 05/21/2015 11:42 AM, Jiri Denemark wrote:
Sometimes the only thing we need is the pointer to virDomainDiskDef and having to call virDomainDiskIndexBy* APIs, storing the disk index, and looking it up in the disks array is ugly. After this patch, we can just call virDomainDiskBy* and get the pointer in one step.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/conf/domain_conf.c | 20 ++++++++++++++++++++ src/conf/domain_conf.h | 8 ++++++++ src/libvirt_private.syms | 2 ++ 3 files changed, 30 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 394890e..892d7d7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12349,6 +12349,17 @@ virDomainDiskIndexByAddress(virDomainDefPtr def, return -1; }
+virDomainDiskDefPtr +virDomainDiskByAddress(virDomainDefPtr def, + virDevicePCIAddressPtr pci_address, + unsigned int bus, + unsigned int target, + unsigned int unit) Shouldn't this new function be called virDomainDiskByPCIAddess(..)?
No. For two reasons, it matches the name of virDomainDiskIndexByAddress and both functions also work with drive addresses (controller, bus, target, unit). Jirka
Most virDomainDiskIndexByName callers do not care about the index; what they really want is a disk def pointer. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/libxl/libxl_driver.c | 4 +-- src/lxc/lxc_driver.c | 10 +++----- src/qemu/qemu_agent.c | 9 ++++--- src/qemu/qemu_blockjob.c | 5 ++-- src/qemu/qemu_driver.c | 62 ++++++++++++++++++----------------------------- src/qemu/qemu_migration.c | 6 ++--- src/qemu/qemu_process.c | 23 ++++-------------- 7 files changed, 42 insertions(+), 77 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 895cf26..12be816 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3502,18 +3502,16 @@ libxlDomainUpdateDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev) { virDomainDiskDefPtr orig; virDomainDiskDefPtr disk; - int idx; int ret = -1; switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: disk = dev->data.disk; - if ((idx = virDomainDiskIndexByName(vmdef, disk->dst, false)) < 0) { + if (!(orig = virDomainDiskByName(vmdef, disk->dst, false))) { virReportError(VIR_ERR_INVALID_ARG, _("target %s doesn't exist."), disk->dst); goto cleanup; } - orig = vmdef->disks[idx]; if (!(orig->device == VIR_DOMAIN_DISK_DEVICE_CDROM)) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("this disk doesn't support update")); diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 188ff80..31fb470 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2332,7 +2332,7 @@ lxcDomainBlockStats(virDomainPtr dom, const char *path, virDomainBlockStatsPtr stats) { - int ret = -1, idx; + int ret = -1; virDomainObjPtr vm; virDomainDiskDefPtr disk = NULL; virLXCDomainObjPrivatePtr priv; @@ -2367,12 +2367,11 @@ lxcDomainBlockStats(virDomainPtr dom, goto cleanup; } - if ((idx = virDomainDiskIndexByName(vm->def, path, false)) < 0) { + if (!(disk = virDomainDiskByName(vm->def, path, false))) { virReportError(VIR_ERR_INVALID_ARG, _("invalid path: %s"), path); goto cleanup; } - disk = vm->def->disks[idx]; if (!disk->info.alias) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -2400,7 +2399,7 @@ lxcDomainBlockStatsFlags(virDomainPtr dom, int * nparams, unsigned int flags) { - int tmp, ret = -1, idx; + int tmp, ret = -1; virDomainObjPtr vm; virDomainDiskDefPtr disk = NULL; virLXCDomainObjPrivatePtr priv; @@ -2449,12 +2448,11 @@ lxcDomainBlockStatsFlags(virDomainPtr dom, goto cleanup; } } else { - if ((idx = virDomainDiskIndexByName(vm->def, path, false)) < 0) { + if (!(disk = virDomainDiskByName(vm->def, path, false))) { virReportError(VIR_ERR_INVALID_ARG, _("invalid path: %s"), path); goto cleanup; } - disk = vm->def->disks[idx]; if (!disk->info.alias) { virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index fc23c41..b57a3df 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1850,9 +1850,10 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info, for (j = 0; j < ndisk; j++) { virJSONValuePtr disk = virJSONValueArrayGet(entry, j); virJSONValuePtr pci; - int diskaddr[3], pciaddr[4], idx; + int diskaddr[3], pciaddr[4]; const char *diskaddr_comp[] = {"bus", "target", "unit"}; const char *pciaddr_comp[] = {"domain", "bus", "slot", "function"}; + virDomainDiskDefPtr diskDef; if (!disk) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -1892,12 +1893,12 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info, pci_address.bus = pciaddr[1]; pci_address.slot = pciaddr[2]; pci_address.function = pciaddr[3]; - if ((idx = virDomainDiskIndexByAddress( + if (!(diskDef = virDomainDiskByAddress( vmdef, &pci_address, - diskaddr[0], diskaddr[1], diskaddr[2])) < 0) + diskaddr[0], diskaddr[1], diskaddr[2]))) continue; - if (VIR_STRDUP(*alias, vmdef->disks[idx]->dst) < 0) + if (VIR_STRDUP(*alias, diskDef->dst) < 0) goto cleanup; if (*alias) { diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index b9572d0..098a43a 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -82,11 +82,10 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver, case VIR_DOMAIN_BLOCK_JOB_COMPLETED: if (disk->mirrorState == VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT) { if (vm->newDef) { - int indx = virDomainDiskIndexByName(vm->newDef, disk->dst, false); virStorageSourcePtr copy = NULL; - if (indx >= 0) { - persistDisk = vm->newDef->disks[indx]; + if ((persistDisk = virDomainDiskByName(vm->newDef, + disk->dst, false))) { copy = virStorageSourceCopy(disk->mirror, false); if (virStorageSourceInitChainElement(copy, persistDisk->src, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 31cc2ee..2ac72a4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8559,13 +8559,11 @@ qemuDomainUpdateDeviceConfig(virQEMUCapsPtr qemuCaps, switch ((virDomainDeviceType) dev->type) { case VIR_DOMAIN_DEVICE_DISK: disk = dev->data.disk; - pos = virDomainDiskIndexByName(vmdef, disk->dst, false); - if (pos < 0) { + if (!(orig = virDomainDiskByName(vmdef, disk->dst, false))) { virReportError(VIR_ERR_INVALID_ARG, _("target %s doesn't exist."), disk->dst); return -1; } - orig = vmdef->disks[pos]; if (!(orig->device == VIR_DOMAIN_DISK_DEVICE_CDROM) && !(orig->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY)) { virReportError(VIR_ERR_INVALID_ARG, "%s", @@ -11163,7 +11161,7 @@ qemuDomainBlockResize(virDomainPtr dom, virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; qemuDomainObjPrivatePtr priv; - int ret = -1, idx; + int ret = -1; char *device = NULL; virDomainDiskDefPtr disk = NULL; @@ -11203,12 +11201,11 @@ qemuDomainBlockResize(virDomainPtr dom, goto endjob; } - if ((idx = virDomainDiskIndexByName(vm->def, path, false)) < 0) { + if (!(disk = virDomainDiskByName(vm->def, path, false)) < 0) { virReportError(VIR_ERR_INVALID_ARG, _("invalid path: %s"), path); goto endjob; } - disk = vm->def->disks[idx]; /* qcow2 and qed must be sized on 512 byte blocks/sectors, * so adjust size if necessary to round up. @@ -11291,13 +11288,10 @@ qemuDomainBlocksStatsGather(virQEMUDriverPtr driver, int ret = -1; if (*path) { - int idx; - - if ((idx = virDomainDiskIndexByName(vm->def, path, false)) < 0) { + if (!(disk = virDomainDiskByName(vm->def, path, false))) { virReportError(VIR_ERR_INVALID_ARG, _("invalid path: %s"), path); goto cleanup; } - disk = vm->def->disks[idx]; if (!disk->info.alias) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -12178,7 +12172,6 @@ qemuDomainGetBlockInfo(virDomainPtr dom, int ret = -1; virDomainDiskDefPtr disk = NULL; virStorageSourcePtr src; - int idx; bool activeFail = false; virQEMUDriverConfigPtr cfg = NULL; @@ -12205,14 +12198,12 @@ qemuDomainGetBlockInfo(virDomainPtr dom, if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) goto cleanup; - /* Check the path belongs to this domain. */ - if ((idx = virDomainDiskIndexByName(vm->def, path, false)) < 0) { + if (!(disk = virDomainDiskByName(vm->def, path, false))) { virReportError(VIR_ERR_INVALID_ARG, _("invalid path %s not assigned to domain"), path); goto endjob; } - disk = vm->def->disks[idx]; src = disk->src; if (virStorageSourceIsEmpty(src)) { virReportError(VIR_ERR_INVALID_ARG, @@ -14640,15 +14631,11 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, if (snap->def->disks[i].snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) continue; - if (vm->newDef) { - int indx = virDomainDiskIndexByName(vm->newDef, - vm->def->disks[i]->dst, - false); - if (indx >= 0) { - persistDisk = vm->newDef->disks[indx]; - persist = true; - } - } + if (vm->newDef && + (persistDisk = virDomainDiskByName(vm->newDef, + vm->def->disks[i]->dst, + false))) + persist = true; ret = qemuDomainSnapshotCreateSingleDiskActive(driver, vm, &snap->def->disks[i], @@ -14681,15 +14668,11 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, if (snap->def->disks[i].snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) continue; - if (vm->newDef) { - int indx = virDomainDiskIndexByName(vm->newDef, - vm->def->disks[i]->dst, - false); - if (indx >= 0) { - persistDisk = vm->newDef->disks[indx]; - persist = true; - } - } + if (vm->newDef && + (persistDisk = virDomainDiskByName(vm->newDef, + vm->def->disks[i]->dst, + false))) + persist = true; qemuDomainSnapshotUndoSingleDiskActive(driver, vm, vm->def->disks[i], @@ -17687,7 +17670,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, int ret = -1; size_t i; int idx = -1; - int conf_idx = -1; + virDomainDiskDefPtr conf_disk = NULL; bool set_bytes = false; bool set_iops = false; bool set_bytes_max = false; @@ -17921,7 +17904,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - if ((conf_idx = virDomainDiskIndexByName(persistentDef, disk, true)) < 0) { + if (!(conf_disk = virDomainDiskByName(persistentDef, disk, true))) { virReportError(VIR_ERR_INVALID_ARG, _("missing persistent configuration for disk '%s'"), disk); @@ -18000,7 +17983,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, if (flags & VIR_DOMAIN_AFFECT_CONFIG) { sa_assert(persistentDef); - oldinfo = &persistentDef->disks[conf_idx]->blkdeviotune; + sa_assert(conf_disk); + oldinfo = &conf_disk->blkdeviotune; if (!set_bytes) { info.total_bytes_sec = oldinfo->total_bytes_sec; info.read_bytes_sec = oldinfo->read_bytes_sec; @@ -18011,7 +17995,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, info.read_iops_sec = oldinfo->read_iops_sec; info.write_iops_sec = oldinfo->write_iops_sec; } - persistentDef->disks[conf_idx]->blkdeviotune = info; + conf_disk->blkdeviotune = info; ret = virDomainSaveConfig(cfg->configDir, persistentDef); if (ret < 0) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -18105,14 +18089,14 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - int idx = virDomainDiskIndexByName(persistentDef, disk, true); - if (idx < 0) { + virDomainDiskDefPtr diskDef; + if (!(diskDef = virDomainDiskByName(persistentDef, disk, true))) { virReportError(VIR_ERR_INVALID_ARG, _("disk '%s' was not found in the domain config"), disk); goto endjob; } - reply = persistentDef->disks[idx]->blkdeviotune; + reply = diskDef->blkdeviotune; } for (i = 0; i < QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX && i < *nparams; i++) { diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 8fe1cfb..f7432e8 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1594,21 +1594,19 @@ qemuMigrationPrecreateStorage(virConnectPtr conn, for (i = 0; i < nbd->ndisks; i++) { virDomainDiskDefPtr disk; - int indx; const char *diskSrcPath; VIR_DEBUG("Looking up disk target '%s' (capacity=%llu)", nbd->disks[i].target, nbd->disks[i].capacity); - if ((indx = virDomainDiskIndexByName(vm->def, - nbd->disks[i].target, false)) < 0) { + if (!(disk = virDomainDiskByName(vm->def, nbd->disks[i].target, + false))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unable to find disk by target: %s"), nbd->disks[i].target); goto cleanup; } - disk = vm->def->disks[indx]; diskSrcPath = virDomainDiskGetSource(disk); if (disk->src->shared || disk->src->readonly || diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 118fc52..3a4c8bf 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -357,21 +357,6 @@ qemuProcessHandleMonitorError(qemuMonitorPtr mon ATTRIBUTE_UNUSED, } -static virDomainDiskDefPtr -qemuProcessFindDomainDiskByPath(virDomainObjPtr vm, - const char *path) -{ - int idx = virDomainDiskIndexByName(vm->def, path, true); - - if (idx >= 0) - return vm->def->disks[idx]; - - virReportError(VIR_ERR_INTERNAL_ERROR, - _("no disk found with path %s"), - path); - return NULL; -} - virDomainDiskDefPtr qemuProcessFindDomainDiskByAlias(virDomainObjPtr vm, const char *alias) @@ -492,10 +477,12 @@ qemuProcessFindVolumeQcowPassphrase(qemuMonitorPtr mon ATTRIBUTE_UNUSED, int ret = -1; virObjectLock(vm); - disk = qemuProcessFindDomainDiskByPath(vm, path); - - if (!disk) + if (!(disk = virDomainDiskByName(vm->def, path, true))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("no disk found with path %s"), + path); goto cleanup; + } ret = qemuProcessGetVolumeQcowPassphrase(conn, disk, secretRet, secretLen); -- 2.4.1
On Thu, May 21, 2015 at 11:42:25AM +0200, Jiri Denemark wrote:
Most virDomainDiskIndexByName callers do not care about the index; what they really want is a disk def pointer.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/libxl/libxl_driver.c | 4 +-- src/lxc/lxc_driver.c | 10 +++----- src/qemu/qemu_agent.c | 9 ++++--- src/qemu/qemu_blockjob.c | 5 ++-- src/qemu/qemu_driver.c | 62 ++++++++++++++++++----------------------------- src/qemu/qemu_migration.c | 6 ++--- src/qemu/qemu_process.c | 23 ++++-------------- 7 files changed, 42 insertions(+), 77 deletions(-)
ACK series.
@@ -18000,7 +17983,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
if (flags & VIR_DOMAIN_AFFECT_CONFIG) { sa_assert(persistentDef);
Can we get rid of this one now? Jan
- oldinfo = &persistentDef->disks[conf_idx]->blkdeviotune; + sa_assert(conf_disk); + oldinfo = &conf_disk->blkdeviotune; if (!set_bytes) { info.total_bytes_sec = oldinfo->total_bytes_sec; info.read_bytes_sec = oldinfo->read_bytes_sec;
On 05/21/2015 07:17 AM, Ján Tomko wrote:
On Thu, May 21, 2015 at 11:42:25AM +0200, Jiri Denemark wrote:
Most virDomainDiskIndexByName callers do not care about the index; what they really want is a disk def pointer.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/libxl/libxl_driver.c | 4 +-- src/lxc/lxc_driver.c | 10 +++----- src/qemu/qemu_agent.c | 9 ++++--- src/qemu/qemu_blockjob.c | 5 ++-- src/qemu/qemu_driver.c | 62 ++++++++++++++++++----------------------------- src/qemu/qemu_migration.c | 6 ++--- src/qemu/qemu_process.c | 23 ++++-------------- 7 files changed, 42 insertions(+), 77 deletions(-)
ACK series.
@@ -18000,7 +17983,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
if (flags & VIR_DOMAIN_AFFECT_CONFIG) { sa_assert(persistentDef);
Can we get rid of this one now?
Jan
It seems so from my quick Coverity run, but hold on before pushing as there are two new issues... I'll respond separately John
- oldinfo = &persistentDef->disks[conf_idx]->blkdeviotune; + sa_assert(conf_disk); + oldinfo = &conf_disk->blkdeviotune; if (!set_bytes) { info.total_bytes_sec = oldinfo->total_bytes_sec; info.read_bytes_sec = oldinfo->read_bytes_sec;
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 05/21/2015 05:42 AM, Jiri Denemark wrote:
Most virDomainDiskIndexByName callers do not care about the index; what they really want is a disk def pointer.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/libxl/libxl_driver.c | 4 +-- src/lxc/lxc_driver.c | 10 +++----- src/qemu/qemu_agent.c | 9 ++++--- src/qemu/qemu_blockjob.c | 5 ++-- src/qemu/qemu_driver.c | 62 ++++++++++++++++++----------------------------- src/qemu/qemu_migration.c | 6 ++--- src/qemu/qemu_process.c | 23 ++++-------------- 7 files changed, 42 insertions(+), 77 deletions(-)
Since Jan asked - I ran this against Coverity... ...
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 31cc2ee..2ac72a4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8559,13 +8559,11 @@ qemuDomainUpdateDeviceConfig(virQEMUCapsPtr qemuCaps, switch ((virDomainDeviceType) dev->type) { case VIR_DOMAIN_DEVICE_DISK: disk = dev->data.disk; - pos = virDomainDiskIndexByName(vmdef, disk->dst, false); - if (pos < 0) { + if (!(orig = virDomainDiskByName(vmdef, disk->dst, false))) { virReportError(VIR_ERR_INVALID_ARG, _("target %s doesn't exist."), disk->dst); return -1; } - orig = vmdef->disks[pos]; if (!(orig->device == VIR_DOMAIN_DISK_DEVICE_CDROM) && !(orig->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY)) { virReportError(VIR_ERR_INVALID_ARG, "%s", @@ -11163,7 +11161,7 @@ qemuDomainBlockResize(virDomainPtr dom, virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; qemuDomainObjPrivatePtr priv; - int ret = -1, idx; + int ret = -1; char *device = NULL; virDomainDiskDefPtr disk = NULL;
@@ -11203,12 +11201,11 @@ qemuDomainBlockResize(virDomainPtr dom, goto endjob; }
- if ((idx = virDomainDiskIndexByName(vm->def, path, false)) < 0) { + if (!(disk = virDomainDiskByName(vm->def, path, false)) < 0) {
(1) Event result_independent_of_operands: "!(disk = virDomainDiskByName(vm->def, path, false /* 0 */)) < 0" is always false regardless of the values of its operands. This occurs as the logical operand of if. That led to a second complaint which won't happen if the " < 0" is removed.
virReportError(VIR_ERR_INVALID_ARG, _("invalid path: %s"), path); goto endjob; } - disk = vm->def->disks[idx];
/* qcow2 and qed must be sized on 512 byte blocks/sectors, * so adjust size if necessary to round up.
second issue was here since disk could be accessed... And yes, the sa_assert(persistent_def) that Jan asked about can be safely removed. Coverity also doesn't complain if the sa_assert(conf_disk) is removed. I'll have to check if a few of those previous false positives just like this could be removed... Although for this case it could be because conf_disk is populated and accessed within the same function for the same condition, so it's much easier for Coverity to track. John
On Thu, May 21, 2015 at 07:42:53 -0400, John Ferlan wrote:
On 05/21/2015 05:42 AM, Jiri Denemark wrote:
Most virDomainDiskIndexByName callers do not care about the index; what they really want is a disk def pointer.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/libxl/libxl_driver.c | 4 +-- src/lxc/lxc_driver.c | 10 +++----- src/qemu/qemu_agent.c | 9 ++++--- src/qemu/qemu_blockjob.c | 5 ++-- src/qemu/qemu_driver.c | 62 ++++++++++++++++++----------------------------- src/qemu/qemu_migration.c | 6 ++--- src/qemu/qemu_process.c | 23 ++++-------------- 7 files changed, 42 insertions(+), 77 deletions(-)
Since Jan asked - I ran this against Coverity... ...
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 31cc2ee..2ac72a4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8559,13 +8559,11 @@ qemuDomainUpdateDeviceConfig(virQEMUCapsPtr qemuCaps, switch ((virDomainDeviceType) dev->type) { case VIR_DOMAIN_DEVICE_DISK: disk = dev->data.disk; - pos = virDomainDiskIndexByName(vmdef, disk->dst, false); - if (pos < 0) { + if (!(orig = virDomainDiskByName(vmdef, disk->dst, false))) { virReportError(VIR_ERR_INVALID_ARG, _("target %s doesn't exist."), disk->dst); return -1; } - orig = vmdef->disks[pos]; if (!(orig->device == VIR_DOMAIN_DISK_DEVICE_CDROM) && !(orig->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY)) { virReportError(VIR_ERR_INVALID_ARG, "%s", @@ -11163,7 +11161,7 @@ qemuDomainBlockResize(virDomainPtr dom, virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; qemuDomainObjPrivatePtr priv; - int ret = -1, idx; + int ret = -1; char *device = NULL; virDomainDiskDefPtr disk = NULL;
@@ -11203,12 +11201,11 @@ qemuDomainBlockResize(virDomainPtr dom, goto endjob; }
- if ((idx = virDomainDiskIndexByName(vm->def, path, false)) < 0) { + if (!(disk = virDomainDiskByName(vm->def, path, false)) < 0) {
(1) Event result_independent_of_operands: "!(disk = virDomainDiskByName(vm->def, path, false /* 0 */)) < 0" is always false regardless of the values of its operands. This occurs as the logical operand of if.
Oops, I fixed two similar issues and didn't notice this third one.
And yes, the sa_assert(persistent_def) that Jan asked about can be safely removed. Coverity also doesn't complain if the sa_assert(conf_disk) is removed.
Great, I'll remove both. Jirka
On Thu, May 21, 2015 at 14:18:44 +0200, Jiri Denemark wrote:
On Thu, May 21, 2015 at 07:42:53 -0400, John Ferlan wrote:
On 05/21/2015 05:42 AM, Jiri Denemark wrote:
Most virDomainDiskIndexByName callers do not care about the index; what they really want is a disk def pointer.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/libxl/libxl_driver.c | 4 +-- src/lxc/lxc_driver.c | 10 +++----- src/qemu/qemu_agent.c | 9 ++++--- src/qemu/qemu_blockjob.c | 5 ++-- src/qemu/qemu_driver.c | 62 ++++++++++++++++++----------------------------- src/qemu/qemu_migration.c | 6 ++--- src/qemu/qemu_process.c | 23 ++++-------------- 7 files changed, 42 insertions(+), 77 deletions(-)
Since Jan asked - I ran this against Coverity... ...
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 31cc2ee..2ac72a4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8559,13 +8559,11 @@ qemuDomainUpdateDeviceConfig(virQEMUCapsPtr qemuCaps, switch ((virDomainDeviceType) dev->type) { case VIR_DOMAIN_DEVICE_DISK: disk = dev->data.disk; - pos = virDomainDiskIndexByName(vmdef, disk->dst, false); - if (pos < 0) { + if (!(orig = virDomainDiskByName(vmdef, disk->dst, false))) { virReportError(VIR_ERR_INVALID_ARG, _("target %s doesn't exist."), disk->dst); return -1; } - orig = vmdef->disks[pos]; if (!(orig->device == VIR_DOMAIN_DISK_DEVICE_CDROM) && !(orig->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY)) { virReportError(VIR_ERR_INVALID_ARG, "%s", @@ -11163,7 +11161,7 @@ qemuDomainBlockResize(virDomainPtr dom, virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; qemuDomainObjPrivatePtr priv; - int ret = -1, idx; + int ret = -1; char *device = NULL; virDomainDiskDefPtr disk = NULL;
@@ -11203,12 +11201,11 @@ qemuDomainBlockResize(virDomainPtr dom, goto endjob; }
- if ((idx = virDomainDiskIndexByName(vm->def, path, false)) < 0) { + if (!(disk = virDomainDiskByName(vm->def, path, false)) < 0) {
(1) Event result_independent_of_operands: "!(disk = virDomainDiskByName(vm->def, path, false /* 0 */)) < 0" is always false regardless of the values of its operands. This occurs as the logical operand of if.
Oops, I fixed two similar issues and didn't notice this third one.
And yes, the sa_assert(persistent_def) that Jan asked about can be safely removed. Coverity also doesn't complain if the sa_assert(conf_disk) is removed.
Great, I'll remove both.
Done and pushed, thanks. Jirka
participants (4)
-
Boris Fiuczynski -
Jiri Denemark -
John Ferlan -
Ján Tomko