| Message ID | 1395396763-26081-13-git-send-email-cyliu@suse.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Mar 21, 2014 at 06:12:23PM +0800, Chunyan Liu wrote: > In QEMUOptionParameter and QemuOptsList conversion, 'assigned' info > is lost. In current code, only qcow2 amend uses 'assigned' for a check. > It will be broken after next patch. So, remove 'assigned' check. If it's > really a must that amend is valid only to explicitly defined options, > we could add it TODO later. > > And for 'prealloc', it's not support amend, since nowhere to compare it > is changed or not, simply ignore it. > > Signed-off-by: Chunyan Liu <cyliu@suse.com> > --- > block/qcow2.c | 8 +------- > 1 file changed, 1 insertion(+), 7 deletions(-) > > diff --git a/block/qcow2.c b/block/qcow2.c > index b9dc960..92d3327 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -2088,11 +2088,6 @@ static int qcow2_amend_options(BlockDriverState *bs, > > for (i = 0; options[i].name; i++) > { > - if (!options[i].assigned) { > - /* only change explicitly defined options */ > - continue; > - } > - > if (!strcmp(options[i].name, "compat")) { > if (!options[i].value.s) { > /* preserve default */ > @@ -2106,8 +2101,7 @@ static int qcow2_amend_options(BlockDriverState *bs, > return -EINVAL; > } > } else if (!strcmp(options[i].name, "preallocation")) { > - fprintf(stderr, "Cannot change preallocation mode.\n"); > - return -ENOTSUP; > + /* Cannot change preallocation mode. Ignore it. */ You're ignoring/silencing an informed option, I think it's fear enough to notify the caller about it - even if we're never using it for amend. Regards... -- Leandro Dorileo > } else if (!strcmp(options[i].name, "size")) { > new_size = options[i].value.n; > } else if (!strcmp(options[i].name, "backing_file")) { > -- > 1.7.12.4 > >
2014-03-26 3:25 GMT+08:00 Leandro Dorileo <l@dorileo.org>: > On Fri, Mar 21, 2014 at 06:12:23PM +0800, Chunyan Liu wrote: > > In QEMUOptionParameter and QemuOptsList conversion, 'assigned' info > > is lost. In current code, only qcow2 amend uses 'assigned' for a check. > > It will be broken after next patch. So, remove 'assigned' check. If it's > > really a must that amend is valid only to explicitly defined options, > > we could add it TODO later. > > > > And for 'prealloc', it's not support amend, since nowhere to compare it > > is changed or not, simply ignore it. > > > > Signed-off-by: Chunyan Liu <cyliu@suse.com> > > --- > > block/qcow2.c | 8 +------- > > 1 file changed, 1 insertion(+), 7 deletions(-) > > > > diff --git a/block/qcow2.c b/block/qcow2.c > > index b9dc960..92d3327 100644 > > --- a/block/qcow2.c > > +++ b/block/qcow2.c > > @@ -2088,11 +2088,6 @@ static int qcow2_amend_options(BlockDriverState > *bs, > > > > for (i = 0; options[i].name; i++) > > { > > - if (!options[i].assigned) { > > - /* only change explicitly defined options */ > > - continue; > > - } > > - > > if (!strcmp(options[i].name, "compat")) { > > if (!options[i].value.s) { > > /* preserve default */ > > @@ -2106,8 +2101,7 @@ static int qcow2_amend_options(BlockDriverState > *bs, > > return -EINVAL; > > } > > } else if (!strcmp(options[i].name, "preallocation")) { > > - fprintf(stderr, "Cannot change preallocation mode.\n"); > > - return -ENOTSUP; > > + /* Cannot change preallocation mode. Ignore it. */ > > > You're ignoring/silencing an informed option, I think it's fear enough to > notify the caller > about it - even if we're never using it for amend. > To ignore it because options is an array which contains all options described in .create_options, not matter it is set or not. Before, it's filtered by "assigned" info, this switch may not enter. But since changing block layer to support both QemuOpts and QEMUOptionParameter, QEMUOptionParameter will be converted to QemuOpts for unified processing and back for here processing, after two conversions, the assigned info is lost, without filtering by "assigned" info, this switch can always enter, and just make the amend work impossible. PS: Add .assigned to QemuOptsDesc might be an easier way for the work. > > Regards... > > -- > Leandro Dorileo > > > > } else if (!strcmp(options[i].name, "size")) { > > new_size = options[i].value.n; > > } else if (!strcmp(options[i].name, "backing_file")) { > > -- > > 1.7.12.4 > > > > > > -- > Leandro Dorileo > >
2014-03-26 15:37 GMT+08:00 Chunyan Liu <cyliu@suse.com>: > > > > 2014-03-26 3:25 GMT+08:00 Leandro Dorileo <l@dorileo.org>: > > On Fri, Mar 21, 2014 at 06:12:23PM +0800, Chunyan Liu wrote: >> > In QEMUOptionParameter and QemuOptsList conversion, 'assigned' info >> > is lost. In current code, only qcow2 amend uses 'assigned' for a check. >> > It will be broken after next patch. So, remove 'assigned' check. If it's >> > really a must that amend is valid only to explicitly defined options, >> > we could add it TODO later. >> > >> > And for 'prealloc', it's not support amend, since nowhere to compare it >> > is changed or not, simply ignore it. >> > >> > Signed-off-by: Chunyan Liu <cyliu@suse.com> >> > --- >> > block/qcow2.c | 8 +------- >> > 1 file changed, 1 insertion(+), 7 deletions(-) >> > >> > diff --git a/block/qcow2.c b/block/qcow2.c >> > index b9dc960..92d3327 100644 >> > --- a/block/qcow2.c >> > +++ b/block/qcow2.c >> > @@ -2088,11 +2088,6 @@ static int qcow2_amend_options(BlockDriverState >> *bs, >> > >> > for (i = 0; options[i].name; i++) >> > { >> > - if (!options[i].assigned) { >> > - /* only change explicitly defined options */ >> > - continue; >> > - } >> > - >> > if (!strcmp(options[i].name, "compat")) { >> > if (!options[i].value.s) { >> > /* preserve default */ >> > @@ -2106,8 +2101,7 @@ static int qcow2_amend_options(BlockDriverState >> *bs, >> > return -EINVAL; >> > } >> > } else if (!strcmp(options[i].name, "preallocation")) { >> > - fprintf(stderr, "Cannot change preallocation mode.\n"); >> > - return -ENOTSUP; >> > + /* Cannot change preallocation mode. Ignore it. */ >> >> >> You're ignoring/silencing an informed option, I think it's fear enough to >> notify the caller >> about it - even if we're never using it for amend. >> > > To ignore it because options is an array which contains all options > described in > .create_options, not matter it is set or not. Before, it's filtered by > "assigned" info, this > switch may not enter. > > But since changing block layer to support both QemuOpts and > QEMUOptionParameter, > QEMUOptionParameter will be converted to QemuOpts for unified processing > and back > for here processing, after two conversions, the assigned info is lost, > without filtering by > "assigned" info, this switch can always enter, and just make the amend > work impossible. > > PS: > Add .assigned to QemuOptsDesc might be an easier way for the work. > Well, I may finally try to keep .assigned info after conversion. When qemu_opt_find can find the opt, that means the opt is set, then convert to QEMUOptionParamter, the .assigned=true. With this process, this patch is not needed. Similarly, when switching to QemuOpts in qcow2.c, previous filtering by .assigned could be replaced with filtering by qemu_opt_find. > > >> >> Regards... >> >> -- >> Leandro Dorileo >> >> >> > } else if (!strcmp(options[i].name, "size")) { >> > new_size = options[i].value.n; >> > } else if (!strcmp(options[i].name, "backing_file")) { >> > -- >> > 1.7.12.4 >> > >> > >> >> -- >> Leandro Dorileo >> >> >
diff --git a/block/qcow2.c b/block/qcow2.c index b9dc960..92d3327 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2088,11 +2088,6 @@ static int qcow2_amend_options(BlockDriverState *bs, for (i = 0; options[i].name; i++) { - if (!options[i].assigned) { - /* only change explicitly defined options */ - continue; - } - if (!strcmp(options[i].name, "compat")) { if (!options[i].value.s) { /* preserve default */ @@ -2106,8 +2101,7 @@ static int qcow2_amend_options(BlockDriverState *bs, return -EINVAL; } } else if (!strcmp(options[i].name, "preallocation")) { - fprintf(stderr, "Cannot change preallocation mode.\n"); - return -ENOTSUP; + /* Cannot change preallocation mode. Ignore it. */ } else if (!strcmp(options[i].name, "size")) { new_size = options[i].value.n; } else if (!strcmp(options[i].name, "backing_file")) {
In QEMUOptionParameter and QemuOptsList conversion, 'assigned' info is lost. In current code, only qcow2 amend uses 'assigned' for a check. It will be broken after next patch. So, remove 'assigned' check. If it's really a must that amend is valid only to explicitly defined options, we could add it TODO later. And for 'prealloc', it's not support amend, since nowhere to compare it is changed or not, simply ignore it. Signed-off-by: Chunyan Liu <cyliu@suse.com> --- block/qcow2.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)