6:41 a.m.
https://bugzilla.redhat.com/show_bug.cgi?id=1368417
So far, when it comes to 'virsh update-device --config' of disks
we are limiting ourselves for just the disk source update and
just for CDROMs and floppies. This makes no sense. Especially if
you look around and see that we already allow full update to
graphics and net devices. So let's just take whatever XML user
wants to have there and replace our internal definition with it.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_driver.c | 26 +++++++-------------------
1 file changed, 7 insertions(+), 19 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2efaa2a..b5abb0c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8104,35 +8104,23 @@ qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef,
unsigned int parse_flags,
virDomainXMLOptionPtr xmlopt)
{
- virDomainDiskDefPtr orig, disk;
+ virDomainDiskDefPtr newDisk;
virDomainGraphicsDefPtr newGraphics;
virDomainNetDefPtr net;
int pos;
switch ((virDomainDeviceType) dev->type) {
case VIR_DOMAIN_DEVICE_DISK:
- disk = dev->data.disk;
- if (!(orig = virDomainDiskByName(vmdef, disk->dst, false))) {
+ newDisk = dev->data.disk;
+ if ((pos = virDomainDiskIndexByName(vmdef, newDisk->dst, false)) < 0) {
virReportError(VIR_ERR_INVALID_ARG,
- _("target %s doesn't exist."), disk->dst);
+ _("target %s doesn't exist."),
newDisk->dst);
return -1;
}
- if (!(orig->device == VIR_DOMAIN_DISK_DEVICE_CDROM) &&
- !(orig->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY)) {
- virReportError(VIR_ERR_INVALID_ARG, "%s",
- _("this disk doesn't support update"));
- return -1;
- }
- /*
- * Update 'orig'
- * We allow updating src/type//driverType/cachemode/
- */
- orig->cachemode = disk->cachemode;
- orig->startupPolicy = disk->startupPolicy;
- virStorageSourceFree(orig->src);
- orig->src = disk->src;
- disk->src = NULL;
+ virDomainDiskDefFree(vmdef->disks[pos]);
+ vmdef->disks[pos] = newDisk;
+ dev->data.disk = NULL;
break;
case VIR_DOMAIN_DEVICE_GRAPHICS:
--
2.8.4
4:42 p.m.
On 09/02/2016 07:41 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1368417
So far, when it comes to 'virsh update-device --config' of disks
we are limiting ourselves for just the disk source update and
just for CDROMs and floppies. This makes no sense. Especially if
you look around and see that we already allow full update to
graphics and net devices. So let's just take whatever XML user
wants to have there and replace our internal definition with it.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_driver.c | 26 +++++++-------------------
1 file changed, 7 insertions(+), 19 deletions(-)
Crazy this has been this way since 0.9.0 (commit id 'f37c29c8a').
Although this patch certainly does more than just ensure the
startupPolicy adjustment works for --config as well as --live (which is
the basis of the bug).
I would think to "just fix" the bug/issue "DISK" could be added to
that
negative if condition, which leaves just LUN as a type that wouldn't be
updateable (since startupPolicy is updated in the subsequent code).
I guess the question is - is there something in the DISK or LUN specific
usages that shouldn't be just updated or something that could be lost if
we simply delete the old one without copying "something" in
(_virDomainDiskDef is used for a lot and especially that private piece
which could be of concern).
The text in the last paragraph of virDomainUpdateDeviceFlags just
doesn't give me enough of a hint why FLOPPY and CDROM were specifically
singled out.
Let's see if there's other comments/thoughts since I've bumped this.
John
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2efaa2a..b5abb0c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8104,35 +8104,23 @@ qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef,
unsigned int parse_flags,
virDomainXMLOptionPtr xmlopt)
{
- virDomainDiskDefPtr orig, disk;
+ virDomainDiskDefPtr newDisk;
virDomainGraphicsDefPtr newGraphics;
virDomainNetDefPtr net;
int pos;
switch ((virDomainDeviceType) dev->type) {
case VIR_DOMAIN_DEVICE_DISK:
- disk = dev->data.disk;
- if (!(orig = virDomainDiskByName(vmdef, disk->dst, false))) {
+ newDisk = dev->data.disk;
+ if ((pos = virDomainDiskIndexByName(vmdef, newDisk->dst, false)) < 0) {
virReportError(VIR_ERR_INVALID_ARG,
- _("target %s doesn't exist."), disk->dst);
+ _("target %s doesn't exist."),
newDisk->dst);
return -1;
}
- if (!(orig->device == VIR_DOMAIN_DISK_DEVICE_CDROM) &&
- !(orig->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY)) {
- virReportError(VIR_ERR_INVALID_ARG, "%s",
- _("this disk doesn't support update"));
- return -1;
- }
- /*
- * Update 'orig'
- * We allow updating src/type//driverType/cachemode/
- */
- orig->cachemode = disk->cachemode;
- orig->startupPolicy = disk->startupPolicy;
- virStorageSourceFree(orig->src);
- orig->src = disk->src;
- disk->src = NULL;
+ virDomainDiskDefFree(vmdef->disks[pos]);
+ vmdef->disks[pos] = newDisk;
+ dev->data.disk = NULL;
break;
case VIR_DOMAIN_DEVICE_GRAPHICS:
3:07 a.m.
On Fri, Sep 23, 2016 at 05:42:41PM -0400, John Ferlan wrote:
On 09/02/2016 07:41 AM, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1368417
>
> So far, when it comes to 'virsh update-device --config' of disks
> we are limiting ourselves for just the disk source update and
> just for CDROMs and floppies. This makes no sense. Especially if
> you look around and see that we already allow full update to
> graphics and net devices. So let's just take whatever XML user
> wants to have there and replace our internal definition with it.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/qemu/qemu_driver.c | 26 +++++++-------------------
> 1 file changed, 7 insertions(+), 19 deletions(-)
>
Crazy this has been this way since 0.9.0 (commit id 'f37c29c8a').
Although this patch certainly does more than just ensure the
startupPolicy adjustment works for --config as well as --live (which is
the basis of the bug).
I would think to "just fix" the bug/issue "DISK" could be added to
that
negative if condition, which leaves just LUN as a type that wouldn't be
updateable (since startupPolicy is updated in the subsequent code).
I guess the question is - is there something in the DISK or LUN specific
usages that shouldn't be just updated or something that could be lost if
we simply delete the old one without copying "something" in
(_virDomainDiskDef is used for a lot and especially that private piece
which could be of concern).
The text in the last paragraph of virDomainUpdateDeviceFlags just
doesn't give me enough of a hint why FLOPPY and CDROM were specifically
singled out.
It has been that way because we can't update everything LIVE. And
CONFIG just followed (or was left to rot in the fiery pits of
I'm-not-touching-this-code land). Basically, update with CONFIG is the
same thing that you would do with 'virsh edit', just for a particular
device. And we allow everything to be edited and saved there. So ther
eis no reason for restricting anything here in this function, unless
there's some requirement in the callers. And since there is one caller
that uses this to modify a temporary definition (and updates the domain
only after everything else succeeded) I see no reason why this couldn't
go in. So from me it's an ACK. Feel free to chime in if you have other
ideas.
Martin
2982
days inactive
3006
days old
2 comments
3 participants
participants (3)
-
John Ferlan -
Martin Kletzander -
Michal Privoznik