| Message ID | 1448294400-476-7-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 11/23/2015 11:59 PM, Kevin Wolf wrote: > Some drivers have nested options (e.g. blkdebug rule arrays), which > don't belong to a child node and shouldn't be removed. Don't remove all > options with "." in their name, but check for the complete prefixes of > actually existing child nodes. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block.c | 19 +++++++++++++++---- > include/block/block_int.h | 1 + > 2 files changed, 16 insertions(+), 4 deletions(-) > > diff --git a/block.c b/block.c > index 23d9e10..02125e2 100644 > --- a/block.c > +++ b/block.c > @@ -1101,11 +1101,13 @@ static int bdrv_fill_options(QDict **options, const char **pfilename, > > static BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs, > BlockDriverState *child_bs, > + const char *child_name, > const BdrvChildRole *child_role) > { > BdrvChild *child = g_new(BdrvChild, 1); > *child = (BdrvChild) { > .bs = child_bs, > + .name = child_name, The child_name may be allocated in the caller's stack. For example: In the function quorum_open(): for (i = 0; i < s->num_children; i++) { char indexstr[32]; ret = snprintf(indexstr, 32, "children.%d", i); assert(ret < 32); s->children[i] = bdrv_open_child(NULL, options, indexstr, bs, &child_format, false, &local_err); if (local_err) { ret = -EINVAL; goto close_exit; } opened[i] = true; } Thanks Wen Congyang > .role = child_role, > }; > > @@ -1165,7 +1167,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) > bs->backing = NULL; > goto out; > } > - bs->backing = bdrv_attach_child(bs, backing_hd, &child_backing); > + bs->backing = bdrv_attach_child(bs, backing_hd, "backing", &child_backing); > bs->open_flags &= ~BDRV_O_NO_BACKING; > pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->filename); > pstrcpy(bs->backing_format, sizeof(bs->backing_format), > @@ -1322,7 +1324,7 @@ BdrvChild *bdrv_open_child(const char *filename, > goto done; > } > > - c = bdrv_attach_child(parent, bs, child_role); > + c = bdrv_attach_child(parent, bs, bdref_key, child_role); > > done: > qdict_del(options, bdref_key); > @@ -3952,13 +3954,22 @@ static bool append_open_options(QDict *d, BlockDriverState *bs) > { > const QDictEntry *entry; > QemuOptDesc *desc; > + BdrvChild *child; > bool found_any = false; > + const char *p; > > for (entry = qdict_first(bs->options); entry; > entry = qdict_next(bs->options, entry)) > { > - /* Only take options for this level */ > - if (strchr(qdict_entry_key(entry), '.')) { > + /* Exclude options for children */ > + QLIST_FOREACH(child, &bs->children, next) { > + if (strstart(qdict_entry_key(entry), child->name, &p) > + && (!*p || *p == '.')) > + { > + break; > + } > + } > + if (child) { > continue; > } > > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 77dc165..b2325aa 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -351,6 +351,7 @@ extern const BdrvChildRole child_format; > > struct BdrvChild { > BlockDriverState *bs; > + const char *name; > const BdrvChildRole *role; > QLIST_ENTRY(BdrvChild) next; > QLIST_ENTRY(BdrvChild) next_parent; >
On 23.11.2015 16:59, Kevin Wolf wrote: > Some drivers have nested options (e.g. blkdebug rule arrays), which > don't belong to a child node and shouldn't be removed. Don't remove all > options with "." in their name, but check for the complete prefixes of > actually existing child nodes. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block.c | 19 +++++++++++++++---- > include/block/block_int.h | 1 + > 2 files changed, 16 insertions(+), 4 deletions(-) Thanks, now I don't need to fix it myself. :-) (I would have had to do that for an in-work series of mine) > diff --git a/block.c b/block.c > index 23d9e10..02125e2 100644 > --- a/block.c > +++ b/block.c > @@ -1101,11 +1101,13 @@ static int bdrv_fill_options(QDict **options, const char **pfilename, > > static BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs, > BlockDriverState *child_bs, > + const char *child_name, > const BdrvChildRole *child_role) > { > BdrvChild *child = g_new(BdrvChild, 1); > *child = (BdrvChild) { > .bs = child_bs, > + .name = child_name, > .role = child_role, > }; > > @@ -1165,7 +1167,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) > bs->backing = NULL; > goto out; > } > - bs->backing = bdrv_attach_child(bs, backing_hd, &child_backing); > + bs->backing = bdrv_attach_child(bs, backing_hd, "backing", &child_backing); > bs->open_flags &= ~BDRV_O_NO_BACKING; > pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->filename); > pstrcpy(bs->backing_format, sizeof(bs->backing_format), > @@ -1322,7 +1324,7 @@ BdrvChild *bdrv_open_child(const char *filename, > goto done; > } > > - c = bdrv_attach_child(parent, bs, child_role); > + c = bdrv_attach_child(parent, bs, bdref_key, child_role); > > done: > qdict_del(options, bdref_key); > @@ -3952,13 +3954,22 @@ static bool append_open_options(QDict *d, BlockDriverState *bs) > { > const QDictEntry *entry; > QemuOptDesc *desc; > + BdrvChild *child; > bool found_any = false; > + const char *p; > > for (entry = qdict_first(bs->options); entry; > entry = qdict_next(bs->options, entry)) > { > - /* Only take options for this level */ > - if (strchr(qdict_entry_key(entry), '.')) { > + /* Exclude options for children */ > + QLIST_FOREACH(child, &bs->children, next) { > + if (strstart(qdict_entry_key(entry), child->name, &p) > + && (!*p || *p == '.')) > + { > + break; > + } > + } > + if (child) { > continue; > } > A good general solution, but I think bdrv_refresh_filename() may be bad enough to break with general solutions. ;-) bdrv_refresh_filename() only considers "file" and "backing" (actually, it only supports "file" for now, I'm working on "backing", though). The only drivers with other children are quorum, blkdebug, blkverify and VMDK. The former three have their own implementation of bdrv_refresh_filename(), so they don't use append_open_options() at all. The latter, however, (VMDK) does not. This change to append_open_options results in the extent.%d options simply being omitted altogether because bdrv_refresh_filename() does not fetch them. Before, they were included in the VMDK BDS's options, which is not ideal but works more or less. In order to "fix" this, I see three ways right now: 1. Just care about "file" and "backing". bdrv_refresh_filename() doesn't support anything else, so that will be fine. 2. Implement bdrv_refresh_filename() specifically for VMDK so append_open_options() will never have to handle anything but "file" and "backing". 3. Fix bdrv_refresh_filename() so that it handles all children and not just "file" and "backing". Since we are shooting for 2.6 anyway (I assume ;-)), I think we should go for option 3. This means that this patch is fine, and I'll see to fixing bdrv_refresh_filename() (because I'm working on that anyway). Reviewed-by: Max Reitz <mreitz@redhat.com>
On 27.11.2015 18:58, Max Reitz wrote: > On 23.11.2015 16:59, Kevin Wolf wrote: >> Some drivers have nested options (e.g. blkdebug rule arrays), which >> don't belong to a child node and shouldn't be removed. Don't remove all >> options with "." in their name, but check for the complete prefixes of >> actually existing child nodes. >> >> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >> --- >> block.c | 19 +++++++++++++++---- >> include/block/block_int.h | 1 + >> 2 files changed, 16 insertions(+), 4 deletions(-) > > Thanks, now I don't need to fix it myself. :-) > > (I would have had to do that for an in-work series of mine) > >> diff --git a/block.c b/block.c >> index 23d9e10..02125e2 100644 >> --- a/block.c >> +++ b/block.c >> @@ -1101,11 +1101,13 @@ static int bdrv_fill_options(QDict **options, const char **pfilename, >> >> static BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs, >> BlockDriverState *child_bs, >> + const char *child_name, >> const BdrvChildRole *child_role) >> { >> BdrvChild *child = g_new(BdrvChild, 1); >> *child = (BdrvChild) { >> .bs = child_bs, >> + .name = child_name, >> .role = child_role, >> }; >> >> @@ -1165,7 +1167,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) >> bs->backing = NULL; >> goto out; >> } >> - bs->backing = bdrv_attach_child(bs, backing_hd, &child_backing); >> + bs->backing = bdrv_attach_child(bs, backing_hd, "backing", &child_backing); >> bs->open_flags &= ~BDRV_O_NO_BACKING; >> pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->filename); >> pstrcpy(bs->backing_format, sizeof(bs->backing_format), >> @@ -1322,7 +1324,7 @@ BdrvChild *bdrv_open_child(const char *filename, >> goto done; >> } >> >> - c = bdrv_attach_child(parent, bs, child_role); >> + c = bdrv_attach_child(parent, bs, bdref_key, child_role); >> >> done: >> qdict_del(options, bdref_key); >> @@ -3952,13 +3954,22 @@ static bool append_open_options(QDict *d, BlockDriverState *bs) >> { >> const QDictEntry *entry; >> QemuOptDesc *desc; >> + BdrvChild *child; >> bool found_any = false; >> + const char *p; >> >> for (entry = qdict_first(bs->options); entry; >> entry = qdict_next(bs->options, entry)) >> { >> - /* Only take options for this level */ >> - if (strchr(qdict_entry_key(entry), '.')) { >> + /* Exclude options for children */ >> + QLIST_FOREACH(child, &bs->children, next) { >> + if (strstart(qdict_entry_key(entry), child->name, &p) >> + && (!*p || *p == '.')) >> + { >> + break; >> + } >> + } >> + if (child) { >> continue; >> } >> > > A good general solution, but I think bdrv_refresh_filename() may be bad > enough to break with general solutions. ;-) > > bdrv_refresh_filename() only considers "file" and "backing" (actually, > it only supports "file" for now, I'm working on "backing", though). The > only drivers with other children are quorum, blkdebug, blkverify and > VMDK. The former three have their own implementation of > bdrv_refresh_filename(), so they don't use append_open_options() at all. > The latter, however, (VMDK) does not. > > This change to append_open_options results in the extent.%d options > simply being omitted altogether because bdrv_refresh_filename() does not > fetch them. Before, they were included in the VMDK BDS's options, which > is not ideal but works more or less. > > In order to "fix" this, I see three ways right now: > 1. Just care about "file" and "backing". bdrv_refresh_filename() > doesn't support anything else, so that will be fine. > 2. Implement bdrv_refresh_filename() specifically for VMDK so > append_open_options() will never have to handle anything but "file" > and "backing". > 3. Fix bdrv_refresh_filename() so that it handles all children and not > just "file" and "backing". > > Since we are shooting for 2.6 anyway (I assume ;-)), I think we should > go for option 3. This means that this patch is fine, and I'll see to > fixing bdrv_refresh_filename() (because I'm working on that anyway). > > Reviewed-by: Max Reitz <mreitz@redhat.com> Oops, focused so much on this that I missed the issue Wen Congyang found. So this R-b is assuming that is fixed *cough*. Max
Am 27.11.2015 um 18:58 hat Max Reitz geschrieben: > On 23.11.2015 16:59, Kevin Wolf wrote: > > Some drivers have nested options (e.g. blkdebug rule arrays), which > > don't belong to a child node and shouldn't be removed. Don't remove all > > options with "." in their name, but check for the complete prefixes of > > actually existing child nodes. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > block.c | 19 +++++++++++++++---- > > include/block/block_int.h | 1 + > > 2 files changed, 16 insertions(+), 4 deletions(-) > > Thanks, now I don't need to fix it myself. :-) > > (I would have had to do that for an in-work series of mine) > > > diff --git a/block.c b/block.c > > index 23d9e10..02125e2 100644 > > --- a/block.c > > +++ b/block.c > > @@ -1101,11 +1101,13 @@ static int bdrv_fill_options(QDict **options, const char **pfilename, > > > > static BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs, > > BlockDriverState *child_bs, > > + const char *child_name, > > const BdrvChildRole *child_role) > > { > > BdrvChild *child = g_new(BdrvChild, 1); > > *child = (BdrvChild) { > > .bs = child_bs, > > + .name = child_name, > > .role = child_role, > > }; > > > > @@ -1165,7 +1167,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) > > bs->backing = NULL; > > goto out; > > } > > - bs->backing = bdrv_attach_child(bs, backing_hd, &child_backing); > > + bs->backing = bdrv_attach_child(bs, backing_hd, "backing", &child_backing); > > bs->open_flags &= ~BDRV_O_NO_BACKING; > > pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->filename); > > pstrcpy(bs->backing_format, sizeof(bs->backing_format), > > @@ -1322,7 +1324,7 @@ BdrvChild *bdrv_open_child(const char *filename, > > goto done; > > } > > > > - c = bdrv_attach_child(parent, bs, child_role); > > + c = bdrv_attach_child(parent, bs, bdref_key, child_role); > > > > done: > > qdict_del(options, bdref_key); > > @@ -3952,13 +3954,22 @@ static bool append_open_options(QDict *d, BlockDriverState *bs) > > { > > const QDictEntry *entry; > > QemuOptDesc *desc; > > + BdrvChild *child; > > bool found_any = false; > > + const char *p; > > > > for (entry = qdict_first(bs->options); entry; > > entry = qdict_next(bs->options, entry)) > > { > > - /* Only take options for this level */ > > - if (strchr(qdict_entry_key(entry), '.')) { > > + /* Exclude options for children */ > > + QLIST_FOREACH(child, &bs->children, next) { > > + if (strstart(qdict_entry_key(entry), child->name, &p) > > + && (!*p || *p == '.')) > > + { > > + break; > > + } > > + } > > + if (child) { > > continue; > > } > > > > A good general solution, but I think bdrv_refresh_filename() may be bad > enough to break with general solutions. ;-) > > bdrv_refresh_filename() only considers "file" and "backing" (actually, > it only supports "file" for now, I'm working on "backing", though). The > only drivers with other children are quorum, blkdebug, blkverify and > VMDK. The former three have their own implementation of > bdrv_refresh_filename(), so they don't use append_open_options() at all. > The latter, however, (VMDK) does not. > > This change to append_open_options results in the extent.%d options > simply being omitted altogether because bdrv_refresh_filename() does not > fetch them. Before, they were included in the VMDK BDS's options, which > is not ideal but works more or less. Are you sure? As far as I can tell, this patch should only keep options that were previously removed, but not remove options that were previously kept (with the exception of direct use of child names, which I added here to address your review comments for v1). Specifically for "extents.%d", this is a child name and is therefore omitted. However, it contains a '.', so it was already removed without this patch. I'm accepting proof of the contrary in the form of a test case. ;-) > In order to "fix" this, I see three ways right now: > 1. Just care about "file" and "backing". bdrv_refresh_filename() > doesn't support anything else, so that will be fine. > 2. Implement bdrv_refresh_filename() specifically for VMDK so > append_open_options() will never have to handle anything but "file" > and "backing". > 3. Fix bdrv_refresh_filename() so that it handles all children and not > just "file" and "backing". > > Since we are shooting for 2.6 anyway (I assume ;-)), I think we should > go for option 3. This means that this patch is fine, and I'll see to > fixing bdrv_refresh_filename() (because I'm working on that anyway). Yes, I agree. Kevin
On 30.11.2015 10:01, Kevin Wolf wrote: > Am 27.11.2015 um 18:58 hat Max Reitz geschrieben: >> On 23.11.2015 16:59, Kevin Wolf wrote: >>> Some drivers have nested options (e.g. blkdebug rule arrays), which >>> don't belong to a child node and shouldn't be removed. Don't remove all >>> options with "." in their name, but check for the complete prefixes of >>> actually existing child nodes. >>> >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >>> --- >>> block.c | 19 +++++++++++++++---- >>> include/block/block_int.h | 1 + >>> 2 files changed, 16 insertions(+), 4 deletions(-) >> >> Thanks, now I don't need to fix it myself. :-) >> >> (I would have had to do that for an in-work series of mine) >> >>> diff --git a/block.c b/block.c >>> index 23d9e10..02125e2 100644 >>> --- a/block.c >>> +++ b/block.c >>> @@ -1101,11 +1101,13 @@ static int bdrv_fill_options(QDict **options, const char **pfilename, >>> >>> static BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs, >>> BlockDriverState *child_bs, >>> + const char *child_name, >>> const BdrvChildRole *child_role) >>> { >>> BdrvChild *child = g_new(BdrvChild, 1); >>> *child = (BdrvChild) { >>> .bs = child_bs, >>> + .name = child_name, >>> .role = child_role, >>> }; >>> >>> @@ -1165,7 +1167,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) >>> bs->backing = NULL; >>> goto out; >>> } >>> - bs->backing = bdrv_attach_child(bs, backing_hd, &child_backing); >>> + bs->backing = bdrv_attach_child(bs, backing_hd, "backing", &child_backing); >>> bs->open_flags &= ~BDRV_O_NO_BACKING; >>> pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->filename); >>> pstrcpy(bs->backing_format, sizeof(bs->backing_format), >>> @@ -1322,7 +1324,7 @@ BdrvChild *bdrv_open_child(const char *filename, >>> goto done; >>> } >>> >>> - c = bdrv_attach_child(parent, bs, child_role); >>> + c = bdrv_attach_child(parent, bs, bdref_key, child_role); >>> >>> done: >>> qdict_del(options, bdref_key); >>> @@ -3952,13 +3954,22 @@ static bool append_open_options(QDict *d, BlockDriverState *bs) >>> { >>> const QDictEntry *entry; >>> QemuOptDesc *desc; >>> + BdrvChild *child; >>> bool found_any = false; >>> + const char *p; >>> >>> for (entry = qdict_first(bs->options); entry; >>> entry = qdict_next(bs->options, entry)) >>> { >>> - /* Only take options for this level */ >>> - if (strchr(qdict_entry_key(entry), '.')) { >>> + /* Exclude options for children */ >>> + QLIST_FOREACH(child, &bs->children, next) { >>> + if (strstart(qdict_entry_key(entry), child->name, &p) >>> + && (!*p || *p == '.')) >>> + { >>> + break; >>> + } >>> + } >>> + if (child) { >>> continue; >>> } >>> >> >> A good general solution, but I think bdrv_refresh_filename() may be bad >> enough to break with general solutions. ;-) >> >> bdrv_refresh_filename() only considers "file" and "backing" (actually, >> it only supports "file" for now, I'm working on "backing", though). The >> only drivers with other children are quorum, blkdebug, blkverify and >> VMDK. The former three have their own implementation of >> bdrv_refresh_filename(), so they don't use append_open_options() at all. >> The latter, however, (VMDK) does not. >> >> This change to append_open_options results in the extent.%d options >> simply being omitted altogether because bdrv_refresh_filename() does not >> fetch them. Before, they were included in the VMDK BDS's options, which >> is not ideal but works more or less. > > Are you sure? As far as I can tell, this patch should only keep options > that were previously removed, but not remove options that were > previously kept (with the exception of direct use of child names, which > I added here to address your review comments for v1). > > Specifically for "extents.%d", this is a child name and is therefore > omitted. However, it contains a '.', so it was already removed without > this patch. Right, it is broken already. The same applies to qcow2's options containing a dot. Max > I'm accepting proof of the contrary in the form of a test case. ;-) > >> In order to "fix" this, I see three ways right now: >> 1. Just care about "file" and "backing". bdrv_refresh_filename() >> doesn't support anything else, so that will be fine. >> 2. Implement bdrv_refresh_filename() specifically for VMDK so >> append_open_options() will never have to handle anything but "file" >> and "backing". >> 3. Fix bdrv_refresh_filename() so that it handles all children and not >> just "file" and "backing". >> >> Since we are shooting for 2.6 anyway (I assume ;-)), I think we should >> go for option 3. This means that this patch is fine, and I'll see to >> fixing bdrv_refresh_filename() (because I'm working on that anyway). > > Yes, I agree. > > Kevin >
diff --git a/block.c b/block.c index 23d9e10..02125e2 100644 --- a/block.c +++ b/block.c @@ -1101,11 +1101,13 @@ static int bdrv_fill_options(QDict **options, const char **pfilename, static BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs, BlockDriverState *child_bs, + const char *child_name, const BdrvChildRole *child_role) { BdrvChild *child = g_new(BdrvChild, 1); *child = (BdrvChild) { .bs = child_bs, + .name = child_name, .role = child_role, }; @@ -1165,7 +1167,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) bs->backing = NULL; goto out; } - bs->backing = bdrv_attach_child(bs, backing_hd, &child_backing); + bs->backing = bdrv_attach_child(bs, backing_hd, "backing", &child_backing); bs->open_flags &= ~BDRV_O_NO_BACKING; pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->filename); pstrcpy(bs->backing_format, sizeof(bs->backing_format), @@ -1322,7 +1324,7 @@ BdrvChild *bdrv_open_child(const char *filename, goto done; } - c = bdrv_attach_child(parent, bs, child_role); + c = bdrv_attach_child(parent, bs, bdref_key, child_role); done: qdict_del(options, bdref_key); @@ -3952,13 +3954,22 @@ static bool append_open_options(QDict *d, BlockDriverState *bs) { const QDictEntry *entry; QemuOptDesc *desc; + BdrvChild *child; bool found_any = false; + const char *p; for (entry = qdict_first(bs->options); entry; entry = qdict_next(bs->options, entry)) { - /* Only take options for this level */ - if (strchr(qdict_entry_key(entry), '.')) { + /* Exclude options for children */ + QLIST_FOREACH(child, &bs->children, next) { + if (strstart(qdict_entry_key(entry), child->name, &p) + && (!*p || *p == '.')) + { + break; + } + } + if (child) { continue; } diff --git a/include/block/block_int.h b/include/block/block_int.h index 77dc165..b2325aa 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -351,6 +351,7 @@ extern const BdrvChildRole child_format; struct BdrvChild { BlockDriverState *bs; + const char *name; const BdrvChildRole *role; QLIST_ENTRY(BdrvChild) next; QLIST_ENTRY(BdrvChild) next_parent;
Some drivers have nested options (e.g. blkdebug rule arrays), which don't belong to a child node and shouldn't be removed. Don't remove all options with "." in their name, but check for the complete prefixes of actually existing child nodes. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block.c | 19 +++++++++++++++---- include/block/block_int.h | 1 + 2 files changed, 16 insertions(+), 4 deletions(-)