[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] bdrv_img_create: Fix segfault
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH] bdrv_img_create: Fix segfault |
Date: |
Mon, 6 Jun 2011 12:10:55 +0100 |
On Mon, Jun 6, 2011 at 11:00 AM, Kevin Wolf <address@hidden> wrote:
> Am 02.06.2011 00:34, schrieb Stefan Hajnoczi:
>> On Wed, Jun 1, 2011 at 1:05 PM, Kevin Wolf <address@hidden> 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 <address@hidden>
>>> ---
>>> 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