Re: [Qemu-devel] [PATCH] block: add .bdrv_reopen_prepare() stub for iscsi

[Stefan Hajnoczi <stefanha@gmail.com>]

On Tue, Jan 14, 2014 at 01:10:24PM -0500, Jeff Cody wrote:
> To suppport reopen(), the .bdrv_reopen_prepare() stub must exist.
> iSCSI does not have anything that needs to be done to support reopen,
> so we can just implement the _prepare() stub.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/iscsi.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index c0ea0c4..5976bd1 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1326,6 +1326,14 @@ static void iscsi_close(BlockDriverState *bs)
>      memset(iscsilun, 0, sizeof(IscsiLun));
>  }
>  
> +/* We have nothing to do for iSCSI reopen, stub just returns
> + * success */
> +static int iscsi_reopen_prepare(BDRVReopenState *state,
> +                                BlockReopenQueue *queue, Error **errp)
> +{
> +    return 0;
> +}

I think we should be more specific here: the assumption is that
bdrv_reopen() is only used to change bdrv_open() flags.  And since
block/iscsi.c ignores bdrv_open() flags there is no difference whether
we actually reopen the LUN.

In other words, this patch means bdrv_reopen() will *not* reconnect to
the iSCSI target, which could be useful for other reasons (e.g. graceful
restart of iSCSI target).  I think this can be justified because
block/raw-posix.c also tries to use dup instead of actually reopening
the filename.  But we should be aware of the semantics of bdrv_reopen().

Anyway, in the future the assumption that block/iscsi.c ignores
bdrv_open() flags could change.  This reopen implementation will break!

Please add a comment to iscsi_open() explaining that anyone wishing to
use flags must consider modifying reopen too.  Please also change the
comment/commit message to be more specific about why "We have nothing to
do for iSCSI reopen".