On 18.11.2019 13:50, Denis Plotnikov wrote: > > > On 10.11.2019 22:08, Denis Plotnikov wrote: >> >> On 10.11.2019 22:03, Denis Plotnikov wrote: >>> This allows to change (replace) the file on a block device and is >>> useful >>> to workaround exclusive file access restrictions, e.g. to implement VM >>> migration with a shared disk stored on some storage with the exclusive >>> file opening model: a destination VM is started waiting for incomming >>> migration with a fake image drive, and later, on the last migration >>> phase, the fake image file is replaced with the real one. >>> >>> Signed-off-by: Denis Plotnikov <dplotni...@virtuozzo.com> >>> --- >>> hw/core/qdev-properties-system.c | 89 >>> +++++++++++++++++++++++++++----- >>> 1 file changed, 77 insertions(+), 12 deletions(-) >>> >>> diff --git a/hw/core/qdev-properties-system.c >>> b/hw/core/qdev-properties-system.c >>> index c534590dcd..aaab1370a4 100644 >>> --- a/hw/core/qdev-properties-system.c >>> +++ b/hw/core/qdev-properties-system.c >>> @@ -79,8 +79,55 @@ static void set_pointer(Object *obj, Visitor *v, >>> Property *prop, >>> /* --- drive --- */ >>> -static void do_parse_drive(DeviceState *dev, const char *str, >>> void **ptr, >>> - const char *propname, bool iothread, >>> Error **errp) >>> +static void do_parse_drive_realized(DeviceState *dev, const char *str, >>> + void **ptr, const char *propname, >>> + bool iothread, Error **errp) >>> +{ >>> + BlockBackend *blk = *ptr; >>> + BlockDriverState *bs = bdrv_lookup_bs(NULL, str, NULL); >>> + int ret; >>> + bool blk_created = false; >>> + >>> + if (!bs) { >>> + error_setg(errp, "Can't find blockdev '%s'", str); >>> + return; >>> + } >>> + >>> + if (!blk) { >>> + AioContext *ctx = iothread ? bdrv_get_aio_context(bs) : >>> + qemu_get_aio_context(); >>> + blk = blk_new(ctx, BLK_PERM_ALL, BLK_PERM_ALL); >>> + blk_created = true; >> >> Actually, I have concerns about situation where blk=null. >> >> Is there any case when scsi-hd (or others) doesn't have a blk >> assigned and it's legal? >> >>> + } else { >>> + if (blk_bs(blk)) { >>> + blk_remove_bs(blk); >>> + } >>> + } >>> + >>> + ret = blk_insert_bs(blk, bs, errp); >>> + >>> + if (!ret && blk_created) { >>> + if (blk_attach_dev(blk, dev) < 0) { >>> + /* >>> + * Shouldn't be any errors here since we just created >>> + * the new blk because the device doesn't have any. >>> + * Leave the message here in case blk_attach_dev is >>> changed >>> + */ >>> + error_setg(errp, "Can't attach drive '%s' to device >>> '%s'", >>> + str, object_get_typename(OBJECT(dev))); >>> + } else { >>> + *ptr = blk; >>> + } >>> + } > Another problem here, is that the "size" of the device dev may not > match after setting a drive. > So, we should update it after the drive setting. > It was found, that it could be done by calling > BlockDevOps.bdrv_parent_cb_resize. > > But I have some concerns about doing it so. In the case of virtio scsi > disk we have the following callstack > > bdrv_parent_cb_resize calls() -> > scsi_device_report_change(dev, SENSE_CODE(CAPACITY_CHANGED)) -> > virtio_scsi_change -> > virtio_scsi_push_event(s, dev, > VIRTIO_SCSI_T_PARAM_CHANGE, > sense.asc | > (sense.ascq << 8)); > > > virtio_scsi_change pushes the event to the guest to make the guest > ask for size refreshing. > If I'm not mistaken, here we can get a race condition when some > another request is processed with an unchanged > size and then the size changing request is processed. > > I didn't find a better way to update device size so any comments are > welcome. > > Thanks! > > Denis >>> + >>> + if (blk_created) { >>> + blk_unref(blk); >>> + } >>> +} >>> + >>> +static void do_parse_drive_unrealized(DeviceState *dev, const char >>> *str, >>> + void **ptr, const char >>> *propname, >>> + bool iothread, Error **errp) >>> { >>> BlockBackend *blk; >>> bool blk_created = false; >>> @@ -137,18 +184,34 @@ fail: >>> } >>> } >>> -static void parse_drive(DeviceState *dev, const char *str, void >>> **ptr, >>> - const char *propname, Error **errp) >>> -{ >>> - do_parse_drive(dev, str, ptr, propname, false, errp); >>> -} >>> - >>> -static void parse_drive_iothread(DeviceState *dev, const char *str, >>> void **ptr, >>> +static void parse_drive_realized(DeviceState *dev, const char *str, >>> void **ptr, >>> const char *propname, Error **errp) >>> { >>> - do_parse_drive(dev, str, ptr, propname, true, errp); >>> + do_parse_drive_realized(dev, str, ptr, propname, false, errp); >>> } >>> +static void parse_drive_realized_iothread(DeviceState *dev, const >>> char *str, >>> + void **ptr, const char >>> *propname, >>> + Error **errp) >>> +{ >>> + do_parse_drive_realized(dev, str, ptr, propname, true, errp); >>> +} >>> + >>> +static void parse_drive_unrealized(DeviceState *dev, const char *str, >>> + void **ptr, const char *propname, >>> + Error **errp) >>> +{ >>> + do_parse_drive_unrealized(dev, str, ptr, propname, false, errp); >>> +} >>> + >>> +static void parse_drive_unrealized_iothread(DeviceState *dev, const >>> char *str, >>> + void **ptr, const char >>> *propname, >>> + Error **errp) >>> +{ >>> + do_parse_drive_unrealized(dev, str, ptr, propname, true, errp); >>> +} >>> + >>> + >>> static void release_drive(Object *obj, const char *name, void >>> *opaque) >>> { >>> DeviceState *dev = DEVICE(obj); >>> @@ -188,13 +251,15 @@ static void get_drive(Object *obj, Visitor *v, >>> const char *name, void *opaque, >>> static void set_drive(Object *obj, Visitor *v, const char *name, >>> void *opaque, >>> Error **errp) >>> { >>> - set_pointer(obj, v, opaque, NULL, parse_drive, name, errp); >>> + set_pointer(obj, v, opaque, parse_drive_realized, >>> parse_drive_unrealized, >>> + name, errp); >>> } >>> static void set_drive_iothread(Object *obj, Visitor *v, const >>> char *name, >>> void *opaque, Error **errp) >>> { >>> - set_pointer(obj, v, opaque, NULL, parse_drive_iothread, name, >>> errp); >>> + set_pointer(obj, v, opaque, parse_drive_realized_iothread, >>> + parse_drive_unrealized_iothread, name, errp); >>> } >>> const PropertyInfo qdev_prop_drive = { >