4:42 a.m.
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
4:42 a.m.
New subject: [libvirt] [PATCH 1/2] Add wrappers 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.
Signed-off-by: Jiri Denemark <jdenemar(a)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
6:53 a.m.
New subject: [libvirt] [PATCH 1/2] Add wrappers for virDomainDiskIndexBy*
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(a)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
7:15 a.m.
New subject: [libvirt] [PATCH 1/2] Add wrappers for virDomainDiskIndexBy*
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(a)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
4:42 a.m.
New subject: [libvirt] [PATCH 2/2] Use virDomainDiskByName where appropriate
Most virDomainDiskIndexByName callers do not care about the index; what
they really want is a disk def pointer.
Signed-off-by: Jiri Denemark <jdenemar(a)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
6:17 a.m.
New subject: [libvirt] [PATCH 2/2] Use virDomainDiskByName where appropriate
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(a)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;
6:31 a.m.
New subject: [libvirt] [PATCH 2/2] Use virDomainDiskByName where appropriate
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(a)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(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
6:42 a.m.
New subject: [libvirt] [PATCH 2/2] Use virDomainDiskByName where appropriate
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(a)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
7:18 a.m.
New subject: [libvirt] [PATCH 2/2] Use virDomainDiskByName where appropriate
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(a)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
7:51 a.m.
New subject: [libvirt] [PATCH 2/2] Use virDomainDiskByName where appropriate
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(a)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
3474
days inactive
3474
days old
9 comments
4 participants
participants (4)
-
Boris Fiuczynski -
Jiri Denemark -
John Ferlan -
Ján Tomko