Re: [Qemu-devel] [PATCH v2 19/36] rbd: Factor out qemu_rbd_connect()

[Kevin Wolf]

Am 23.02.2018 um 00:10 hat Max Reitz geschrieben:
> On 2018-02-21 14:53, Kevin Wolf wrote:
> > The code to establish an RBD connection is duplicated between open and
> > create. In order to be able to share the code, factor out the code from
> > qemu_rbd_open() as a first step.
> > 
> > Signed-off-by: Kevin Wolf 
> > ---
> >  block/rbd.c | 100 
> > 
> >  1 file changed, 60 insertions(+), 40 deletions(-)
> > 
> > diff --git a/block/rbd.c b/block/rbd.c
> > index 27fa11b473..4bbcce4eca 100644
> > --- a/block/rbd.c
> > +++ b/block/rbd.c
> > @@ -544,32 +544,17 @@ out:
> >  return rados_str;
> >  }
> >  
> > -static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
> > - Error **errp)
> > +static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
> > +char **s_snap, char **s_image_name,
> > +QDict *options, bool cache, Error **errp)
> 
> Bikeshedding ahead:  Maybe this should be called qemu_rados_connect()?
> I don't know anything about this, but there seems to be a distinction
> between rados_* functions and rbd_* functions -- the former work on the
> pool, the latter on the single block device.
> 
> Since this function only connects to the pool and not to a single device
> within, I think it should be called qemu_rados_connect() instead of
> qemu_rbd_connect().
> 
> (Also because qemu_rbd_connect() seems so similar to qemu_rbd_open().)

I think librados is the lower level interface, and librbd builds a
higher level interface on top of it. But I don't know anything about the
details either.

However, for functions in the block driver, qemu_rbd_* is the only
prefix used, there is no qemu_rados_* function. So I assume the prefix
comes from the block driver name 'rbd' rather than which library it
accesses, and that it would be better to keep qemu_rbd_connect().

> Up to you:
> 
> Reviewed-by: Max Reitz 

Thanks.

Kevin


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v2 19/36] rbd: Factor out qemu_rbd_connect()

[Max Reitz]

On 2018-02-21 14:53, Kevin Wolf wrote:
> The code to establish an RBD connection is duplicated between open and
> create. In order to be able to share the code, factor out the code from
> qemu_rbd_open() as a first step.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/rbd.c | 100 
> 
>  1 file changed, 60 insertions(+), 40 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index 27fa11b473..4bbcce4eca 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -544,32 +544,17 @@ out:
>  return rados_str;
>  }
>  
> -static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
> - Error **errp)
> +static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
> +char **s_snap, char **s_image_name,
> +QDict *options, bool cache, Error **errp)

Bikeshedding ahead:  Maybe this should be called qemu_rados_connect()?
I don't know anything about this, but there seems to be a distinction
between rados_* functions and rbd_* functions -- the former work on the
pool, the latter on the single block device.

Since this function only connects to the pool and not to a single device
within, I think it should be called qemu_rados_connect() instead of
qemu_rbd_connect().

(Also because qemu_rbd_connect() seems so similar to qemu_rbd_open().)

Up to you:

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature