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

[Markus Armbruster]

Prasanna Kumar Kalever <address@hidden> writes:

> currently all the libgfapi logs defaults to '/dev/stderr' as it was hardcoded
> in a call to glfs logging api. When the debug level is chosen to DEBUG/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
> 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
>
>  *JSON Style:
>   ----------
>   'json:{
>            "driver":"qcow2",
>            "file":{
>               "driver":"gluster",
>               "volume":"volname",
>               "path":"image.qcow2",
>               "debug":"9",
>               "logfile":"/var/log/qemu/qemu-gfapi.log",
>               "server":[
>                  {
>                     "type":"tcp",
>                     "host":"1.2.3.4",
>                     "port":24007
>                  },
>                  {
>                     "type":"unix",
>                     "socket":"/var/run/glusterd.socket"
>                  }
>               ]
>            }
>         }'
>
> Signed-off-by: Prasanna Kumar Kalever <address@hidden>
> ---
> v4: address review comments from Eric Blake on v3.
> v3: rebased on master, which is now QMP compatible.
> v2: address comments from Jeff Cody, thanks Jeff!
> v1: initial patch
> ---
>  block/gluster.c      | 42 ++++++++++++++++++++++++++++++++++++++----
>  qapi/block-core.json |  5 ++++-
>  2 files changed, 42 insertions(+), 5 deletions(-)
>
> diff --git a/block/gluster.c b/block/gluster.c
> index 01b479f..e7bd13c 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -30,6 +30,8 @@
>  #define GLUSTER_DEFAULT_PORT        24007
>  #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 */
>  
>  #define GERR_INDEX_HINT "hint: check in 'server' array index '%d'\n"
>  
> @@ -44,6 +46,7 @@ typedef struct GlusterAIOCB {
>  typedef struct BDRVGlusterState {
>      struct glfs *glfs;
>      struct glfs_fd *fd;
> +    char *logfile;
>      bool supports_seek_data;
>      int debug_level;
>  } BDRVGlusterState;

Outside this patch's scope, but have you considered embedding
BlockdevOptionsGluster in BDRVGlusterState instead of selectively
duplicating it there?  Mind, I'm not claiming it would be an
improvement, I just wonder whether it would be.

> @@ -73,6 +76,11 @@ static QemuOptsList qemu_gluster_create_opts = {
>              .type = QEMU_OPT_NUMBER,
>              .help = "Gluster log level, valid range is 0-9",
>          },
> +        {
> +            .name = GLUSTER_OPT_LOGFILE,
> +            .type = QEMU_OPT_STRING,
> +            .help = "Logfile path of libgfapi",
> +        },
>          { /* end of list */ }
>      }
>  };
> @@ -91,6 +99,11 @@ static QemuOptsList runtime_opts = {
>              .type = QEMU_OPT_NUMBER,
>              .help = "Gluster log level, valid range is 0-9",
>          },
> +        {
> +            .name = GLUSTER_OPT_LOGFILE,
> +            .type = QEMU_OPT_STRING,
> +            .help = "Logfile path of libgfapi",
> +        },
>          { /* end of list */ }
>      },
>  };
> @@ -341,7 +354,7 @@ static struct glfs 
> *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
>          }
>      }
>  
> -    ret = glfs_set_logging(glfs, "-", gconf->debug_level);
> +    ret = glfs_set_logging(glfs, gconf->logfile, gconf->debug_level);
>      if (ret < 0) {
>          goto out;
>      }
> @@ -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]]volume/path[?socket=...]"
> +                                    "[,file.debug=N]"
> +                                    "[,file.logfile=/path/filename.log]\n");
>              errno = -ret;
>              return NULL;
>          }
> @@ -586,7 +601,9 @@ 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,"
>                               "file.server.0.host=1.2.3.4,"
>                               "file.server.0.port=24007,"
>                               "file.server.1.transport=unix,"
> @@ -677,7 +694,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 +717,13 @@ 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);
> +    s->logfile = g_strdup(logfile ? logfile : GLUSTER_LOGFILE_DEFAULT);

One copy for BDRVGlusterState, and ...

> +
> +    gconf->logfile = g_strdup(s->logfile);

... another copy for BlockdevOptionsGluster.  The latter will be freed
on return from this function.  Not a problem, it's just what made me ask
about embedding.

> +    gconf->has_logfile = true;
> +

Optional, but always present.  If that's what you want...

>      s->glfs = qemu_gluster_init(gconf, filename, options, errp);
>      if (!s->glfs) {
>          ret = -errno;
> @@ -738,6 +762,7 @@ out:
>      if (!ret) {
>          return ret;
>      }
> +    g_free(s->logfile);
>      if (s->fd) {
>          glfs_close(s->fd);
>      }
> @@ -769,6 +794,8 @@ static int qemu_gluster_reopen_prepare(BDRVReopenState 
> *state,
>      gconf = g_new0(BlockdevOptionsGluster, 1);
>      gconf->debug_level = s->debug_level;
>      gconf->has_debug_level = true;
> +    gconf->logfile = g_strdup(s->logfile);
> +    gconf->has_logfile = true;
>      reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, NULL, errp);
>      if (reop_s->glfs == NULL) {
>          ret = -errno;
> @@ -914,6 +941,12 @@ static int qemu_gluster_create(const char *filename,
>      }
>      gconf->has_debug_level = true;
>  
> +    gconf->logfile = qemu_opt_get_del(opts, GLUSTER_OPT_LOGFILE);
> +    if (!gconf->logfile) {
> +        gconf->logfile = g_strdup(GLUSTER_LOGFILE_DEFAULT);
> +    }
> +    gconf->has_logfile = true;
> +
>      glfs = qemu_gluster_init(gconf, filename, NULL, errp);
>      if (!glfs) {
>          ret = -errno;
> @@ -1025,6 +1058,7 @@ static void qemu_gluster_close(BlockDriverState *bs)
>  {
>      BDRVGlusterState *s = bs->opaque;
>  
> +    g_free(s->logfile);
>      if (s->fd) {
>          glfs_close(s->fd);
>          s->fd = NULL;
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index f462345..bed531d 100644
> --- a/qapi/block-core.json
> +++ 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' } }
>  
>  ##
>  # @BlockdevOptions

Simple conflict with commit 0a189ff "block/gluster: fix doc in the qapi
schema and member name".  Perhaps the maintainer can resolve it for you.

I'm not too familiar with current block drivers, but I'll risk giving my
Reviewed-by: Markus Armbruster <address@hidden>