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

[Prasanna Kalever]

On Fri, Jul 22, 2016 at 7:37 PM, Eric Blake <address@hidden> wrote:
> 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
>>

done!

>
>> +++ 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?

Dropping

>
>>  #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"?

you caught it :) It was unintentional, did a copy paste from v2
dropping

>
>> +                                    "[,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.

oh yeah

>
>>                               "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);

Looks good to me, picking this up

>
>> +++ 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.

Not sure, but this is really needed, at least while image creation
these fills all the stderr

Thanks Eric!

Cheers,
--
Prasanna

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