Re: [Qemu-block] [PATCH 14/18] xen: add implementations of xen-qdisk connect and disconnect functions...

[Kevin Wolf]

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 <address@hidden>
> ---
> Cc: Stefano Stabellini <address@hidden>
> Cc: Anthony Perard <address@hidden>
> Cc: Kevin Wolf <address@hidden>
> Cc: Max Reitz <address@hidden>
> ---
>  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.

> +    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).

Kevin