[libvirt] [PATCH 3/6] Implement virDomainUpdateDeviceFlags API in all drivers with media change

[Daniel P. Berrange]

On Mon, Mar 22, 2010 at 02:58:16PM -0600, Eric Blake wrote:
> On 03/22/2010 01:05 PM, Daniel P. Berrange wrote:
> > To allow the new virDomainUpdateDeviceFlags() API to be universally
> > used with all drivers, this patch adds an impl to all the current
> > drivers which support CDROM or Floppy disk media change via the
> > current virDomainAttachDeviceFlags API
> > 
> > +    switch (dev->type) {
> > +    case VIR_DOMAIN_DEVICE_DISK:
> ...
> > +
> > +        switch (dev->data.disk->device) {
> > +        case VIR_DOMAIN_DISK_DEVICE_CDROM:
> > +        case VIR_DOMAIN_DISK_DEVICE_FLOPPY:
> > +            ret = qemudDomainChangeEjectableMedia(driver, vm, dev->data.disk);
> > +            if (ret == 0)
> > +                dev->data.disk = NULL;
> > +            break;
> > +
> > +
> > +        default:
> > +            qemuReportError(VIR_ERR_NO_SUPPORT,
> > +                            _("disk bus '%s' cannot be updated."),
> > +                            virDomainDiskBusTypeToString(dev->data.disk->bus));
> > +            break;
> 
> This break only exits the innermost switch...
> 
> > +        }
> > +
> > +        if (ret != 0 && cgroup) {
> > +            virCgroupDenyDevicePath(cgroup,
> > +                                    dev->data.disk->src);
> > +        }
> > +        break;
> 
> ...is it still safe to call this after reporting an error like that,
> before breaking from the outermost switch?

Yep, this is correct. THe virCgroupDenyDevicePath() function has to be called
on all error scenarios here, both from the "default:" case, and error in the
previous  'VIR_DOMAIN_DISK_DEVICE_FLOPPY:' case, so that it removes the ACL
that was setup immediately before the switch.

> > -static int vboxDomainAttachDevice(virDomainPtr dom, const char *xml) {
> > +static int vboxDomainAttachDeviceImpl(virDomainPtr dom, const char *xml, int mediaChangeOnly ATTRIBUTE_UNUSED) {
> 
> 80 columns?
> 
> > +        /* Only live config can be changed if xendConfigVersion < 3 */
> > +        if (priv->xendConfigVersion < 3 &&
> > +            (flags != VIR_DOMAIN_DEVICE_MODIFY_CURRENT ||
> > +             flags != VIR_DOMAIN_DEVICE_MODIFY_LIVE)) {
> > +            virXendError(domain->conn, VIR_ERR_OPERATION_INVALID, "%s",
> > +                         _("Xend version does not support modifying "
> > +                           "persisted config"));
> 
> I think 'persistent' sounds better than 'persisted' as an adjective
> (multiple instances).

This was just a copied diagnostic from code higher up


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 :|