| Message ID | 1469188874-9292-1-git-send-email-prasanna.kalever@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
On Fri, Jul 22, 2016 at 7:37 PM, Eric Blake <eblake@redhat.com> 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 >
diff --git a/block/gluster.c b/block/gluster.c index 01b479f..51a1089 100644 --- a/block/gluster.c +++ 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" #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; @@ -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]]volname/image[?socket=...]" + "[,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," "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); + } + + gconf->logfile = g_strdup(s->logfile); + gconf->has_logfile = true; + s->glfs = qemu_gluster_init(gconf, filename, options, errp); if (!s->glfs) { ret = -errno; @@ -738,6 +765,7 @@ out: if (!ret) { return ret; } + g_free(s->logfile); if (s->fd) { glfs_close(s->fd); } @@ -769,6 +797,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 +944,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 +1061,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
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 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 <prasanna.kalever@redhat.com> --- v3: rebased on master, which is now QMP compatible. v2: address comments from Jeff Cody, thanks Jeff! v1: initial patch --- block/gluster.c | 47 ++++++++++++++++++++++++++++++++++++++++++----- qapi/block-core.json | 5 ++++- 2 files changed, 46 insertions(+), 6 deletions(-)