On Fri, Oct 17, 2008 at 04:24:52PM +0200, Guido Günther wrote:
On Fri, Oct 17, 2008 at 02:37:17PM +0200, Daniel Veillard wrote: [..snip..]
Looks fine, i just removed a couple of extra spaces at end of line before commiting :-) Thanks a lot for applying this so quickly! Attached is a patch that allows for unplugging of disks. To do that I added a token to virDomainDiskDef that holds the PCI bus/slot number [1] so we can indentify the device on unplug. It's a union since other busses/hypervisors might have other requirements. The token is meant as an internal handle and will never show up in an XML and should probably move up into virDomainDeviceDef. Comments are welcome.
In principle this looks just fine to me except a couple of stylistic issues: [...]
+++ b/src/domain_conf.h @@ -84,6 +84,12 @@ struct _virDomainDiskDef { int type; int device; int bus; + union { + struct { + int bus; + int slot; + } pci; + } token;
I'm not sure what the point of the token union is. Other adressing mechanism may be used still I think having the structure not unioned at this point makes things a bit clearer.
+static int qemudDomainDetchPciDiskDevice(virDomainPtr dom, virDomainDeviceDefPtr dev)
Well I don't really see an improvement in using "Detch" instead of "Detach", it's not like the identifier is much smaller, makes it less readable IMHO, ditto for qemudDomainDetchDevice() [...]
+ if (detach->token.pci.slot < 1 || detach->token.pci.bus != 0) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("disk %s cannot be detached"), detach->dst); + return -1; + }
shouldn't the error be "non PCI disk %s cannot be detached" instead ?
+ ret = asprintf(&cmd, "pci_del 0 %d", detach->token.pci.slot); + if (ret == -1) { + qemudReportError(dom->conn, NULL, NULL, VIR_ERR_NO_MEMORY, NULL); + return ret; + } + + if (qemudMonitorCommand(driver, vm, cmd, &reply) < 0) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("cannot detach disk %s"), detach->dst);
Live attach/detach operations are among the ones the harder to debug from an user perspective I would suggest to try to be as explicit as possible "failed to execute detach disk %s command"
+ VIR_FREE(cmd); + return -1; + } + + DEBUG ("pci_del reply: %s", reply); + /* If the command fails due to a wrong slot qemu prints: invalid slot, + * nothing is printed on success */ + if (strstr(reply, "invalid slot")) { + qemudReportError (dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("cannot detach disk %s"), detach->dst);
same thing if we have a hint about why this failed ... "cannot detach disk %s : invalid slot" [...]
+static int qemudDomainDetchDevice(virDomainPtr dom, + const char *xml) {
Detach :-)
+ if (dev->type == VIR_DOMAIN_DEVICE_DISK && + dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_DISK && + (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_SCSI || + dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO)) + ret = qemudDomainDetchPciDiskDevice(dom, dev); + else { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT, + "%s", _("this device type cannot be attached"));
or even better turn this positively as "only SCSI or virtio disk device can be attached dynamically" Those are just stylistic issues, I can apply the patch with those changed if you wish if you don't have time for a new patch, thanks, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/