| Message ID | 1390762963-25538-9-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
Le Sunday 26 Jan 2014 à 20:02:41 (+0100), Max Reitz a écrit : > Setting bs->options in bdrv_file_open() is not necessary if it is > already done in bdrv_open(). > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/block.c b/block.c > index 0f2cd3f..f847c4b 100644 > --- a/block.c > +++ b/block.c > @@ -956,9 +956,6 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename, > Error *local_err = NULL; > int ret; > > - bs->options = options; > - options = qdict_clone_shallow(options); > - > /* Fetch the file name from the options QDict if necessary */ > if (!filename) { > filename = qdict_get_try_str(options, "filename"); > @@ -1234,6 +1231,9 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, > bs = bdrv_new(""); > } > > + bs->options = options; > + options = qdict_clone_shallow(options); > + > if (flags & BDRV_O_PROTOCOL) { > assert(!drv); > ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL, > @@ -1250,9 +1250,6 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, > return 0; > } > > - bs->options = options; > - options = qdict_clone_shallow(options); > - > /* For snapshot=on, create a temporary qcow2 overlay */ > if (flags & BDRV_O_SNAPSHOT) { > BlockDriverState *bs1 = NULL; > -- > 1.8.5.3 > > Reviewed-by: Benoit Canet <benoit@irqsave.net>
Am 26.01.2014 um 20:02 hat Max Reitz geschrieben: > Setting bs->options in bdrv_file_open() is not necessary if it is > already done in bdrv_open(). > > Signed-off-by: Max Reitz <mreitz@redhat.com> Perhaps squash this into patch 7? Because now the reference is created by bdrv_open() instead of bdrv_file_open(). Also my suggestion in a reply to a patch earlier in this series (keeping all option-related code at one place), probably means that it all needs to be done together (or immediately after) handling the NULL options case. Kevin
On 29.01.2014 14:45, Kevin Wolf wrote: > Am 26.01.2014 um 20:02 hat Max Reitz geschrieben: >> Setting bs->options in bdrv_file_open() is not necessary if it is >> already done in bdrv_open(). >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> > Perhaps squash this into patch 7? Because now the reference is created > by bdrv_open() instead of bdrv_file_open(). Yes, why not. > Also my suggestion in a reply to a patch earlier in this series (keeping > all option-related code at one place), probably means that it all needs > to be done together (or immediately after) handling the NULL options > case. I'll have a look. Thank you for reviewing. Max
diff --git a/block.c b/block.c index 0f2cd3f..f847c4b 100644 --- a/block.c +++ b/block.c @@ -956,9 +956,6 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename, Error *local_err = NULL; int ret; - bs->options = options; - options = qdict_clone_shallow(options); - /* Fetch the file name from the options QDict if necessary */ if (!filename) { filename = qdict_get_try_str(options, "filename"); @@ -1234,6 +1231,9 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, bs = bdrv_new(""); } + bs->options = options; + options = qdict_clone_shallow(options); + if (flags & BDRV_O_PROTOCOL) { assert(!drv); ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL, @@ -1250,9 +1250,6 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, return 0; } - bs->options = options; - options = qdict_clone_shallow(options); - /* For snapshot=on, create a temporary qcow2 overlay */ if (flags & BDRV_O_SNAPSHOT) { BlockDriverState *bs1 = NULL;
Setting bs->options in bdrv_file_open() is not necessary if it is already done in bdrv_open(). Signed-off-by: Max Reitz <mreitz@redhat.com> --- block.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)