| Message ID | 1416924485-13304-2-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
Max Reitz <mreitz@redhat.com> writes: > Albeit absolutely impossible right now, bdrv_find_format("qcow2") may > fail. bdrv_append_temp_snapshot() should heed that case. Impossible because we always compile in bdrv_qcow2. > Cc: qemu-stable@nongnu.org > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/block.c b/block.c > index 866c8b4..b31fb67 100644 > --- a/block.c > +++ b/block.c > @@ -1320,6 +1320,12 @@ int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp) > } > > bdrv_qcow2 = bdrv_find_format("qcow2"); > + if (!bdrv_qcow2) { > + error_setg(errp, "Failed to locate qcow2 driver"); > + ret = -ENOENT; > + goto out; > + } > + > opts = qemu_opts_create(bdrv_qcow2->create_opts, NULL, 0, > &error_abort); > qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_size); This dynamic qcow2 driver lookup business is silly. Compiling without qcow2 would be a massive loss of functionality, and I wouldn't bet a nickel on the error paths it would pring to live. Even sillier lookups: "file" in bdrv_find_protocol(), and "raw" in find_image_format(). Statically linking to them would be simpler and more honest. Aside: similar silliness exists around QemuOpts. Patch looks correct.
On 2014-11-26 at 08:23, Markus Armbruster wrote: > Max Reitz <mreitz@redhat.com> writes: > >> Albeit absolutely impossible right now, bdrv_find_format("qcow2") may >> fail. bdrv_append_temp_snapshot() should heed that case. > Impossible because we always compile in bdrv_qcow2. Right now we do, right. >> Cc: qemu-stable@nongnu.org >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> block.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/block.c b/block.c >> index 866c8b4..b31fb67 100644 >> --- a/block.c >> +++ b/block.c >> @@ -1320,6 +1320,12 @@ int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp) >> } >> >> bdrv_qcow2 = bdrv_find_format("qcow2"); >> + if (!bdrv_qcow2) { >> + error_setg(errp, "Failed to locate qcow2 driver"); >> + ret = -ENOENT; >> + goto out; >> + } >> + >> opts = qemu_opts_create(bdrv_qcow2->create_opts, NULL, 0, >> &error_abort); >> qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_size); > This dynamic qcow2 driver lookup business is silly. Compiling without > qcow2 would be a massive loss of functionality, and I wouldn't bet a > nickel on the error paths it would pring to live. True. > Even sillier lookups: "file" in bdrv_find_protocol(), and "raw" in > find_image_format(). > > Statically linking to them would be simpler and more honest. > > Aside: similar silliness exists around QemuOpts. > > Patch looks correct. Well, at least it will silence Coverity... Max
On 11/26/2014 02:13 AM, Max Reitz wrote: > On 2014-11-26 at 08:23, Markus Armbruster wrote: >> Max Reitz <mreitz@redhat.com> writes: >> >>> Albeit absolutely impossible right now, bdrv_find_format("qcow2") may >>> fail. bdrv_append_temp_snapshot() should heed that case. >> Impossible because we always compile in bdrv_qcow2. > >>> +++ b/block.c >>> @@ -1320,6 +1320,12 @@ int bdrv_append_temp_snapshot(BlockDriverState >>> *bs, int flags, Error **errp) >>> } >>> bdrv_qcow2 = bdrv_find_format("qcow2"); >>> + if (!bdrv_qcow2) { Would it be shorter to 'assert(bdrv_qcow2);' to still silence Coverity?
On 2014-11-26 at 16:19, Eric Blake wrote: > On 11/26/2014 02:13 AM, Max Reitz wrote: >> On 2014-11-26 at 08:23, Markus Armbruster wrote: >>> Max Reitz <mreitz@redhat.com> writes: >>> >>>> Albeit absolutely impossible right now, bdrv_find_format("qcow2") may >>>> fail. bdrv_append_temp_snapshot() should heed that case. >>> Impossible because we always compile in bdrv_qcow2. >>>> +++ b/block.c >>>> @@ -1320,6 +1320,12 @@ int bdrv_append_temp_snapshot(BlockDriverState >>>> *bs, int flags, Error **errp) >>>> } >>>> bdrv_qcow2 = bdrv_find_format("qcow2"); >>>> + if (!bdrv_qcow2) { > Would it be shorter to 'assert(bdrv_qcow2);' to still silence Coverity? Sounds like a good compromise. Will do. Max
Am 26.11.2014 um 16:20 hat Max Reitz geschrieben: > On 2014-11-26 at 16:19, Eric Blake wrote: > >On 11/26/2014 02:13 AM, Max Reitz wrote: > >>On 2014-11-26 at 08:23, Markus Armbruster wrote: > >>>Max Reitz <mreitz@redhat.com> writes: > >>> > >>>>Albeit absolutely impossible right now, bdrv_find_format("qcow2") may > >>>>fail. bdrv_append_temp_snapshot() should heed that case. > >>>Impossible because we always compile in bdrv_qcow2. > >>>>+++ b/block.c > >>>>@@ -1320,6 +1320,12 @@ int bdrv_append_temp_snapshot(BlockDriverState > >>>>*bs, int flags, Error **errp) > >>>> } > >>>> bdrv_qcow2 = bdrv_find_format("qcow2"); > >>>>+ if (!bdrv_qcow2) { > >Would it be shorter to 'assert(bdrv_qcow2);' to still silence Coverity? > > Sounds like a good compromise. Will do. I think it's better to have either proper error handling for the case that someone compiles it out, like implemented by this patch, or to reference the symbol so that compiling it out already breaks the build. The assert() would potentially be a crash of a running VM, which is not as nice. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 26.11.2014 um 16:20 hat Max Reitz geschrieben: >> On 2014-11-26 at 16:19, Eric Blake wrote: >> >On 11/26/2014 02:13 AM, Max Reitz wrote: >> >>On 2014-11-26 at 08:23, Markus Armbruster wrote: >> >>>Max Reitz <mreitz@redhat.com> writes: >> >>> >> >>>>Albeit absolutely impossible right now, bdrv_find_format("qcow2") may >> >>>>fail. bdrv_append_temp_snapshot() should heed that case. >> >>>Impossible because we always compile in bdrv_qcow2. >> >>>>+++ b/block.c >> >>>>@@ -1320,6 +1320,12 @@ int bdrv_append_temp_snapshot(BlockDriverState >> >>>>*bs, int flags, Error **errp) >> >>>> } >> >>>> bdrv_qcow2 = bdrv_find_format("qcow2"); >> >>>>+ if (!bdrv_qcow2) { >> >Would it be shorter to 'assert(bdrv_qcow2);' to still silence Coverity? >> >> Sounds like a good compromise. Will do. > > I think it's better to have either proper error handling for the case > that someone compiles it out, like implemented by this patch, or to > reference the symbol so that compiling it out already breaks the build. > The assert() would potentially be a crash of a running VM, which is not > as nice. Ways to bind to a well-known block driver, and how the binding fails when the driver isn't around, in decreasing order of preference: 1. Static binding Fails at compile time. Me like. 2. Dynamic binding, assert the required code is there Fails at run time in a catastrophic, but locally obvious way. 3. Dynamic binding without error checking Fails at run time in a catastrophic way, when the pointer gets dereferenced. Less predictable than 2. 4. Dynamic binding, error out if the required code is there Fails at run time in a way that could be recoverable, but probably isn't, not least because nobody ever tests it. Moreover, code clutter. Use of dynamic binding to reference a well-known block driver feels like "Look ma, dynamic binding in C!" to me.
diff --git a/block.c b/block.c index 866c8b4..b31fb67 100644 --- a/block.c +++ b/block.c @@ -1320,6 +1320,12 @@ int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp) } bdrv_qcow2 = bdrv_find_format("qcow2"); + if (!bdrv_qcow2) { + error_setg(errp, "Failed to locate qcow2 driver"); + ret = -ENOENT; + goto out; + } + opts = qemu_opts_create(bdrv_qcow2->create_opts, NULL, 0, &error_abort); qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_size);
Albeit absolutely impossible right now, bdrv_find_format("qcow2") may fail. bdrv_append_temp_snapshot() should heed that case. Cc: qemu-stable@nongnu.org Signed-off-by: Max Reitz <mreitz@redhat.com> --- block.c | 6 ++++++ 1 file changed, 6 insertions(+)