Re: [Qemu-devel] [PATCH RFC v3 for-2.9 02/11] rbd: Fix to cleanly reject -drive without pool or image

[Jeff Cody]

On Mon, Mar 27, 2017 at 03:26:26PM +0200, Markus Armbruster wrote:
> qemu_rbd_open() neglects to check pool and image are present.
> Reproducer:
> 
>     $ qemu-system-x86_64 -nodefaults -drive if=none,driver=rbd,pool=p
>     Segmentation fault (core dumped)

This reproducer is wrong, I think.  Omitting the image should be caught
earlier, but it is an error caught by the rbd_open() call.

What doesn't work is omitting the pool name, and that causes an abort()
from rados_ioctx_create(), e.g.:


$ qemu-system-x86_64 -nodefaults -drive 
driver=rbd,id=rbd,image=i,server.0.port=6789,server.0.host=192.168.15.180
terminate called after throwing an instance of 'std::logic_error'
  what():  basic_string::_M_construct null not valid
Aborted (core dumped)


>     $ qemu-system-x86_64 -nodefaults -drive if=none,driver=rbd,image=i
>     qemu-system-x86_64: -drive if=none,driver=rbd,image=i: error opening pool 
> (null)
> 
> Doesn't affect -drive with file=..., because qemu_rbd_parse_filename()
> always sets both pool and image.
> 
> Doesn't affect -blockdev, because pool and image are mandatory in the
> QAPI schema.
> 
> Fix by adding the missing checks.
> 
> Signed-off-by: Markus Armbruster <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>

With an updated commit message:

Reviewed-by: Jeff Cody <address@hidden>

> ---
>  block/rbd.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index ee13f3d..5ba2a87 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -711,6 +711,12 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
> *options, int flags,
>      name           = qemu_opt_get(opts, "image");
>      keypairs       = qemu_opt_get(opts, "keyvalue-pairs");
>  
> +    if (!pool || !name) {
> +        error_setg(errp, "Parameters 'pool' and 'image' are required");
> +        r = -EINVAL;
> +        goto failed_opts;
> +    }
> +
>      r = rados_create(&s->cluster, clientname);
>      if (r < 0) {
>          error_setg_errno(errp, -r, "error initializing");
> @@ -718,9 +724,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
> *options, int flags,
>      }
>  
>      s->snap = g_strdup(snap);
> -    if (name) {
> -        pstrcpy(s->name, RBD_MAX_IMAGE_NAME_SIZE, name);
> -    }
> +    pstrcpy(s->name, RBD_MAX_IMAGE_NAME_SIZE, name);
>  
>      /* try default location when conf=NULL, but ignore failure */
>      r = rados_conf_read_file(s->cluster, conf);
> -- 
> 2.7.4
>