| Message ID | 1371750371-18491-8-git-send-email-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
Il 20/06/2013 19:46, Marc-André Lureau ha scritto: > This allows the Spice block driver to eject the associated device. The child can change when you have for example a streaming operation. What exactly are you trying to do here (I guess I'll understand more when I get to the later patches)? Can you draw the relationships between all the BlockDriverStates in a spicebd: drive? Paolo > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > block.c | 46 +++++++++++++++++++++++++++++----------------- > include/block/block_int.h | 1 + > 2 files changed, 30 insertions(+), 17 deletions(-) > > diff --git a/block.c b/block.c > index b88ad2f..f502eed 100644 > --- a/block.c > +++ b/block.c > @@ -294,7 +294,8 @@ void bdrv_register(BlockDriver *bdrv) > } > > /* create a new block device (by default it is empty) */ > -BlockDriverState *bdrv_new(const char *device_name) > +static BlockDriverState *bdrv_new_int(const char *device_name, > + BlockDriverState *child) > { > BlockDriverState *bs; > > @@ -305,10 +306,16 @@ BlockDriverState *bdrv_new(const char *device_name) > } > bdrv_iostatus_disable(bs); > notifier_list_init(&bs->close_notifiers); > + bs->child = child; > > return bs; > } > > +BlockDriverState *bdrv_new(const char *device_name) > +{ > + return bdrv_new_int(device_name, NULL); > +} > + > void bdrv_add_close_notifier(BlockDriverState *bs, Notifier *notify) > { > notifier_list_add(&bs->close_notifiers, notify); > @@ -769,16 +776,8 @@ free_and_fail: > return ret; > } > > -/* > - * Opens a file using a protocol (file, host_device, nbd, ...) > - * > - * options is a QDict of options to pass to the block drivers, or NULL for an > - * empty set of options. The reference to the QDict belongs to the block layer > - * after the call (even on failure), so if the caller intends to reuse the > - * dictionary, it needs to use QINCREF() before calling bdrv_file_open. > - */ > -int bdrv_file_open(BlockDriverState **pbs, const char *filename, > - QDict *options, int flags) > +static int bdrv_file_open_int(BlockDriverState **pbs, const char *filename, > + QDict *options, int flags, BlockDriverState *child) > { > BlockDriverState *bs; > BlockDriver *drv; > @@ -790,7 +789,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename, > options = qdict_new(); > } > > - bs = bdrv_new(""); > + bs = bdrv_new_int("", child); > bs->options = options; > options = qdict_clone_shallow(options); > > @@ -873,6 +872,20 @@ fail: > } > > /* > + * Opens a file using a protocol (file, host_device, nbd, ...) > + * > + * options is a QDict of options to pass to the block drivers, or NULL for an > + * empty set of options. The reference to the QDict belongs to the block layer > + * after the call (even on failure), so if the caller intends to reuse the > + * dictionary, it needs to use QINCREF() before calling bdrv_file_open. > + */ > +int bdrv_file_open(BlockDriverState **pbs, const char *filename, > + QDict *options, int flags) > +{ > + return bdrv_file_open_int(pbs, filename, options, flags, NULL); > +} > + > +/* > * Opens the backing file for a BlockDriverState if not yet open > * > * options is a QDict of options to pass to the block drivers, or NULL for an > @@ -904,7 +917,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options) > return 0; > } > > - bs->backing_hd = bdrv_new(""); > + bs->backing_hd = bdrv_new_int("", bs); > bdrv_get_full_backing_filename(bs, backing_filename, > sizeof(backing_filename)); > > @@ -990,7 +1003,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options, > instead of opening 'filename' directly */ > > /* if there is a backing file, use it */ > - bs1 = bdrv_new(""); > + bs1 = bdrv_new_int("", bs); > ret = bdrv_open(bs1, filename, NULL, 0, drv); > if (ret < 0) { > bdrv_delete(bs1); > @@ -1043,9 +1056,8 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options, > } > > extract_subqdict(options, &file_options, "file."); > - > - ret = bdrv_file_open(&file, filename, file_options, > - bdrv_open_flags(bs, flags)); > + ret = bdrv_file_open_int(&file, filename, file_options, > + bdrv_open_flags(bs, flags), bs); > if (ret < 0) { > goto fail; > } > diff --git a/include/block/block_int.h b/include/block/block_int.h > index ba52247..9c72b32 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -245,6 +245,7 @@ struct BlockDriverState { > > BlockDriverState *backing_hd; > BlockDriverState *file; > + BlockDriverState *child; > > NotifierList close_notifiers; > >
Hi On Fri, Jun 21, 2013 at 12:25 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 20/06/2013 19:46, Marc-André Lureau ha scritto: > > This allows the Spice block driver to eject the associated device. > > The child can change when you have for example a streaming operation. > Ah, can you point me to some function or some way I can reproduce? I don't know what you mean by a streaming operation here. What exactly are you trying to do here (I guess I'll understand more > when I get to the later patches)? > Getting to the bottom BlockDriverState to be able to eject & change it. (it could also be named the "parent", but other parts of the code suggest the "child" name) > > Can you draw the relationships between all the BlockDriverStates in a > spicebd: drive? > Hopefully, but I have only tested with raw images (w/wo snapshot). > Paolo > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > block.c | 46 > +++++++++++++++++++++++++++++----------------- > > include/block/block_int.h | 1 + > > 2 files changed, 30 insertions(+), 17 deletions(-) > > > > diff --git a/block.c b/block.c > > index b88ad2f..f502eed 100644 > > --- a/block.c > > +++ b/block.c > > @@ -294,7 +294,8 @@ void bdrv_register(BlockDriver *bdrv) > > } > > > > /* create a new block device (by default it is empty) */ > > -BlockDriverState *bdrv_new(const char *device_name) > > +static BlockDriverState *bdrv_new_int(const char *device_name, > > + BlockDriverState *child) > > { > > BlockDriverState *bs; > > > > @@ -305,10 +306,16 @@ BlockDriverState *bdrv_new(const char *device_name) > > } > > bdrv_iostatus_disable(bs); > > notifier_list_init(&bs->close_notifiers); > > + bs->child = child; > > > > return bs; > > } > > > > +BlockDriverState *bdrv_new(const char *device_name) > > +{ > > + return bdrv_new_int(device_name, NULL); > > +} > > + > > void bdrv_add_close_notifier(BlockDriverState *bs, Notifier *notify) > > { > > notifier_list_add(&bs->close_notifiers, notify); > > @@ -769,16 +776,8 @@ free_and_fail: > > return ret; > > } > > > > -/* > > - * Opens a file using a protocol (file, host_device, nbd, ...) > > - * > > - * options is a QDict of options to pass to the block drivers, or NULL > for an > > - * empty set of options. The reference to the QDict belongs to the > block layer > > - * after the call (even on failure), so if the caller intends to reuse > the > > - * dictionary, it needs to use QINCREF() before calling bdrv_file_open. > > - */ > > -int bdrv_file_open(BlockDriverState **pbs, const char *filename, > > - QDict *options, int flags) > > +static int bdrv_file_open_int(BlockDriverState **pbs, const char > *filename, > > + QDict *options, int flags, BlockDriverState *child) > > { > > BlockDriverState *bs; > > BlockDriver *drv; > > @@ -790,7 +789,7 @@ int bdrv_file_open(BlockDriverState **pbs, const > char *filename, > > options = qdict_new(); > > } > > > > - bs = bdrv_new(""); > > + bs = bdrv_new_int("", child); > > bs->options = options; > > options = qdict_clone_shallow(options); > > > > @@ -873,6 +872,20 @@ fail: > > } > > > > /* > > + * Opens a file using a protocol (file, host_device, nbd, ...) > > + * > > + * options is a QDict of options to pass to the block drivers, or NULL > for an > > + * empty set of options. The reference to the QDict belongs to the > block layer > > + * after the call (even on failure), so if the caller intends to reuse > the > > + * dictionary, it needs to use QINCREF() before calling bdrv_file_open. > > + */ > > +int bdrv_file_open(BlockDriverState **pbs, const char *filename, > > + QDict *options, int flags) > > +{ > > + return bdrv_file_open_int(pbs, filename, options, flags, NULL); > > +} > > + > > +/* > > * Opens the backing file for a BlockDriverState if not yet open > > * > > * options is a QDict of options to pass to the block drivers, or NULL > for an > > @@ -904,7 +917,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, > QDict *options) > > return 0; > > } > > > > - bs->backing_hd = bdrv_new(""); > > + bs->backing_hd = bdrv_new_int("", bs); > > bdrv_get_full_backing_filename(bs, backing_filename, > > sizeof(backing_filename)); > > > > @@ -990,7 +1003,7 @@ int bdrv_open(BlockDriverState *bs, const char > *filename, QDict *options, > > instead of opening 'filename' directly */ > > > > /* if there is a backing file, use it */ > > - bs1 = bdrv_new(""); > > + bs1 = bdrv_new_int("", bs); > > ret = bdrv_open(bs1, filename, NULL, 0, drv); > > if (ret < 0) { > > bdrv_delete(bs1); > > @@ -1043,9 +1056,8 @@ int bdrv_open(BlockDriverState *bs, const char > *filename, QDict *options, > > } > > > > extract_subqdict(options, &file_options, "file."); > > - > > - ret = bdrv_file_open(&file, filename, file_options, > > - bdrv_open_flags(bs, flags)); > > + ret = bdrv_file_open_int(&file, filename, file_options, > > + bdrv_open_flags(bs, flags), bs); > > if (ret < 0) { > > goto fail; > > } > > diff --git a/include/block/block_int.h b/include/block/block_int.h > > index ba52247..9c72b32 100644 > > --- a/include/block/block_int.h > > +++ b/include/block/block_int.h > > @@ -245,6 +245,7 @@ struct BlockDriverState { > > > > BlockDriverState *backing_hd; > > BlockDriverState *file; > > + BlockDriverState *child; > > > > NotifierList close_notifiers; > > > > > >
Il 21/06/2013 10:36, Marc-André Lureau ha scritto: > Hi > > > On Fri, Jun 21, 2013 at 12:25 AM, Paolo Bonzini <pbonzini@redhat.com > <mailto:pbonzini@redhat.com>> wrote: > > Il 20/06/2013 19:46, Marc-André Lureau ha scritto: > > This allows the Spice block driver to eject the associated device. > > The child can change when you have for example a streaming operation. > > Ah, can you point me to some function or some way I can reproduce? I > don't know what you mean by a streaming operation here. Let's just discuss the design so that it just works and you don't have to worry about it. :) > What exactly are you trying to do here (I guess I'll understand more > when I get to the later patches)? > > Getting to the bottom BlockDriverState to be able to eject & change it. > (it could also be named the "parent", but other parts of the code > suggest the "child" name) There is already an interface for eject/change, which is the monitor. To signal an eject or medium change, the SPICE client should simply ask libvirt to do so. It is fine to change "from" spicebd: "to" spicebd:, the guest would still perceive it as a change. I'm not sure how libvirt communicates the change back to the SPICE client, and whether it is asynchronous or synchronous. BTW, note that IDE or virtio-blk do not support removable media, and are not able to pass eject or media change notifications. SCSI devices (such as usb-bot, usb-uas and virtio-scsi) can. > Can you draw the relationships between all the BlockDriverStates in a > spicebd: drive? > > > Hopefully, but I have only tested with raw images (w/wo snapshot). Then draw it. :) But from the above description it looks like it is not necessary, it should simply be "raw" on top of "spicebd". Parsing formats should be done on the client side, perhaps by invoking qemu-nbd and tunnelling the NBD data on the SPICE channel. Paolo
Hi On Fri, Jun 21, 2013 at 1:57 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > What exactly are you trying to do here (I guess I'll understand more > > when I get to the later patches)? > > > > Getting to the bottom BlockDriverState to be able to eject & change it. > > (it could also be named the "parent", but other parts of the code > > suggest the "child" name) > > There is already an interface for eject/change, which is the monitor. > To signal an eject or medium change, the SPICE client should simply ask > libvirt to do so. It is fine to change "from" spicebd: "to" spicebd:, > the guest would still perceive it as a change. > > I'm not sure how libvirt communicates the change back to the SPICE > client, and whether it is asynchronous or synchronous. > > Spice is not using libvirt, and doesn't have access to monitor. I looked at the monitor code. Basically, the spice block driver I proposed uses a similar code to forcefully eject and change. Perhaps it's possible for the Spice qemu-side code to have access to the monitor, but the end result would be similar anyway. > BTW, note that IDE or virtio-blk do not support removable media, and are > not able to pass eject or media change notifications. SCSI devices > (such as usb-bot, usb-uas and virtio-scsi) can. > > > Can you draw the relationships between all the BlockDriverStates in a > > spicebd: drive? > > > > > > Hopefully, but I have only tested with raw images (w/wo snapshot). > > Then draw it. :) But from the above description it looks like it is not > necessary, it should simply be "raw" on top of "spicebd". Parsing > formats should be done on the client side, perhaps by invoking qemu-nbd > and tunnelling the NBD data on the SPICE channel. > > Right, "raw" on top of "spicebd" for iso/raw images (and "qcow2" on top for snapshot support - even in readonly). I wonder what would happen if spicebd image would be something else than raw/iso, I suppose there would be more BlockDriverStates to handle the various format.
Il 21/06/2013 15:30, Marc-André Lureau ha scritto: > > Getting to the bottom BlockDriverState to be able to eject & > > change it. (it could also be named the "parent", but other parts > > of the code suggest the "child" name) > > There is already an interface for eject/change, which is the monitor. > To signal an eject or medium change, the SPICE client should simply ask > libvirt to do so. It is fine to change "from" spicebd: "to" spicebd:, > the guest would still perceive it as a change. > > I'm not sure how libvirt communicates the change back to the SPICE > client, and whether it is asynchronous or synchronous. > > Spice is not using libvirt, and doesn't have access to monitor. I looked > at the monitor code. Basically, the spice block driver I proposed uses a > similar code to forcefully eject and change. Similar, but it also violates encapsulation by adding bs->child. :) > Perhaps it's possible for the Spice qemu-side code to have access to the > monitor, but the end result would be similar anyway. That would work better. Spice qemu-side would keep an association between Spice channels and block device names, and use that to convert Spice commands to monitor commands (the API is qmp_change_blockdev/qmp_eject). Paolo > > BTW, note that IDE or virtio-blk do not support removable media, and are > not able to pass eject or media change notifications. SCSI devices > (such as usb-bot, usb-uas and virtio-scsi) can. > > > Can you draw the relationships between all the > BlockDriverStates in a > > spicebd: drive? > > > > > > Hopefully, but I have only tested with raw images (w/wo snapshot). > > Then draw it. :) But from the above description it looks like it is not > necessary, it should simply be "raw" on top of "spicebd". Parsing > formats should be done on the client side, perhaps by invoking qemu-nbd > and tunnelling the NBD data on the SPICE channel. > > > Right, "raw" on top of "spicebd" for iso/raw images (and "qcow2" on top > for snapshot support - even in readonly). > > I wonder what would happen if spicebd image would be something else than > raw/iso, I suppose there would be more BlockDriverStates to handle the > various format. > > -- > Marc-André Lureau
diff --git a/block.c b/block.c index b88ad2f..f502eed 100644 --- a/block.c +++ b/block.c @@ -294,7 +294,8 @@ void bdrv_register(BlockDriver *bdrv) } /* create a new block device (by default it is empty) */ -BlockDriverState *bdrv_new(const char *device_name) +static BlockDriverState *bdrv_new_int(const char *device_name, + BlockDriverState *child) { BlockDriverState *bs; @@ -305,10 +306,16 @@ BlockDriverState *bdrv_new(const char *device_name) } bdrv_iostatus_disable(bs); notifier_list_init(&bs->close_notifiers); + bs->child = child; return bs; } +BlockDriverState *bdrv_new(const char *device_name) +{ + return bdrv_new_int(device_name, NULL); +} + void bdrv_add_close_notifier(BlockDriverState *bs, Notifier *notify) { notifier_list_add(&bs->close_notifiers, notify); @@ -769,16 +776,8 @@ free_and_fail: return ret; } -/* - * Opens a file using a protocol (file, host_device, nbd, ...) - * - * options is a QDict of options to pass to the block drivers, or NULL for an - * empty set of options. The reference to the QDict belongs to the block layer - * after the call (even on failure), so if the caller intends to reuse the - * dictionary, it needs to use QINCREF() before calling bdrv_file_open. - */ -int bdrv_file_open(BlockDriverState **pbs, const char *filename, - QDict *options, int flags) +static int bdrv_file_open_int(BlockDriverState **pbs, const char *filename, + QDict *options, int flags, BlockDriverState *child) { BlockDriverState *bs; BlockDriver *drv; @@ -790,7 +789,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename, options = qdict_new(); } - bs = bdrv_new(""); + bs = bdrv_new_int("", child); bs->options = options; options = qdict_clone_shallow(options); @@ -873,6 +872,20 @@ fail: } /* + * Opens a file using a protocol (file, host_device, nbd, ...) + * + * options is a QDict of options to pass to the block drivers, or NULL for an + * empty set of options. The reference to the QDict belongs to the block layer + * after the call (even on failure), so if the caller intends to reuse the + * dictionary, it needs to use QINCREF() before calling bdrv_file_open. + */ +int bdrv_file_open(BlockDriverState **pbs, const char *filename, + QDict *options, int flags) +{ + return bdrv_file_open_int(pbs, filename, options, flags, NULL); +} + +/* * Opens the backing file for a BlockDriverState if not yet open * * options is a QDict of options to pass to the block drivers, or NULL for an @@ -904,7 +917,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options) return 0; } - bs->backing_hd = bdrv_new(""); + bs->backing_hd = bdrv_new_int("", bs); bdrv_get_full_backing_filename(bs, backing_filename, sizeof(backing_filename)); @@ -990,7 +1003,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options, instead of opening 'filename' directly */ /* if there is a backing file, use it */ - bs1 = bdrv_new(""); + bs1 = bdrv_new_int("", bs); ret = bdrv_open(bs1, filename, NULL, 0, drv); if (ret < 0) { bdrv_delete(bs1); @@ -1043,9 +1056,8 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options, } extract_subqdict(options, &file_options, "file."); - - ret = bdrv_file_open(&file, filename, file_options, - bdrv_open_flags(bs, flags)); + ret = bdrv_file_open_int(&file, filename, file_options, + bdrv_open_flags(bs, flags), bs); if (ret < 0) { goto fail; } diff --git a/include/block/block_int.h b/include/block/block_int.h index ba52247..9c72b32 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -245,6 +245,7 @@ struct BlockDriverState { BlockDriverState *backing_hd; BlockDriverState *file; + BlockDriverState *child; NotifierList close_notifiers;
This allows the Spice block driver to eject the associated device. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- block.c | 46 +++++++++++++++++++++++++++++----------------- include/block/block_int.h | 1 + 2 files changed, 30 insertions(+), 17 deletions(-)