Am 28.11.2018 um 17:40 hat Paul Durrant geschrieben:
> > -----Original Message-----
> > From: Kevin Wolf [mailto:[email protected]]
> > Sent: 28 November 2018 16:35
> > To: Paul Durrant <[email protected]>
> > Cc: [email protected]; [email protected]; xen-
> > [email protected]; Stefano Stabellini <[email protected]>;
> > Anthony Perard <[email protected]>; Max Reitz <[email protected]>
> > Subject: Re: [PATCH 14/18] xen: add implementations of xen-qdisk connect
> > and disconnect functions...
> >
> > Am 21.11.2018 um 16:12 hat Paul Durrant geschrieben:
> > > ...and wire in the dataplane.
> > >
> > > This patch adds the remaining code to make the xen-qdisk XenDevice
> > > functional. The parameters that a block frontend expects to find are
> > > populated in the backend xenstore area, and the 'ring-ref' and
> > > 'event-channel' values specified in the frontend xenstore area are
> > > mapped/bound and used to set up the dataplane.
> > >
> > > Signed-off-by: Paul Durrant <[email protected]>
> > > ---
> > > Cc: Stefano Stabellini <[email protected]>
> > > Cc: Anthony Perard <[email protected]>
> > > Cc: Kevin Wolf <[email protected]>
> > > Cc: Max Reitz <[email protected]>
> > > ---
> > > hw/block/xen-qdisk.c | 140
> > +++++++++++++++++++++++++++++++++++++++++++++
> > > hw/xen/xen-bus.c | 12 ++--
> > > include/hw/xen/xen-bus.h | 8 +++
> > > include/hw/xen/xen-qdisk.h | 12 ++++
> > > 4 files changed, 166 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/hw/block/xen-qdisk.c b/hw/block/xen-qdisk.c
> > > index 35f7b70480..8c88393832 100644
> > > --- a/hw/block/xen-qdisk.c
> > > +++ b/hw/block/xen-qdisk.c
> > > @@ -9,6 +9,10 @@
> > > #include "qapi/visitor.h"
> > > #include "hw/hw.h"
> > > #include "hw/xen/xen-qdisk.h"
> > > +#include "sysemu/blockdev.h"
> > > +#include "sysemu/block-backend.h"
> > > +#include "sysemu/iothread.h"
> > > +#include "dataplane/xen-qdisk.h"
> > > #include "trace.h"
> > >
> > > static char *xen_qdisk_get_name(XenDevice *xendev, Error **errp)
> > > @@ -23,6 +27,11 @@ static void xen_qdisk_realize(XenDevice *xendev,
> > Error **errp)
> > > {
> > > XenQdiskDevice *qdiskdev = XEN_QDISK_DEVICE(xendev);
> > > XenQdiskVdev *vdev = &qdiskdev->vdev;
> > > + BlockConf *conf = &qdiskdev->conf;
> > > + DriveInfo *dinfo;
> > > + bool is_cdrom;
> > > + unsigned int info;
> > > + int64_t size;
> > >
> > > if (!vdev->valid) {
> > > error_setg(errp, "vdev property not set");
> > > @@ -30,13 +39,134 @@ static void xen_qdisk_realize(XenDevice *xendev,
> > Error **errp)
> > > }
> > >
> > > trace_xen_qdisk_realize(vdev->disk, vdev->partition);
> > > +
> > > + if (!conf->blk) {
> > > + error_setg(errp, "drive property not set");
> > > + return;
> > > + }
> > > +
> > > + if (!blk_is_inserted(conf->blk)) {
> > > + error_setg(errp, "device needs media, but drive is empty");
> > > + return;
> > > + }
> >
> > Hm, the code below suggests that you support CD-ROMs. Don't you want to
> > support media change as well then? Which would mean that you need to
> > support empty drives.
>
> Yes, that's a good point. I should get rid of that check.
Or rather apply it only to hard disks. And for empty CDs, you'll
probably need to create an empty BlockBackend (the !conf->blk case).
Just check the IDE and/or SCSI code for comparison.
> >
> > > + if (!blkconf_apply_backend_options(conf, blk_is_read_only(conf-
> > >blk),
> > > + false, errp)) {
> > > + return;
> > > + }
> > > +
> > > + if (!blkconf_geometry(conf, NULL, 65535, 255, 255, errp)) {
> > > + return;
> > > + }
> > > +
> > > + dinfo = blk_legacy_dinfo(conf->blk);
> > > + is_cdrom = (dinfo && dinfo->media_cd);
> >
> > It's called legacy for a reason. Don't use this in new devices.
> >
> > The proper way is to have two different devices for hard disks and CDs
> > (like scsi-hd and scsi-cd).
>
> ...or presumably I could have a property? The legacy init code could
> then set it based on the drive info.
Technically yes, but why would that be a good way to model things? I
mean, it's true that xen-qdisk is not real hardware, but I've never seen
any hardware that has a switch to decide whether it should behave as a
CD drive or a hard disk.
Both have very different characteristics (read-only with removable
media, or a single read-write disk), and the existing implementations
use two separate devices. So even if you're not convinced that users
will consider them different concepts (I am; and if they weren't
different concepts, you wouldn't need an is_cdrom variable), consistency
is still a good thing.
Kevin
