| Message ID | 1306929912-15484-1-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. 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. Perhaps we should just catch the error when set_option_parameter_int() fails? Stefan
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). > 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. Adding some error handling there, too, wouldn't hurt, though. Kevin
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
diff --git a/block.c b/block.c index ebd1501..8df2a5f 100644 --- a/block.c +++ b/block.c @@ -2900,7 +2900,7 @@ int bdrv_img_create(const char *filename, const char *fmt, char *options, uint64_t img_size, int flags) { QEMUOptionParameter *param = NULL, *create_options = NULL; - QEMUOptionParameter *backing_fmt, *backing_file; + QEMUOptionParameter *backing_fmt, *backing_file, *size; BlockDriverState *bs = NULL; BlockDriver *drv, *proto_drv; BlockDriver *backing_drv = NULL; @@ -2983,7 +2983,8 @@ int bdrv_img_create(const char *filename, const char *fmt, // The size for the image must always be specified, with one exception: // If we are using a backing file, we can obtain the size from there - if (get_option_parameter(param, BLOCK_OPT_SIZE)->value.n == -1) { + size = get_option_parameter(param, BLOCK_OPT_SIZE); + if (size && size->value.n == -1) { if (backing_file && backing_file->value.s) { uint64_t size; char buf[32];
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(-)