Re: [Qemu-devel] [PATCH 16/18] xen: automatically create XenQdiskDevice-s

[Paul Durrant]

> -----Original Message-----
> From: Anthony PERARD [mailto:address@hidden
> Sent: 04 December 2018 16:41
> To: Paul Durrant <address@hidden>
> Cc: address@hidden; address@hidden; xen-
> address@hidden; Kevin Wolf <address@hidden>; Max Reitz
> <address@hidden>; Stefano Stabellini <address@hidden>
> Subject: Re: [PATCH 16/18] xen: automatically create XenQdiskDevice-s
> 
> On Wed, Nov 21, 2018 at 03:12:09PM +0000, Paul Durrant wrote:
> > This patch adds a creator function for XenQdiskDevice-s so that they can
> > be created automatically when the Xen toolstack instantiates a new
> > PV backend. When the XenQdiskDevice is created this way it is also
> > necessary to create a drive which matches the configuration that the Xen
> > toolstack has written into xenstore. This drive is marked 'auto_del' so
> > that it will be removed when the XenQdiskDevice is destroyed. Also, for
> > compatibilitye with the legacy 'xen_disk' implementation, an iothread
> > is automatically created for the new XenQdiskDevice. This will also be
> > removed when he XenQdiskDevice is destroyed.
> 
> "the XenQdiskDevice"
> 
> [...]
> > +    qemu_opt_set(drive_opts, "file.locking", "off", &local_err);
> 
> That looks new, compared to the xen_disk.c implementation. Maybe that
> should be mention in the commit message.
> 

It's necessary to avoid problems when an emulated device is also present. The 
xen_disk code avoided the issue by basically bypassing the checks and stooging 
into the middle of the block code. I'll add a comment to the code saying why 
locking needs to be off.

> 
> [..]
> 
> > +static void xen_qdisk_device_create(BusState *bus, const char *name,
> > +                                    QDict *opts, Error **errp)
> > +{
> [...]
> > +    iothread = iothread_create(vdev, &error_abort);
> 
> I would just propagate the error, since iothread could fail for external
> reason. No need to crash qemu while a VM is running.

True.

> 
> > +
> > +    dev = qdev_create(bus, TYPE_XEN_QDISK_DEVICE);
> > +
> > +    qdev_prop_set_string(dev, "vdev", vdev);
> > +
> > +    if (XEN_QDISK_DEVICE(dev)->vdev.number != number) {
> > +        error_setg(errp, "invalid dev parameter '%s'", vdev);
> > +        goto unref;
> > +    }
> > +
> > +    qdev_prop_set_drive(dev, "drive", blk, &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        error_prepend(errp, "failed to set 'drive': ");
> > +        goto unref;
> > +    }
> > +
> > +    XEN_QDISK_DEVICE(dev)->auto_iothread = iothread;
> > +
> > +    qdev_init_nofail(dev);
> 
> That function shouldn't be use during hotplug. But I'm not sure what
> should be done instead, probably object_property_set_bool(..., true
> "realized") and check for error.

Ok, I'll do that.

  Paul

> 
> 
> Thanks,
> 
> --
> Anthony PERARD