Thank you for review.
On Thu, 24 Feb 2011 14:25:24 +0800
Hu Tao <hutao@xxxxxxxxxxxxxx> wrote:
> On Thu, Feb 24, 2011 at 01:06:36PM +0900, KAMEZAWA Hiroyuki wrote:
> > >From d6c65102224c493f67008b6521b0115d798f5a67 Mon Sep 17 00:00:00 2001
> > From: KAMEZAWA Hiroyuki <kamezawa@bluextal.(none)>
> > Date: Thu, 24 Feb 2011 12:14:25 +0900
> > Subject: [PATCH 1/2] libvirt/qemu - support updating inactive domains.
> >
> > Now, virsh attach-disk/detach-disk has --persistent option and
> > it can update XML definition of inactive domains. But, it's only
> > supported in Xen.
> >
> > This patch adds support for attach-disk/detach-disk for qemu.
> >
> > Changelog v1->v2:
> > - fixed TABs
> > - fixed PCI address assignment for VIRTIO driver.
> >
> > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
> > ---
> > src/qemu/qemu_driver.c | 168 +++++++++++++++++++++++++++++++++++++++++++++---
> > 1 files changed, 158 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 8b15a3e..e75478a 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -4085,14 +4085,164 @@ cleanup:
> > return ret;
> > }
> >
> > -static int qemudDomainAttachDeviceFlags(virDomainPtr dom,
> > +static int qemuDomainFindDiskByName(virDomainDefPtr vmdef, const char *name)
> > +{
> > + virDomainDiskDefPtr vdisk;
> > + int i;
> > +
> > + for (i = 0; i < vmdef->ndisks; i++) {
> > + vdisk = vmdef->disks[i];
> > + if (!strcmp(vdisk->dst, name))
>
> Maybe STREQ() is better here as other libvirt code uses STREQ() rather
> than strcmp().
>
Hmm, ok.
> > + return i;
> > + }
> > + return -1;
> > +}
> > +/*
> > + * Attach a device given by XML, the change will be persistent
> > + * and domain XML definition file is updated.
> > + */
> > +static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef,
> > + virDomainDeviceDefPtr newdev)
> > +{
> > + virDomainDiskDefPtr disk;
> > +
> > + /* At first, check device confliction */
> > + switch(newdev->type) {
> > + case VIR_DOMAIN_DEVICE_DISK:
> > + disk = newdev->data.disk;
> > + if (qemuDomainFindDiskByName(vmdef, disk->dst) >= 0) {
> > + qemuReportError(VIR_ERR_INVALID_ARG,
> > + _("target %s already exists."), disk->dst);
> > + return -1;
> > + }
> > +
> > + if (virDomainDiskInsert(vmdef, disk)) {
> > + virReportOOMError();
>
> I think it's better to report OOM error by virDomainDiskInsert() itself,
> otherwise every caller has to call virReportOOMError() when fails.
>
ok, will add a patch for conf/domain_conf.c
> > + return -1;
> > + }
> > + if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) {
> > + /*
> > + * Address/Drive information is NULL. If virtio, PCI address
> > + * shoule be fixed. Other devices as IDE, SCSI...will get ID
> > + * automatically
> > + */
> > + qemuDomainAssignPCIAddresses(vmdef);
> > +
> > + }
> > + newdev->data.disk = NULL;
> > + break;
> > + default:
> > + qemuReportError(VIR_ERR_INVALID_ARG, "%s",
> > + _("Sorry, the device is not suppored for now"));
> > + return -1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int qemuDomainDetachDevicePersistent(virDomainDefPtr vmdef,
> > + virDomainDeviceDefPtr device)
> > +{
> > + int x;
> > + virDomainDiskDefPtr disk;
> > +
> > + switch(device->type) {
> > + case VIR_DOMAIN_DEVICE_DISK:
> > + disk = device->data.disk;
> > + x = qemuDomainFindDiskByName(vmdef, disk->dst);
> > + if (x < 0) {
> > + qemuReportError(VIR_ERR_INVALID_ARG,
> > + _("target %s doesn't exist."), disk->dst);
> > + return -1;
> > + }
> > + virDomainDiskRemove(vmdef, x);
> > + break;
> > + default:
> > + qemuReportError(VIR_ERR_INVALID_ARG, "%s",
> > + _("Sorry, the device is not suppored for now"));
> > + return -1;
> > + }
> > + return 0;
> > +}
> > +
> > +static int qemuDomainModifyDevicePersistent(virDomainPtr dom,
> > const char *xml,
> > - unsigned int flags) {
> > - if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) {
> > - qemuReportError(VIR_ERR_OPERATION_INVALID,
> > - "%s", _("cannot modify the persistent configuration of a domain"));
> > + unsigned int attach)
> > +{
> > + struct qemud_driver *driver;
> > + virDomainDeviceDefPtr device;
> > + virDomainDefPtr vmdef;
> > + virDomainObjPtr vm;
> > + int ret = -1;
> > +
> > + if (!dom || !dom->conn || !dom->name || !xml) {
> > + qemuReportError(VIR_ERR_INVALID_ARG,
> > + _("internal error : %s"), __FUNCTION__);
> > + return -1;
> > + }
> > +
> > + if (dom->conn->flags & VIR_CONNECT_RO)
> > return -1;
>
> No need to check the RO flag here: it should be and is done in
> virDomainAttachDevice().
>
Ok, will remove.
> > +
> > + driver = dom->conn->privateData;
> > + qemuDriverLock(driver);
> > + vm = virDomainFindByUUID(&driver->domains, dom->uuid);
> > + if (!vm) {
> > + qemuReportError(VIR_ERR_NO_DOMAIN, _("cannot find domain '%s'"),
> > + dom->name);
> > + goto unlock_out;
> > + }
> > +
> > + if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) {
> > + /*
> > + * For now, just allow updating inactive domains. Further development
> > + * will allow updating both active domain and its config file at
> > + * the same time.
> > + */
> > + qemuReportError(VIR_ERR_INVALID_ARG,
> > + _("cannot update alive domain : %s"), __FUNCTION__);
>
> I think we can silently fail here. Currently this function (qemuDomainModifyDevicePersistent)
> is called only for modifying inactive domains.
>
It seems...."virDomainObjIsActive(vm) check" is lacked..
Above 'if' returns true only when timeout..
> > + goto endjob;
> > + }
> > + vmdef = virDomainObjGetPersistentDef(driver->caps, vm);
> > +
> > + if (!vmdef)
> > + goto endjob;
> > +
> > + device = virDomainDeviceDefParse(driver->caps,
> > + vmdef, xml, VIR_DOMAIN_XML_INACTIVE);
> > + if (!device)
> > + goto endjob;
> > +
> > + if (attach) {
> > + ret = qemuDomainAttachDevicePersistent(vmdef, device);
> > + if (ret < 0)
> > + goto out;
> > + } else {
> > + ret = qemuDomainDetachDevicePersistent(vmdef, device);
> > + if (ret < 0)
> > + goto out;
> > }
>
> This can be refined as:
>
> if (attach)
> ret = qemuDomainAttachDevicePersistent(vmdef, device);
> else
> ret = qemuDomainDetachDevicePersistent(vmdef, device);
>
> if (ret < 0)
> goto out;
>
will fix.
Thanks,
-Kame
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list