12:41 p.m.
Originally, ejecting a cdrom from a qemu guest entailed
passing 'eject cdrom' to the monitor. But since qemu
added the -drive option, more than one cdrom can be
specified, so just using 'cdrom' isn't explicit enough.
The attached patch updates media change/eject to use
the current qemu syntax. The new generated commands
look something like "eject ide0-cd1", with the name
derived from device target and bus type.
While I was in there I added support for inserting/
ejecting media from scsi cdroms and floppy devices.
This is built around my previous two patches:
- Fix cd eject segfault
- Attempt to detect cdrom change failures
Thanks,
Cole
5:48 a.m.
On Mon, Aug 25, 2008 at 01:41:24PM -0400, Cole Robinson wrote:
Originally, ejecting a cdrom from a qemu guest entailed
passing 'eject cdrom' to the monitor. But since qemu
added the -drive option, more than one cdrom can be
specified, so just using 'cdrom' isn't explicit enough.
The attached patch updates media change/eject to use
the current qemu syntax. The new generated commands
look something like "eject ide0-cd1", with the name
derived from device target and bus type.
I'm a bit unclear -- will this break for people using older versions
of qemu/kvm?
Rich.
--
Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones
Read my OCaml programming blog: http://camltastic.blogspot.com/
Fedora now supports 64 OCaml packages (the OPEN alternative to F#)
http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora
5:57 a.m.
On Wed, Aug 27, 2008 at 11:48:27AM +0100, Richard W.M. Jones wrote:
On Mon, Aug 25, 2008 at 01:41:24PM -0400, Cole Robinson wrote:
> Originally, ejecting a cdrom from a qemu guest entailed
> passing 'eject cdrom' to the monitor. But since qemu
> added the -drive option, more than one cdrom can be
> specified, so just using 'cdrom' isn't explicit enough.
>
> The attached patch updates media change/eject to use
> the current qemu syntax. The new generated commands
> look something like "eject ide0-cd1", with the name
> derived from device target and bus type.
I'm a bit unclear -- will this break for people using older versions
of qemu/kvm?
Looks very much like it will. If we're lucky this change in monitor
syntax occurred at the time the -drive command was introduced. In which
case its easy to conditionalize based on the whether that's in use or
not.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
6:01 a.m.
On Mon, Aug 25, 2008 at 01:41:24PM -0400, Cole Robinson wrote:
+
+ int idx = virDiskNameToIndex(newdisk->dst);
+ switch (newdisk->bus) {
+ /* Assume that if we are here, device targets don't exceed hypervisor
+ * limits, and we are already an appropriate device type */
+ case VIR_DOMAIN_DISK_BUS_IDE:
+ /* Device name of the form 'ide{0-1}-cd{0-1}' */
+ ret = asprintf(&devname, "ide%d-cd%d", ((idx - (idx % 2)) /
2),
+ (idx % 2));
+ break;
+ case VIR_DOMAIN_DISK_BUS_SCSI:
+ /* Device name of the form 'scsi{bus#}-cd{0-6}
+ * Each bus holds seven devs */
+ ret = asprintf(&devname, "scsi%d-cd%d", ((idx - (idx % 7)) /
7),
+ (idx % 7));
+ break;
+ case VIR_DOMAIN_DISK_BUS_FDC:
+ /* Device name is 'floppy{0-1}' */
+ ret = asprintf(&devname, "floppy%d", idx);
+ break;
+
+ default:
+ qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
+ _("Cannot hotplug device with bus
'%s'"),
+ virDomainDiskBusTypeToString(newdisk->bus));
+ return -1;
+ }
I think this block could be re-factored a little into one or more helper
methods, because I think we'll need to re-use this for hot-unplug of
disks. I'd suggest a helper to turn the plain integer index into the
(bus,device) index pair
virDiskNameToBusDeviceIndex(virDomainDiskDefPtr disk,
int *busIdx,
int *devIdx);
And then a QEMU specific function
char *virQEMUDeviceName(virDomainDiskDefPtr disk);
and have this call virDiskNameToBusDeviceIndex() and return the 'ide1-2'
type string.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
4:32 p.m.
Daniel P. Berrange wrote:
On Mon, Aug 25, 2008 at 01:41:24PM -0400, Cole Robinson wrote:
> +
> + int idx = virDiskNameToIndex(newdisk->dst);
> + switch (newdisk->bus) {
> + /* Assume that if we are here, device targets don't exceed hypervisor
> + * limits, and we are already an appropriate device type */
> + case VIR_DOMAIN_DISK_BUS_IDE:
> + /* Device name of the form 'ide{0-1}-cd{0-1}' */
> + ret = asprintf(&devname, "ide%d-cd%d", ((idx - (idx % 2))
/ 2),
> + (idx % 2));
> + break;
> + case VIR_DOMAIN_DISK_BUS_SCSI:
> + /* Device name of the form 'scsi{bus#}-cd{0-6}
> + * Each bus holds seven devs */
> + ret = asprintf(&devname, "scsi%d-cd%d", ((idx - (idx % 7))
/ 7),
> + (idx % 7));
> + break;
> + case VIR_DOMAIN_DISK_BUS_FDC:
> + /* Device name is 'floppy{0-1}' */
> + ret = asprintf(&devname, "floppy%d", idx);
> + break;
> +
> + default:
> + qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
> + _("Cannot hotplug device with bus
'%s'"),
> + virDomainDiskBusTypeToString(newdisk->bus));
> + return -1;
> + }
I think this block could be re-factored a little into one or more helper
methods, because I think we'll need to re-use this for hot-unplug of
disks. I'd suggest a helper to turn the plain integer index into the
(bus,device) index pair
virDiskNameToBusDeviceIndex(virDomainDiskDefPtr disk,
int *busIdx,
int *devIdx);
And then a QEMU specific function
char *virQEMUDeviceName(virDomainDiskDefPtr disk);
and have this call virDiskNameToBusDeviceIndex() and return the 'ide1-2'
type string.
Okay, second cut attached. I followed your recommendations:
virDiskNameToBusDeviceIndex added to domain_conf.c
qemudDiskDeviceName added to qemu_driver.c
I also hopefully solved the back compatibility issue. Seems
-drive was added to qemu in Dec 07 (qemu commit 3759) and the
device naming scheme was updated at the end of that month (qemu
commit 3846), so checking for -drive should be a sufficient
indicator that we need to use the new device name format. So
if drive was detected (using the already present qemu_conf
flag), we revert to the old style cdrom/fda naming style.
I tested this on f8 using plain qemu (which lacks the -drive
option) and it appeared to work as expected for cdrom and
floppy devices.
Thanks,
Cole
2:36 a.m.
+1 for the second version.
Rich.
--
Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top
4:17 a.m.
On Wed, Aug 27, 2008 at 05:32:21PM -0400, Cole Robinson wrote:
Daniel P. Berrange wrote:
>
> I think this block could be re-factored a little into one or more helper
> methods, because I think we'll need to re-use this for hot-unplug of
> disks. I'd suggest a helper to turn the plain integer index into the
> (bus,device) index pair
>
> virDiskNameToBusDeviceIndex(virDomainDiskDefPtr disk,
> int *busIdx,
> int *devIdx);
>
> And then a QEMU specific function
>
> char *virQEMUDeviceName(virDomainDiskDefPtr disk);
>
> and have this call virDiskNameToBusDeviceIndex() and return the 'ide1-2'
> type string.
>
Okay, second cut attached. I followed your recommendations:
virDiskNameToBusDeviceIndex added to domain_conf.c
qemudDiskDeviceName added to qemu_driver.c
I also hopefully solved the back compatibility issue. Seems
-drive was added to qemu in Dec 07 (qemu commit 3759) and the
device naming scheme was updated at the end of that month (qemu
commit 3846), so checking for -drive should be a sufficient
indicator that we need to use the new device name format. So
if drive was detected (using the already present qemu_conf
flag), we revert to the old style cdrom/fda naming style.
Excellant - that intermediate month can be argued as a bug
because people could add multiple CDROMs and the monitor
provided no way to select between them. So this check for
-drive is sufficient.
+/* Translates a device name of the form (regex)
"[fhv]d[a-z]+" into
+ * the corresponding bus,index combination (e.g. sda => (0,0), sdi (1,1),
+ * hdd => (1,1), vdaa => (0,27))
+ * @param disk The disk device
+ * @param busIdx parsed bus number
+ * @param devIdx parsed device number
+ * @return 0 on success, -1 on failure
+ */
+int virDiskNameToBusDeviceIndex(virDomainDiskDefPtr disk,
+ int *busIdx,
+ int *devIdx) {
+
+ int idx = virDiskNameToIndex(disk->dst);
Minor bug here
if (idx < 0)
return -1;
+
+ switch (disk->bus) {
+ case VIR_DOMAIN_DISK_BUS_IDE:
+ *busIdx = ((idx - (idx % 2)) / 2);
+ *devIdx = (idx % 2);
+ break;
+ case VIR_DOMAIN_DISK_BUS_SCSI:
+ *busIdx = ((idx - (idx % 7)) / 7);
+ *devIdx = (idx % 7);
+ break;
+ case VIR_DOMAIN_DISK_BUS_FDC:
+ case VIR_DOMAIN_DISK_BUS_VIRTIO:
Could also add
case VIR_DOMAIN_DISK_BUS_USB:
case VIR_DOMAIN_DISK_BUS_XEN:
+ default:
+ *busIdx = 0;
+ *devIdx = idx;
+ break;
+ }
+
+ return 0;
+}
+/* Return the disks name for use in monitor commands */
+static char *qemudDiskDeviceName(virDomainPtr dom,
+ virDomainDiskDefPtr disk) {
+
+ int busid, devid;
+ int ret;
+ char *devname;
+
+ virDiskNameToBusDeviceIndex(disk, &busid, &devid);
if (virDiskNameToBusDeviceIndex(disk, &busid, &devid) < 0)
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
_("cannot convertor disk name to bus/device
index"));
return NULL;
}
+
+ switch (disk->bus) {
+ case VIR_DOMAIN_DISK_BUS_IDE:
+ ret = asprintf(&devname, "ide%d-cd%d", busid, devid);
+ break;
+ case VIR_DOMAIN_DISK_BUS_SCSI:
+ ret = asprintf(&devname, "scsi%d-cd%d", busid, devid);
+ break;
+ case VIR_DOMAIN_DISK_BUS_FDC:
+ ret = asprintf(&devname, "floppy%d", devid);
+ break;
+ case VIR_DOMAIN_DISK_BUS_VIRTIO:
+ ret = asprintf(&devname, "virtio%d", devid);
+ break;
+ default:
+ qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
+ _("Unknown disk name mapping for bus
'%s'"),
+ virDomainDiskBusTypeToString(disk->bus));
s/Unknown/Unsupported/ because we know about Xen / USB, merely don't
support them.
+ return NULL;
+ }
+
+ if (ret == -1) {
+ qemudReportError(dom->conn, NULL, NULL, VIR_ERR_NO_MEMORY, NULL);
+ return NULL;
+ }
+
+ return devname;
+}
+
+static int qemudDomainChangeEjectableMedia(virDomainPtr dom,
+ virDomainDeviceDefPtr dev)
+{
struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
+ virDomainObjPtr vm = virDomainFindByUUID(driver->domains, dom->uuid);
+ virDomainDiskDefPtr origdisk, newdisk;
char *cmd, *reply, *safe_path;
+ char *devname = NULL;
+ int qemuCmdFlags;
+
+ if (!vm) {
+ qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
+ "%s", _("no domain with matching uuid"));
+ return -1;
+ }
+
+ newdisk = dev->data.disk;
+ origdisk = vm->def->disks;
+ while (origdisk) {
+ if (origdisk->bus == newdisk->bus &&
+ STREQ(origdisk->dst, newdisk->dst))
+ break;
+ origdisk = origdisk->next;
+ }
+
+ if (!origdisk) {
+ qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
+ _("No device with bus '%s' and target
'%s'"),
+ virDomainDiskBusTypeToString(newdisk->bus),
+ newdisk->dst);
+ return -1;
+ }
+
+ if (qemudExtractVersionInfo(vm->def->emulator,
+ NULL,
+ &qemuCmdFlags) < 0) {
+ qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
+ _("Cannot determine QEMU argv syntax %s"),
+ vm->def->emulator);
+ return -1;
+ }
+
+ if (qemuCmdFlags & QEMUD_CMD_FLAG_DRIVE) {
+ devname = qemudDiskDeviceName(dom, newdisk);
+ } else {
+ /* Back compat for no -drive option */
+ if (newdisk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY)
+ devname = strdup(newdisk->dst);
+ else if (newdisk->device == VIR_DOMAIN_DISK_DEVICE_CDROM &&
+ STREQ(newdisk->dst, "hdc"))
+ devname = strdup("cdrom");
+ else {
+ qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
+ _("Emulator version does not support removable "
+ "media for device '%s' and target
'%s'"),
+ virDomainDiskDeviceTypeToString(newdisk->device),
+ newdisk->dst);
+ return -1;
+ }
+ }
+
+ if (!devname) {
+ qemudReportError(dom->conn, NULL, NULL, VIR_ERR_NO_MEMORY, NULL);
You don't want to call qemudReportError here, because qemudDiskDeviceName()
will already have raised an error. You need to push this particular error
raise up into the 'else { ... }' clause above.
+ return -1;
+ }
if (newdisk->src) {
safe_path = qemudEscapeMonitorArg(newdisk->src);
if (!safe_path) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
"%s", _("out of memory"));
+ VIR_FREE(devname);
return -1;
}
- if (asprintf (&cmd, "change %s \"%s\"",
- /* XXX qemu may support multiple CDROM in future */
- /* olddisk->dst */ "cdrom",
- safe_path) == -1) {
+ if (asprintf (&cmd, "change %s \"%s\"", devname,
safe_path) == -1) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
"%s", _("out of memory"));
I know this was here already, but we should change it to the OOM error code
qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_MEMORY...
VIR_FREE(safe_path);
+ VIR_FREE(devname);
return -1;
}
VIR_FREE(safe_path);
- } else if (asprintf(&cmd, "eject cdrom") == -1) {
+ } else if (asprintf(&cmd, "eject %s", devname) == -1) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
"%s", _("out of memory"));
Likewise here..
+ VIR_FREE(devname);
return -1;
}
+ VIR_FREE(devname);
if (qemudMonitorCommand(driver, vm, cmd, &reply) < 0) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
@@ -2984,7 +3082,7 @@ static int qemudDomainChangeCDROM(virDomainPtr dom,
@@ -3186,10 +3251,11 @@ static int
qemudDomainAttachDevice(virDomainPtr dom,
}
if (dev->type == VIR_DOMAIN_DEVICE_DISK &&
- dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
- ret = qemudDomainAttachCdromDevice(dom, dev);
+ (dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM ||
+ dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY)) {
+ ret = qemudDomainChangeEjectableMedia(dom, dev);
} else if (dev->type == VIR_DOMAIN_DEVICE_DISK &&
- dev->data.disk->device == VIR_DOMAIN_DEVICE_DISK &&
+ dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_DISK &&
OOpps, nice catch for that typo !
dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
ret = qemudDomainAttachUsbMassstorageDevice(dom, dev);
} else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV &&
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
1:50 a.m.
Cole Robinson <crobinso(a)redhat.com> wrote:
...
Okay, second cut attached. I followed your recommendations:
...
With all of Dan's comments addressed, this should be fine.
diff --git a/src/domain_conf.c b/src/domain_conf.c
...
+int virDiskNameToBusDeviceIndex(virDomainDiskDefPtr disk,
...
+ switch (disk->bus) {
+ case VIR_DOMAIN_DISK_BUS_IDE:
+ *busIdx = ((idx - (idx % 2)) / 2);
+ *devIdx = (idx % 2);
+ break;
+ case VIR_DOMAIN_DISK_BUS_SCSI:
+ *busIdx = ((idx - (idx % 7)) / 7);
+ *devIdx = (idx % 7);
+ break;
It doesn't affect correctness, but you can remove the
unnecessary "- (idx % 2)" and "- (idx % 7)" expressions:
switch (disk->bus) {
case VIR_DOMAIN_DISK_BUS_IDE:
*busIdx = idx / 2;
*devIdx = idx % 2;
break;
case VIR_DOMAIN_DISK_BUS_SCSI:
*busIdx = idx / 7;
*devIdx = idx % 7;
break;
9:27 a.m.
Jim Meyering wrote:
Cole Robinson <crobinso(a)redhat.com> wrote:
...
> Okay, second cut attached. I followed your recommendations:
...
With all of Dan's comments addressed, this should be fine.
> diff --git a/src/domain_conf.c b/src/domain_conf.c
...
> +int virDiskNameToBusDeviceIndex(virDomainDiskDefPtr disk,
...
> + switch (disk->bus) {
> + case VIR_DOMAIN_DISK_BUS_IDE:
> + *busIdx = ((idx - (idx % 2)) / 2);
> + *devIdx = (idx % 2);
> + break;
> + case VIR_DOMAIN_DISK_BUS_SCSI:
> + *busIdx = ((idx - (idx % 7)) / 7);
> + *devIdx = (idx % 7);
> + break;
It doesn't affect correctness, but you can remove the
unnecessary "- (idx % 2)" and "- (idx % 7)" expressions:
switch (disk->bus) {
case VIR_DOMAIN_DISK_BUS_IDE:
*busIdx = idx / 2;
*devIdx = idx % 2;
break;
case VIR_DOMAIN_DISK_BUS_SCSI:
*busIdx = idx / 7;
*devIdx = idx % 7;
break;
Okay, latest cut. This addresses the above piece pointed out
by Jim and all of Dan's comments.
Thanks,
Cole
9:28 a.m.
On Fri, Aug 29, 2008 at 10:27:04AM -0400, Cole Robinson wrote:
Jim Meyering wrote:
> Cole Robinson <crobinso(a)redhat.com> wrote:
> ...
>> Okay, second cut attached. I followed your recommendations:
> ...
>
> With all of Dan's comments addressed, this should be fine.
>
>> diff --git a/src/domain_conf.c b/src/domain_conf.c
> ...
>> +int virDiskNameToBusDeviceIndex(virDomainDiskDefPtr disk,
> ...
>> + switch (disk->bus) {
>> + case VIR_DOMAIN_DISK_BUS_IDE:
>> + *busIdx = ((idx - (idx % 2)) / 2);
>> + *devIdx = (idx % 2);
>> + break;
>> + case VIR_DOMAIN_DISK_BUS_SCSI:
>> + *busIdx = ((idx - (idx % 7)) / 7);
>> + *devIdx = (idx % 7);
>> + break;
>
> It doesn't affect correctness, but you can remove the
> unnecessary "- (idx % 2)" and "- (idx % 7)" expressions:
>
> switch (disk->bus) {
> case VIR_DOMAIN_DISK_BUS_IDE:
> *busIdx = idx / 2;
> *devIdx = idx % 2;
> break;
> case VIR_DOMAIN_DISK_BUS_SCSI:
> *busIdx = idx / 7;
> *devIdx = idx % 7;
> break;
Okay, latest cut. This addresses the above piece pointed out
by Jim and all of Dan's comments.
ACK from me now.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
11:20 a.m.
Cole Robinson wrote:
Jim Meyering wrote:
> Cole Robinson <crobinso(a)redhat.com> wrote:
> ...
>> Okay, second cut attached. I followed your recommendations:
> ...
>
> With all of Dan's comments addressed, this should be fine.
>
>> diff --git a/src/domain_conf.c b/src/domain_conf.c
> ...
>> +int virDiskNameToBusDeviceIndex(virDomainDiskDefPtr disk,
> ...
>> + switch (disk->bus) {
>> + case VIR_DOMAIN_DISK_BUS_IDE:
>> + *busIdx = ((idx - (idx % 2)) / 2);
>> + *devIdx = (idx % 2);
>> + break;
>> + case VIR_DOMAIN_DISK_BUS_SCSI:
>> + *busIdx = ((idx - (idx % 7)) / 7);
>> + *devIdx = (idx % 7);
>> + break;
> It doesn't affect correctness, but you can remove the
> unnecessary "- (idx % 2)" and "- (idx % 7)" expressions:
>
> switch (disk->bus) {
> case VIR_DOMAIN_DISK_BUS_IDE:
> *busIdx = idx / 2;
> *devIdx = idx % 2;
> break;
> case VIR_DOMAIN_DISK_BUS_SCSI:
> *busIdx = idx / 7;
> *devIdx = idx % 7;
> break;
Okay, latest cut. This addresses the above piece pointed out
by Jim and all of Dan's comments.
Thanks,
Cole
I've committed this latest version.
Thanks,
Cole
5923
days inactive
5932
days old
11 comments
4 participants
participants (4)
-
Cole Robinson -
Daniel P. Berrange -
Jim Meyering -
Richard W.M. Jones