Re: [Qemu-devel] [PATCH v2 RFC] block/vxhs: Initial commit to add Veritas HyperScale VxHS block device support

[Daniel P. Berrange]

On Sun, Aug 07, 2016 at 07:01:01PM -0700, Ashish Mittal wrote:
> This patch adds support for a new block device type called "vxhs".

I'd suggest this commit message should include an example of the
syntax for using this on the CLI

> Source code for the library that this code loads can be downloaded from:
> https://github.com/MittalAshish/libqnio.git
> 
> Version 2 patch submission fixes the following issues:
> (1) Removed code to dlopen library. We now check if libqnio is installed 
> during
>     configure, and directly link with it.
> (2) Changed file headers to mention GPLv2-or-later license.
> (3) Removed unnecessary type casts and inlines.
> (4) Removed custom tokenize function and modified code to use g_strsplit.
> (5) Replaced malloc/free with g_new/g_free and removed code that checks for
>     memory allocation failure conditions.
> (6) Removed some block ops implementations that were place-holders only.
> (7) Removed all custom debug messages. Added new messages in 
> block/trace-events
> (8) Other miscellaneous corrections.

FYI, if you put notes about what changed in between postings below the
'---' line, then with your patch is applied, git is intelligent enough
to drop these notes.

> 
> TODO: QAPI changes and (fixes to) review comments from Stefan.
> Signed-off-by: Ashish Mittal <address@hidden>
> ---

eg, put your 'Changed in v2' stuff just here.

>  block/Makefile.objs |    2 +
>  block/trace-events  |   40 ++
>  block/vxhs.c        | 1199 
> +++++++++++++++++++++++++++++++++++++++++++++++++++
>  block/vxhs.h        |  294 +++++++++++++
>  configure           |   50 +++
>  5 files changed, 1585 insertions(+)
>  create mode 100644 block/vxhs.c
>  create mode 100644 block/vxhs.h
> 


> diff --git a/block/vxhs.c b/block/vxhs.c
> new file mode 100644
> index 0000000..9a82757
> --- /dev/null
> +++ b/block/vxhs.c
> @@ -0,0 +1,1199 @@
> +/*
> + * QEMU Block driver for Veritas HyperScale (VxHS)
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "vxhs.h"
> +#include <qnio/qnio_api.h>
> +#include "trace.h"
> +
> +/* qnio client ioapi_ctx */
> +static void __attribute__((unused)) *global_qnio_ctx;
> +
> +/* insure init once */
> +static pthread_mutex_t __attribute__((unused))  of_global_ctx_lock =
> +    PTHREAD_MUTEX_INITIALIZER;

Not sure why you're adding attribute__(unused) here, since both these
vars are clearly used.

BTW, since we're using glib, you should use  G_GNUC_UNUSD as the
annotation, but since these vars are used, just drop the annotation
entirely.

> +
> +/* HyperScale Driver Version */
> +static const int __attribute__((unused)) vxhs_drv_version = 8895;

This only seems to be used in a single trace() message, so can probably
be just deleted entirely


> diff --git a/configure b/configure
> index f57fcc6..c4f6261 100755
> --- a/configure
> +++ b/configure
> @@ -320,6 +320,7 @@ vhdx=""
>  numa=""
>  tcmalloc="no"
>  jemalloc="no"
> +vxhs="yes"
>  
>  # parse CC options first
>  for opt do
> @@ -1150,6 +1151,11 @@ for opt do
>    ;;
>    --enable-jemalloc) jemalloc="yes"
>    ;;
> +  --disable-vxhs) vxhs="no"
> +  ;;
> +  --enable-vxhs) vxhs="yes"
> +  ;;
> +
>    *)
>        echo "ERROR: unknown option $opt"
>        echo "Try '$0 --help' for more information"
> @@ -1380,6 +1386,7 @@ disabled with --disable-FEATURE, default is enabled if 
> available:
>    numa            libnuma support
>    tcmalloc        tcmalloc support
>    jemalloc        jemalloc support
> +  vxhs            Veritas HyperScale vDisk backend support
>  
>  NOTE: The object files are built at the place where configure is launched
>  EOF
> @@ -4543,6 +4550,43 @@ if do_cc -nostdlib -Wl,-r -Wl,--no-relax -o $TMPMO 
> $TMPO; then
>  fi
>  
>  ##########################################
> +# Veritas HyperScale block driver VxHS
> +# Check if libqnio is installed
> +if test "$vxhs" != "no" ; then
> +  cat > $TMPC <<EOF
> +#include <stdio.h>
> +#include <qnio/qnio_api.h>
> +
> +void vxhs_inc_acb_segment_count(void *acb, int count);
> +void vxhs_dec_acb_segment_count(void *acb, int count);
> +void vxhs_set_acb_buffer(void *ptr, void *buffer);
> +
> +void vxhs_inc_acb_segment_count(void *ptr, int count)
> +{
> +}
> +void vxhs_dec_acb_segment_count(void *ptr, int count)
> +{
> +}
> +void vxhs_set_acb_buffer(void *ptr, void *buffer)
> +{
> +}
> +int main(void) {
> +    qemu_ck_initialize_lock();
> +    return 0;
> +}
> +EOF
> +  vxhs_libs="-lqnioshim -lqnio"
> +  if compile_prog "" "$vxhs_libs" ; then
> +    vxhs=yes
> +  else
> +    if test "$vxhs" = "yes" ; then
> +      feature_not_found "vxhs block device" "Install libqnio. See github"
> +    fi
> +    vxhs=no
> +  fi
> +fi

There's nothing wrong with this code, but it occurs to me that
since libqnio is a completely new library you've written yourself,
it would be nice if you could add a pkg-config file to the libqio
install. That would allow QEMU to detect libqnio using the standard
pkg-config mechanism that most libraries uses these days.



Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|