* [Qemu-devel] [PATCH v4] block/gluster: add support to choose libgfapi logfile
@ 2016-07-22 14:56 Prasanna Kumar Kalever
2016-08-04 10:16 ` Markus Armbruster
0 siblings, 1 reply; 2+ messages in thread
From: Prasanna Kumar Kalever @ 2016-07-22 14:56 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, eblake, jcody, Prasanna Kumar Kalever
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 <prasanna.kalever@redhat.com>
---
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;
@@ -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);
+
+ 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 +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
--
2.7.4
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [Qemu-devel] [PATCH v4] block/gluster: add support to choose libgfapi logfile
2016-07-22 14:56 [Qemu-devel] [PATCH v4] block/gluster: add support to choose libgfapi logfile Prasanna Kumar Kalever
@ 2016-08-04 10:16 ` Markus Armbruster
0 siblings, 0 replies; 2+ messages in thread
From: Markus Armbruster @ 2016-08-04 10:16 UTC (permalink / raw)
To: Prasanna Kumar Kalever; +Cc: qemu-devel, jcody
Prasanna Kumar Kalever <prasanna.kalever@redhat.com> 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 <prasanna.kalever@redhat.com>
> ---
> 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 <armbru@redhat.com>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2016-08-04 10:16 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-07-22 14:56 [Qemu-devel] [PATCH v4] block/gluster: add support to choose libgfapi logfile Prasanna Kumar Kalever
2016-08-04 10:16 ` Markus Armbruster
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).