Re: [Qemu-devel] [PATCH] bdrv_img_create: Fix segfault

[Stefan Hajnoczi <stefanha@gmail.com>]

On Mon, Jun 6, 2011 at 11:00 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 02.06.2011 00:34, schrieb Stefan Hajnoczi:
>> On Wed, Jun 1, 2011 at 1:05 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>>> Block drivers that don't support creating images don't have a size option. Fail
>>> gracefully instead of segfaulting when trying to access the option's value.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>  block.c |    5 +++--
>>>  1 files changed, 3 insertions(+), 2 deletions(-)
>>
>> A command-line to reproduce the crash would be nice.
>
> qemu-img create -f bochs nbd:foo 32M
>
> It doesn't happen with a file protocol any more since we merge the
> create options of the protocol with those of the format (was introduced
> with Sheepdog).

Interesting, I was just looking at the format + protocol
create_options merging because I tried to replace QEMUOptionParameter
with QemuOpts.  The idea was to make the block layer use QemuOpts and
then introduce a BlockDriver open_options QemuOptsList so that image
format parameters like image streaming or copy-on-read can be
specified on launch.

Here is where I got to:

http://repo.or.cz/w/qemu/stefanha.git/commitdiff/b49babb2c8b476a36357cfd7276ca45a11039ca5

The main thing stopping me from dropping QEMUOptionParameter
completely is this merging behavior.  Any suggestions?

>> I noticed this line above your fix:
>> set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size);
>>
>> If I follow correctly there should be an "Unknown option 'size'" error
>> message before set_option_parameter_int() returns -1 (which we ignore)
>> and then crash.
>
> Right, this is what happens.
>
>> Perhaps we should just catch the error when set_option_parameter_int() fails?
>
> We could do that, but the segfault isn't really related to a failing
> set_option_parameter_int() but to the failing get_option_parameter(). I
> think it's good style not to rely on the error handling of an unrelated
> action some lines above.

It's also bad to keep executing code after something has broken, which
is the case when set_option_parameter_int() fails but we ignore the
return value.

Thanks for explaining, I now understand your fix better and am happy with it.

Stefan