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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list