Re: [Qemu-devel] [PATCH v3] block/gluster: add support to choose libgfapi logfile

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4483 bytes --]

On 07/22/2016 06:01 AM, Prasanna Kumar Kalever wrote:
> currently all the libgfapi logs defaults to '/dev/stderr' as it was hardcoded
> in a call to glfs logging api, in case if debug level is chosen to DEBUG/TRACE

s/api, in case if/api. When the/
s/TRACE/TRACE,/

> gfapi logs will be huge and fill/overflow the console view.
> 
> this patch provides a commandline option to mention log file path which helps

s/this/This/

> in logging to the specified file and also help in persisting the gfapi logs.
> 
> Usage:
> -----
>  *URI Style:
>   ---------
>   -drive file=gluster://hostname/volname/image.qcow2,file.debug=9,\
>                       file.logfile=/var/log/qemu/qemu-gfapi.log
> 

> +++ b/block/gluster.c
> @@ -26,10 +26,12 @@
>  #define GLUSTER_OPT_IPV4            "ipv4"
>  #define GLUSTER_OPT_IPV6            "ipv6"
>  #define GLUSTER_OPT_SOCKET          "socket"
> -#define GLUSTER_OPT_DEBUG           "debug"
>  #define GLUSTER_DEFAULT_PORT        24007
> +#define GLUSTER_OPT_DEBUG           "debug"

Why move this line?

>  #define GLUSTER_DEBUG_DEFAULT       4
>  #define GLUSTER_DEBUG_MAX           9
> +#define GLUSTER_OPT_LOGFILE         "logfile"
> +#define GLUSTER_LOGFILE_DEFAULT     "-" /* handled in libgfapi as /dev/stderr */
>  

> @@ -576,7 +589,9 @@ static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf,
>          if (ret < 0) {
>              error_setg(errp, "invalid URI");
>              error_append_hint(errp, "Usage: file=gluster[+transport]://"
> -                                    "[host[:port]]/volume/path[?socket=...]\n");
> +                                    "[host[:port]]volname/image[?socket=...]"

Why did you change absolute "/volume/path" to relative "volname/image"?

> +                                    "[,file.debug=N]"
> +                                    "[,file.logfile=/path/filename.log]\n");
>              errno = -ret;
>              return NULL;
>          }
> @@ -586,7 +601,8 @@ static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf,
>              error_append_hint(errp, "Usage: "
>                               "-drive driver=qcow2,file.driver=gluster,"
>                               "file.volume=testvol,file.path=/path/a.qcow2"
> -                             "[,file.debug=9],file.server.0.type=tcp,"
> +                             "[,file.debug=9][,file.logfile=/path/filename.log]"
> +                             "file.server.0.type=tcp,"

Missing a comma between the file.logfile and file.server keys.

>                               "file.server.0.host=1.2.3.4,"
>                               "file.server.0.port=24007,"
>                               "file.server.1.transport=unix,"
> @@ -677,7 +693,7 @@ static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
>      BlockdevOptionsGluster *gconf = NULL;
>      QemuOpts *opts;
>      Error *local_err = NULL;
> -    const char *filename;
> +    const char *filename, *logfile;
>  
>      opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
>      qemu_opts_absorb_qdict(opts, options, &local_err);
> @@ -700,6 +716,17 @@ static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
>      gconf = g_new0(BlockdevOptionsGluster, 1);
>      gconf->debug_level = s->debug_level;
>      gconf->has_debug_level = true;
> +
> +    logfile = qemu_opt_get(opts, GLUSTER_OPT_LOGFILE);
> +    if (logfile) {
> +        s->logfile = g_strdup(logfile);
> +    } else {
> +        s->logfile = g_strdup(GLUSTER_LOGFILE_DEFAULT);
> +    }

I might have written
s->logfile = g_strdup(logfile ? logfile : GLUSTER_LOGFILE_DEFAULT);

> +++ b/qapi/block-core.json
> @@ -2138,13 +2138,16 @@
>  #
>  # @debug-level: #optional libgfapi log level (default '4' which is Error)
>  #
> +# @logfile:     #optional libgfapi log file (default /dev/stderr)
> +#
>  # Since: 2.7
>  ##
>  { 'struct': 'BlockdevOptionsGluster',
>    'data': { 'volume': 'str',
>              'path': 'str',
>              'server': ['GlusterServer'],
> -            '*debug_level': 'int' } }
> +            '*debug_level': 'int',
> +            '*logfile': 'str' } }

Very borderline on whether this qualifies as a bugfix, or if it is a
feature to be deferred to 2.8.  I'll let the maintainer chime in.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]