10:18 a.m.
Detaching disk devices is not only possible for VIR_DOMAIN_DISK_BUS_VIRTIO,
but also for VIR_DOMAIN_DISK_BUS_SCSI, so take care of this possibility.
Additionally, when the new-style device syntax is used, we do not need to
check if the PCI address is valid since we don't need it to do the
hot-unplugging. And while we're at it, drop the "pci" part
in qemudDomainDetachPciDiskDevice() -- it's misleading since we
do not necessarily have to deal with PCI addresses.
Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer(a)siemens.com>
---
src/qemu/qemu_driver.c | 16 +++++++++-------
1 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 704f824..f2b8517 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7862,10 +7862,10 @@ cleanup:
}
-static int qemudDomainDetachPciDiskDevice(struct qemud_driver *driver,
- virDomainObjPtr vm,
- virDomainDeviceDefPtr dev,
- unsigned long long qemuCmdFlags)
+static int qemudDomainDetachDiskDevice(struct qemud_driver *driver,
+ virDomainObjPtr vm,
+ virDomainDeviceDefPtr dev,
+ unsigned long long qemuCmdFlags)
{
int i, ret = -1;
virDomainDiskDefPtr detach = NULL;
@@ -7884,7 +7884,8 @@ static int qemudDomainDetachPciDiskDevice(struct qemud_driver
*driver,
goto cleanup;
}
- if (!virDomainDeviceAddressIsValid(&detach->info,
+ if (!(qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) &&
+ !virDomainDeviceAddressIsValid(&detach->info,
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) {
qemuReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("device cannot be detached without a PCI address"));
@@ -8373,8 +8374,9 @@ static int qemudDomainDetachDevice(virDomainPtr dom,
if (dev->type == VIR_DOMAIN_DEVICE_DISK &&
dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_DISK &&
- dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) {
- ret = qemudDomainDetachPciDiskDevice(driver, vm, dev, qemuCmdFlags);
+ (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO ||
+ dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_SCSI)) {
+ ret = qemudDomainDetachDiskDevice(driver, vm, dev, qemuCmdFlags);
} else if (dev->type == VIR_DOMAIN_DEVICE_NET) {
ret = qemudDomainDetachNetDevice(driver, vm, dev, qemuCmdFlags);
} else if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) {
--
1.6.4
10:22 a.m.
On Tue, May 04, 2010 at 05:18:24PM +0200, Wolfgang Mauerer wrote:
Detaching disk devices is not only possible for
VIR_DOMAIN_DISK_BUS_VIRTIO,
but also for VIR_DOMAIN_DISK_BUS_SCSI, so take care of this possibility.
Additionally, when the new-style device syntax is used, we do not need to
check if the PCI address is valid since we don't need it to do the
hot-unplugging. And while we're at it, drop the "pci" part
in qemudDomainDetachPciDiskDevice() -- it's misleading since we
do not necessarily have to deal with PCI addresses.
Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer(a)siemens.com>
---
src/qemu/qemu_driver.c | 16 +++++++++-------
1 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 704f824..f2b8517 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7862,10 +7862,10 @@ cleanup:
}
-static int qemudDomainDetachPciDiskDevice(struct qemud_driver *driver,
- virDomainObjPtr vm,
- virDomainDeviceDefPtr dev,
- unsigned long long qemuCmdFlags)
+static int qemudDomainDetachDiskDevice(struct qemud_driver *driver,
+ virDomainObjPtr vm,
+ virDomainDeviceDefPtr dev,
+ unsigned long long qemuCmdFlags)
I'd rather we introduced a separate method
qemudDomainDetachSCSIDiskDevice
since logically these are different types of objects/operations, that just
happen to share a common monitor command with new QEMU. It also needs to
give a more explicit error in the case where QEMUD_CMD_FLAG_DEVICE is not
set for SCSI, since we can't support that for SCSI, but can for VirtIO.
{
int i, ret = -1;
virDomainDiskDefPtr detach = NULL;
@@ -7884,7 +7884,8 @@ static int qemudDomainDetachPciDiskDevice(struct qemud_driver
*driver,
goto cleanup;
}
- if (!virDomainDeviceAddressIsValid(&detach->info,
+ if (!(qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) &&
+ !virDomainDeviceAddressIsValid(&detach->info,
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) {
qemuReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("device cannot be detached without a PCI
address"));
@@ -8373,8 +8374,9 @@ static int qemudDomainDetachDevice(virDomainPtr dom,
if (dev->type == VIR_DOMAIN_DEVICE_DISK &&
dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_DISK &&
- dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) {
- ret = qemudDomainDetachPciDiskDevice(driver, vm, dev, qemuCmdFlags);
+ (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO ||
+ dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_SCSI)) {
+ ret = qemudDomainDetachDiskDevice(driver, vm, dev, qemuCmdFlags);
} else if (dev->type == VIR_DOMAIN_DEVICE_NET) {
ret = qemudDomainDetachNetDevice(driver, vm, dev, qemuCmdFlags);
} else if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) {
Regards,
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
9:51 a.m.
Daniel P. Berrange wrote:
On Tue, May 04, 2010 at 05:18:24PM +0200, Wolfgang Mauerer wrote:
> Detaching disk devices is not only possible for VIR_DOMAIN_DISK_BUS_VIRTIO,
> but also for VIR_DOMAIN_DISK_BUS_SCSI, so take care of this possibility.
>
> Additionally, when the new-style device syntax is used, we do not need to
> check if the PCI address is valid since we don't need it to do the
> hot-unplugging. And while we're at it, drop the "pci" part
> in qemudDomainDetachPciDiskDevice() -- it's misleading since we
> do not necessarily have to deal with PCI addresses.
>
> Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer(a)siemens.com>
> ---
> src/qemu/qemu_driver.c | 16 +++++++++-------
> 1 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 704f824..f2b8517 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -7862,10 +7862,10 @@ cleanup:
> }
>
>
> -static int qemudDomainDetachPciDiskDevice(struct qemud_driver *driver,
> - virDomainObjPtr vm,
> - virDomainDeviceDefPtr dev,
> - unsigned long long qemuCmdFlags)
> +static int qemudDomainDetachDiskDevice(struct qemud_driver *driver,
> + virDomainObjPtr vm,
> + virDomainDeviceDefPtr dev,
> + unsigned long long qemuCmdFlags)
I'd rather we introduced a separate method
qemudDomainDetachSCSIDiskDevice
since logically these are different types of objects/operations, that just
happen to share a common monitor command with new QEMU. It also needs to
give a more explicit error in the case where QEMUD_CMD_FLAG_DEVICE is not
set for SCSI, since we can't support that for SCSI, but can for VirtIO.
okay, I've created a separate function for SCSI disk unplug. Since
the differences between VirtIO and SCSI disk unplug are tiny,
I've tried to extract at least a few commonalities -- see the two
follow-up patches. Albeit there's much more duplicated code lurking in
qemu_driver.c, it might make sense to look at this some day.
Best, Wolfgang
9:52 a.m.
New subject: [libvirt] [PATCH v2 1/2] Refactor disk unplugging
We can reuse some of the code for other purposes.
Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer(a)siemens.com>
---
src/qemu/qemu_driver.c | 56 ++++++++++++++++++++++++++++++-----------------
1 files changed, 36 insertions(+), 20 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 47ae52c..63ca57c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7867,6 +7867,36 @@ cleanup:
}
+static inline int qemudFindDisk(virDomainDefPtr def, char *dst)
+{
+ int i;
+
+ for (i = 0 ; i < def->ndisks ; i++) {
+ if (STREQ(def->disks[i]->dst, dst)) {
+ return i;
+ }
+ }
+
+ return -1;
+}
+
+static inline void qemudShrinkDisks(virDomainDefPtr def, int i)
+{
+ if (def->ndisks > 1) {
+ memmove(def->disks + i,
+ def->disks + i + 1,
+ sizeof(*def->disks) *
+ (def->ndisks - (i + 1)));
+ def->ndisks--;
+ if (VIR_REALLOC_N(def->disks, def->ndisks) < 0) {
+ /* ignore, harmless */
+ }
+ } else {
+ VIR_FREE(def->disks);
+ def->ndisks = 0;
+ }
+}
+
static int qemudDomainDetachPciDiskDevice(struct qemud_driver *driver,
virDomainObjPtr vm,
virDomainDeviceDefPtr dev,
@@ -7876,19 +7906,16 @@ static int qemudDomainDetachPciDiskDevice(struct qemud_driver
*driver,
virDomainDiskDefPtr detach = NULL;
qemuDomainObjPrivatePtr priv = vm->privateData;
- for (i = 0 ; i < vm->def->ndisks ; i++) {
- if (STREQ(vm->def->disks[i]->dst, dev->data.disk->dst)) {
- detach = vm->def->disks[i];
- break;
- }
- }
+ i = qemudFindDisk(vm->def, dev->data.disk->dst);
- if (!detach) {
+ if (i < 0) {
qemuReportError(VIR_ERR_OPERATION_FAILED,
_("disk %s not found"), dev->data.disk->dst);
goto cleanup;
}
+ detach = vm->def->disks[i];
+
if (!virDomainDeviceAddressIsValid(&detach->info,
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) {
qemuReportError(VIR_ERR_OPERATION_FAILED, "%s",
@@ -7911,19 +7938,8 @@ static int qemudDomainDetachPciDiskDevice(struct qemud_driver
*driver,
}
qemuDomainObjExitMonitorWithDriver(driver, vm);
- if (vm->def->ndisks > 1) {
- memmove(vm->def->disks + i,
- vm->def->disks + i + 1,
- sizeof(*vm->def->disks) *
- (vm->def->ndisks - (i + 1)));
- vm->def->ndisks--;
- if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks) < 0) {
- /* ignore, harmless */
- }
- } else {
- VIR_FREE(vm->def->disks);
- vm->def->ndisks = 0;
- }
+ qemudShrinkDisks(vm->def, i);
+
virDomainDiskDefFree(detach);
if (driver->securityDriver &&
--
1.6.4
4:27 p.m.
New subject: [libvirt] [PATCH v2 1/2] Refactor disk unplugging
On 05/05/2010 08:52 AM, Wolfgang Mauerer wrote:
We can reuse some of the code for other purposes.
Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer(a)siemens.com>
---
src/qemu/qemu_driver.c | 56 ++++++++++++++++++++++++++++++-----------------
1 files changed, 36 insertions(+), 20 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 47ae52c..63ca57c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7867,6 +7867,36 @@ cleanup:
}
+static inline int qemudFindDisk(virDomainDefPtr def, char *dst)
dst can be const char *.
+{
+ int i;
+
+ for (i = 0 ; i < def->ndisks ; i++) {
+ if (STREQ(def->disks[i]->dst, dst)) {
+ return i;
+ }
+ }
+
+ return -1;
+}
+
+static inline void qemudShrinkDisks(virDomainDefPtr def, int i)
And i can be unsigned (better yet, size_t).
+{
+ if (def->ndisks > 1) {
+ memmove(def->disks + i,
+ def->disks + i + 1,
+ sizeof(*def->disks) *
+ (def->ndisks - (i + 1)));
+ def->ndisks--;
+ if (VIR_REALLOC_N(def->disks, def->ndisks) < 0) {
+ /* ignore, harmless */
+ }
+ } else {
+ VIR_FREE(def->disks);
+ def->ndisks = 0;
+ }
+}
But since this is just code motion for future use, it looks fine to me.
ACK, and I went ahead and pushed it with those edits.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4:43 p.m.
New subject: [libvirt] [PATCH] qemu: use better types
* src/qemu/qemu_driver.c (qemudFindDisk): Mark parameter const.
(qemudShrinkDisks): Mark parameter unsigned.
---
> +static inline int qemudFindDisk(virDomainDefPtr def, char *dst)
dst can be const char *.
> +static inline void qemudShrinkDisks(virDomainDefPtr def, int i)
And i can be unsigned (better yet, size_t).
But since this is just code motion for future use, it looks fine to me.
ACK, and I went ahead and pushed it with those edits.
Silly me; I made the changes in my editor, but did not save them, so I
ended up pushing your patch as written. I'm pushing this followup as
trivial, to match my intentions.
src/qemu/qemu_driver.c | 4 ++-- 1 files changed, 2 insertions(+), 2
deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 90b9089..cbf6602 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7866,7 +7866,7 @@ cleanup:
}
-static inline int qemudFindDisk(virDomainDefPtr def, char *dst)
+static inline int qemudFindDisk(virDomainDefPtr def, const char *dst)
{
int i;
@@ -7879,7 +7879,7 @@ static inline int qemudFindDisk(virDomainDefPtr def, char *dst)
return -1;
}
-static inline void qemudShrinkDisks(virDomainDefPtr def, int i)
+static inline void qemudShrinkDisks(virDomainDefPtr def, size_t i)
{
if (def->ndisks > 1) {
memmove(def->disks + i,
--
1.6.6.1
9:52 a.m.
New subject: [libvirt] [PATCH v2 2/2] Implement SCSI disk unplugging
With the introduction of the generic qemu device model, unplugging
SCSI disks works like a charm, so support it in libvirt.
* src/qemu/qemu_driver.c: Add qemudDomainDetachSCSIDiskDevice() to do the
unplugging, extend qemudDomainDetachDeviceAdd().
Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer(a)siemens.com>
---
src/qemu/qemu_driver.c | 60 +++++++++++++++++++++++++++++++++++++++++++++--
1 files changed, 57 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 63ca57c..7321960 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7940,6 +7940,51 @@ static int qemudDomainDetachPciDiskDevice(struct qemud_driver
*driver,
qemudShrinkDisks(vm->def, i);
+ if (driver->securityDriver &&
+ driver->securityDriver->domainRestoreSecurityImageLabel &&
+ driver->securityDriver->domainRestoreSecurityImageLabel(vm,
dev->data.disk) < 0)
+ VIR_WARN("Unable to restore security label on %s",
dev->data.disk->src);
+
+ ret = 0;
+
+cleanup:
+ return ret;
+}
+
+static int qemudDomainDetachSCSIDiskDevice(struct qemud_driver *driver,
+ virDomainObjPtr vm,
+ virDomainDeviceDefPtr dev,
+ unsigned long long qemuCmdFlags)
+{
+ int i, ret = -1;
+ virDomainDiskDefPtr detach = NULL;
+ qemuDomainObjPrivatePtr priv = vm->privateData;
+
+ i = qemudFindDisk(vm->def, dev->data.disk->dst);
+
+ if (i < 0) {
+ qemuReportError(VIR_ERR_OPERATION_FAILED,
+ _("disk %s not found"), dev->data.disk->dst);
+ goto cleanup;
+ }
+
+ if (!(qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) {
+ qemuReportError(VIR_ERR_OPERATION_FAILED,
+ _("Underlying qemu does not support SCSI disk removal"));
+ goto cleanup;
+ }
+
+ detach = vm->def->disks[i];
+
+ qemuDomainObjEnterMonitorWithDriver(driver, vm);
+ if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) {
+ qemuDomainObjExitMonitor(vm);
+ goto cleanup;
+ }
+ qemuDomainObjExitMonitorWithDriver(driver, vm);
+
+ qemudShrinkDisks(vm->def, i);
+
virDomainDiskDefFree(detach);
if (driver->securityDriver &&
@@ -8393,9 +8438,18 @@ static int qemudDomainDetachDevice(virDomainPtr dom,
goto endjob;
if (dev->type == VIR_DOMAIN_DEVICE_DISK &&
- dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_DISK &&
- dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) {
- ret = qemudDomainDetachPciDiskDevice(driver, vm, dev, qemuCmdFlags);
+ dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
+ if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) {
+ ret = qemudDomainDetachPciDiskDevice(driver, vm, dev, qemuCmdFlags);
+ }
+ else if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) {
+ ret = qemudDomainDetachSCSIDiskDevice(driver, vm, dev,
+ qemuCmdFlags);
+ }
+ else {
+ qemuReportError(VIR_ERR_NO_SUPPORT,
+ "%s", _("This type of disk cannot be hot
unplugged"));
+ }
} else if (dev->type == VIR_DOMAIN_DEVICE_NET) {
ret = qemudDomainDetachNetDevice(driver, vm, dev, qemuCmdFlags);
} else if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) {
--
1.6.4
4:57 p.m.
New subject: [libvirt] [PATCH v2 2/2] Implement SCSI disk unplugging
On 05/05/2010 08:52 AM, Wolfgang Mauerer wrote:
With the introduction of the generic qemu device model, unplugging
SCSI disks works like a charm, so support it in libvirt.
* src/qemu/qemu_driver.c: Add qemudDomainDetachSCSIDiskDevice() to do the
unplugging, extend qemudDomainDetachDeviceAdd().
Good, you addressed Daniel's comments.
@@ -7940,6 +7940,51 @@ static int
qemudDomainDetachPciDiskDevice(struct qemud_driver *driver,
qemudShrinkDisks(vm->def, i);
+ if (driver->securityDriver &&
+ driver->securityDriver->domainRestoreSecurityImageLabel &&
+ driver->securityDriver->domainRestoreSecurityImageLabel(vm,
dev->data.disk) < 0)
+ VIR_WARN("Unable to restore security label on %s",
dev->data.disk->src);
+
+ ret = 0;
+
+cleanup:
+ return ret;
+}
Sometimes diff algorithms pick the oddest points to start at, don't they :)
+
+static int qemudDomainDetachSCSIDiskDevice(struct qemud_driver *driver,
+ virDomainObjPtr vm,
+ virDomainDeviceDefPtr dev,
+ unsigned long long qemuCmdFlags)
Your spacing here wasn't quite right ('SCSI' is longer than 'Pci', so
the '(' is one column further over), so I adjusted that (and this time,
I saved my editor and amended the patch before sending this email). You
also had some trailing blanks, caught by 'make syntax-check'.
ACK, and pushed.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5315
days inactive
5317
days old
7 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Wolfgang Mauerer