* [Qemu-devel] [PULL for-2.8 1/3] block/gluster: fix QMP to match debug option
2016-12-05 21:31 [Qemu-devel] [PULL for-2.8 0/3] Block patches for -rc3 Jeff Cody
@ 2016-12-05 21:31 ` Jeff Cody
2016-12-05 21:32 ` [Qemu-devel] [PULL for-2.8 2/3] block/nfs: " Jeff Cody
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Jeff Cody @ 2016-12-05 21:31 UTC (permalink / raw)
To: qemu-block; +Cc: peter.maydell, jcody, qemu-devel, stefanha
From: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
The QMP definition of BlockdevOptionsGluster:
{ 'struct': 'BlockdevOptionsGluster',
'data': { 'volume': 'str',
'path': 'str',
'server': ['GlusterServer'],
'*debug-level': 'int',
'*logfile': 'str' } }
But instead of 'debug-level we have exported 'debug' as the option for choosing
debug level of gluster protocol driver.
This patch fix QMP definition BlockdevOptionsGluster
s/debug-level/debug/
Suggested-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
block/gluster.c | 38 +++++++++++++++++++-------------------
qapi/block-core.json | 4 ++--
2 files changed, 21 insertions(+), 21 deletions(-)
diff --git a/block/gluster.c b/block/gluster.c
index 891c13b..a0a74e4 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -48,7 +48,7 @@ typedef struct BDRVGlusterState {
struct glfs_fd *fd;
char *logfile;
bool supports_seek_data;
- int debug_level;
+ int debug;
} BDRVGlusterState;
typedef struct BDRVGlusterReopenState {
@@ -434,7 +434,7 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
}
}
- ret = glfs_set_logging(glfs, gconf->logfile, gconf->debug_level);
+ ret = glfs_set_logging(glfs, gconf->logfile, gconf->debug);
if (ret < 0) {
goto out;
}
@@ -788,17 +788,17 @@ static int qemu_gluster_open(BlockDriverState *bs, QDict *options,
filename = qemu_opt_get(opts, GLUSTER_OPT_FILENAME);
- s->debug_level = qemu_opt_get_number(opts, GLUSTER_OPT_DEBUG,
- GLUSTER_DEBUG_DEFAULT);
- if (s->debug_level < 0) {
- s->debug_level = 0;
- } else if (s->debug_level > GLUSTER_DEBUG_MAX) {
- s->debug_level = GLUSTER_DEBUG_MAX;
+ s->debug = qemu_opt_get_number(opts, GLUSTER_OPT_DEBUG,
+ GLUSTER_DEBUG_DEFAULT);
+ if (s->debug < 0) {
+ s->debug = 0;
+ } else if (s->debug > GLUSTER_DEBUG_MAX) {
+ s->debug = GLUSTER_DEBUG_MAX;
}
gconf = g_new0(BlockdevOptionsGluster, 1);
- gconf->debug_level = s->debug_level;
- gconf->has_debug_level = true;
+ gconf->debug = s->debug;
+ gconf->has_debug = true;
logfile = qemu_opt_get(opts, GLUSTER_OPT_LOGFILE);
s->logfile = g_strdup(logfile ? logfile : GLUSTER_LOGFILE_DEFAULT);
@@ -874,8 +874,8 @@ static int qemu_gluster_reopen_prepare(BDRVReopenState *state,
qemu_gluster_parse_flags(state->flags, &open_flags);
gconf = g_new0(BlockdevOptionsGluster, 1);
- gconf->debug_level = s->debug_level;
- gconf->has_debug_level = true;
+ gconf->debug = s->debug;
+ gconf->has_debug = true;
gconf->logfile = g_strdup(s->logfile);
gconf->has_logfile = true;
reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, NULL, errp);
@@ -1011,14 +1011,14 @@ static int qemu_gluster_create(const char *filename,
char *tmp = NULL;
gconf = g_new0(BlockdevOptionsGluster, 1);
- gconf->debug_level = qemu_opt_get_number_del(opts, GLUSTER_OPT_DEBUG,
- GLUSTER_DEBUG_DEFAULT);
- if (gconf->debug_level < 0) {
- gconf->debug_level = 0;
- } else if (gconf->debug_level > GLUSTER_DEBUG_MAX) {
- gconf->debug_level = GLUSTER_DEBUG_MAX;
+ gconf->debug = qemu_opt_get_number_del(opts, GLUSTER_OPT_DEBUG,
+ GLUSTER_DEBUG_DEFAULT);
+ if (gconf->debug < 0) {
+ gconf->debug = 0;
+ } else if (gconf->debug > GLUSTER_DEBUG_MAX) {
+ gconf->debug = GLUSTER_DEBUG_MAX;
}
- gconf->has_debug_level = true;
+ gconf->has_debug = true;
gconf->logfile = qemu_opt_get_del(opts, GLUSTER_OPT_LOGFILE);
if (!gconf->logfile) {
diff --git a/qapi/block-core.json b/qapi/block-core.json
index c29bef7..03b19f1 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2195,7 +2195,7 @@
#
# @server: gluster servers description
#
-# @debug-level: #optional libgfapi log level (default '4' which is Error)
+# @debug: #optional libgfapi log level (default '4' which is Error)
#
# @logfile: #optional libgfapi log file (default /dev/stderr) (Since 2.8)
#
@@ -2205,7 +2205,7 @@
'data': { 'volume': 'str',
'path': 'str',
'server': ['GlusterServer'],
- '*debug-level': 'int',
+ '*debug': 'int',
'*logfile': 'str' } }
##
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* [Qemu-devel] [PULL for-2.8 2/3] block/nfs: fix QMP to match debug option
2016-12-05 21:31 [Qemu-devel] [PULL for-2.8 0/3] Block patches for -rc3 Jeff Cody
2016-12-05 21:31 ` [Qemu-devel] [PULL for-2.8 1/3] block/gluster: fix QMP to match debug option Jeff Cody
@ 2016-12-05 21:32 ` Jeff Cody
2016-12-05 21:32 ` [Qemu-devel] [PULL for-2.8 3/3] qemu-doc: update gluster protocol usage guide Jeff Cody
2016-12-06 10:04 ` [Qemu-devel] [Qemu-block] [PULL for-2.8 0/3] Block patches for -rc3 Stefan Hajnoczi
3 siblings, 0 replies; 9+ messages in thread
From: Jeff Cody @ 2016-12-05 21:32 UTC (permalink / raw)
To: qemu-block; +Cc: peter.maydell, jcody, qemu-devel, stefanha
From: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
The QMP definition of BlockdevOptionsNfs:
{ 'struct': 'BlockdevOptionsNfs',
'data': { 'server': 'NFSServer',
'path': 'str',
'*user': 'int',
'*group': 'int',
'*tcp-syn-count': 'int',
'*readahead-size': 'int',
'*page-cache-size': 'int',
'*debug-level': 'int' } }
To make this consistent with other block protocols like gluster, lets
change s/debug-level/debug/
Suggested-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
block/nfs.c | 4 ++--
qapi/block-core.json | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/block/nfs.c b/block/nfs.c
index d082783..a490660 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -134,7 +134,7 @@ static int nfs_parse_uri(const char *filename, QDict *options, Error **errp)
qdict_put(options, "page-cache-size",
qstring_from_str(qp->p[i].value));
} else if (!strcmp(qp->p[i].name, "debug")) {
- qdict_put(options, "debug-level",
+ qdict_put(options, "debug",
qstring_from_str(qp->p[i].value));
} else {
error_setg(errp, "Unknown NFS parameter name: %s",
@@ -165,7 +165,7 @@ static bool nfs_has_filename_options_conflict(QDict *options, Error **errp)
!strcmp(qe->key, "tcp-syn-count") ||
!strcmp(qe->key, "readahead-size") ||
!strcmp(qe->key, "page-cache-size") ||
- !strcmp(qe->key, "debug-level") ||
+ !strcmp(qe->key, "debug") ||
strstart(qe->key, "server.", NULL))
{
error_setg(errp, "Option %s cannot be used with a filename",
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 03b19f1..f22ed2a 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2292,7 +2292,7 @@
# @page-cache-size: #optional set the pagecache size in bytes (defaults
# to libnfs default)
#
-# @debug-level: #optional set the NFS debug level (max 2) (defaults
+# @debug: #optional set the NFS debug level (max 2) (defaults
# to libnfs default)
#
# Since 2.8
@@ -2305,7 +2305,7 @@
'*tcp-syn-count': 'int',
'*readahead-size': 'int',
'*page-cache-size': 'int',
- '*debug-level': 'int' } }
+ '*debug': 'int' } }
##
# @BlockdevOptionsCurl
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* [Qemu-devel] [PULL for-2.8 3/3] qemu-doc: update gluster protocol usage guide
2016-12-05 21:31 [Qemu-devel] [PULL for-2.8 0/3] Block patches for -rc3 Jeff Cody
2016-12-05 21:31 ` [Qemu-devel] [PULL for-2.8 1/3] block/gluster: fix QMP to match debug option Jeff Cody
2016-12-05 21:32 ` [Qemu-devel] [PULL for-2.8 2/3] block/nfs: " Jeff Cody
@ 2016-12-05 21:32 ` Jeff Cody
2016-12-06 10:04 ` [Qemu-devel] [Qemu-block] [PULL for-2.8 0/3] Block patches for -rc3 Stefan Hajnoczi
3 siblings, 0 replies; 9+ messages in thread
From: Jeff Cody @ 2016-12-05 21:32 UTC (permalink / raw)
To: qemu-block; +Cc: peter.maydell, jcody, qemu-devel, stefanha
From: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
Document:
1. The new debug and logfile options with their usages
2. New json format and its usage and
3. update "GlusterFS, Device URL Syntax" section in "Invocation"
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
qemu-doc.texi | 59 +++++++++++++++++++++++++++++++++++++++++++--------------
qemu-options.hx | 25 ++++++++++++++++++++++--
2 files changed, 68 insertions(+), 16 deletions(-)
diff --git a/qemu-doc.texi b/qemu-doc.texi
index 023c140..02cb39d 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -1041,35 +1041,55 @@ GlusterFS is an user space distributed file system.
You can boot from the GlusterFS disk image with the command:
@example
-qemu-system-x86_64 -drive file=gluster[+@var{transport}]://[@var{server}[:@var{port}]]/@var{volname}/@var{image}[?socket=...]
+URI:
+qemu-system-x86_64 -drive file=gluster[+@var{type}]://[@var{host}[:@var{port}]]/@var{volume}/@var{path}
+ [?socket=...][,file.debug=9][,file.logfile=...]
+
+JSON:
+qemu-system-x86_64 'json:@{"driver":"qcow2",
+ "file":@{"driver":"gluster",
+ "volume":"testvol","path":"a.img","debug":9,"logfile":"...",
+ "server":[@{"type":"tcp","host":"...","port":"..."@},
+ @{"type":"unix","socket":"..."@}]@}@}'
@end example
@var{gluster} is the protocol.
-@var{transport} specifies the transport type used to connect to gluster
+@var{type} specifies the transport type used to connect to gluster
management daemon (glusterd). Valid transport types are
-tcp, unix and rdma. If a transport type isn't specified, then tcp
-type is assumed.
+tcp and unix. In the URI form, if a transport type isn't specified,
+then tcp type is assumed.
-@var{server} specifies the server where the volume file specification for
-the given volume resides. This can be either hostname, ipv4 address
-or ipv6 address. ipv6 address needs to be within square brackets [ ].
-If transport type is unix, then @var{server} field should not be specified.
+@var{host} specifies the server where the volume file specification for
+the given volume resides. This can be either a hostname or an ipv4 address.
+If transport type is unix, then @var{host} field should not be specified.
Instead @var{socket} field needs to be populated with the path to unix domain
socket.
@var{port} is the port number on which glusterd is listening. This is optional
-and if not specified, QEMU will send 0 which will make gluster to use the
-default port. If the transport type is unix, then @var{port} should not be
-specified.
+and if not specified, it defaults to port 24007. If the transport type is unix,
+then @var{port} should not be specified.
+
+@var{volume} is the name of the gluster volume which contains the disk image.
+
+@var{path} is the path to the actual disk image that resides on gluster volume.
+
+@var{debug} is the logging level of the gluster protocol driver. Debug levels
+are 0-9, with 9 being the most verbose, and 0 representing no debugging output.
+The default level is 4. The current logging levels defined in the gluster source
+are 0 - None, 1 - Emergency, 2 - Alert, 3 - Critical, 4 - Error, 5 - Warning,
+6 - Notice, 7 - Info, 8 - Debug, 9 - Trace
+
+@var{logfile} is a commandline option to mention log file path which helps in
+logging to the specified file and also help in persisting the gfapi logs. The
+default is stderr.
+
-@var{volname} is the name of the gluster volume which contains the disk image.
-@var{image} is the path to the actual disk image that resides on gluster volume.
You can create a GlusterFS disk image with the command:
@example
-qemu-img create gluster://@var{server}/@var{volname}/@var{image} @var{size}
+qemu-img create gluster://@var{host}/@var{volume}/@var{path} @var{size}
@end example
Examples
@@ -1082,6 +1102,17 @@ qemu-system-x86_64 -drive file=gluster+tcp://[1:2:3:4:5:6:7:8]:24007/testvol/dir
qemu-system-x86_64 -drive file=gluster+tcp://server.domain.com:24007/testvol/dir/a.img
qemu-system-x86_64 -drive file=gluster+unix:///testvol/dir/a.img?socket=/tmp/glusterd.socket
qemu-system-x86_64 -drive file=gluster+rdma://1.2.3.4:24007/testvol/a.img
+qemu-system-x86_64 -drive file=gluster://1.2.3.4/testvol/a.img,file.debug=9,file.logfile=/var/log/qemu-gluster.log
+qemu-system-x86_64 'json:@{"driver":"qcow2",
+ "file":@{"driver":"gluster",
+ "volume":"testvol","path":"a.img",
+ "debug":9,"logfile":"/var/log/qemu-gluster.log",
+ "server":[@{"type":"tcp","host":"1.2.3.4","port":24007@},
+ @{"type":"unix","socket":"/var/run/glusterd.socket"@}]@}@}'
+qemu-system-x86_64 -drive driver=qcow2,file.driver=gluster,file.volume=testvol,file.path=/path/a.img,
+ file.debug=9,file.logfile=/var/log/qemu-gluster.log,
+ file.server.0.type=tcp,file.server.0.host=1.2.3.4,file.server.0.port=24007,
+ file.server.1.type=unix,file.server.1.socket=/var/run/glusterd.socket
@end example
@node disk_images_ssh
diff --git a/qemu-options.hx b/qemu-options.hx
index 4a5b29f..c534a2f 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2595,13 +2595,34 @@ TCP, Unix Domain Sockets and RDMA transport protocols.
Syntax for specifying a VM disk image on GlusterFS volume is
@example
-gluster[+transport]://[server[:port]]/volname/image[?socket=...]
+
+URI:
+gluster[+type]://[host[:port]]/volume/path[?socket=...][,debug=N][,logfile=...]
+
+JSON:
+'json:@{"driver":"qcow2","file":@{"driver":"gluster","volume":"testvol","path":"a.img","debug":N,"logfile":"...",
+@ "server":[@{"type":"tcp","host":"...","port":"..."@},
+@ @{"type":"unix","socket":"..."@}]@}@}'
@end example
Example
@example
-qemu-system-x86_64 --drive file=gluster://192.0.2.1/testvol/a.img
+URI:
+qemu-system-x86_64 --drive file=gluster://192.0.2.1/testvol/a.img,
+@ file.debug=9,file.logfile=/var/log/qemu-gluster.log
+
+JSON:
+qemu-system-x86_64 'json:@{"driver":"qcow2",
+@ "file":@{"driver":"gluster",
+@ "volume":"testvol","path":"a.img",
+@ "debug":9,"logfile":"/var/log/qemu-gluster.log",
+@ "server":[@{"type":"tcp","host":"1.2.3.4","port":24007@},
+@ @{"type":"unix","socket":"/var/run/glusterd.socket"@}]@}@}'
+qemu-system-x86_64 -drive driver=qcow2,file.driver=gluster,file.volume=testvol,file.path=/path/a.img,
+@ file.debug=9,file.logfile=/var/log/qemu-gluster.log,
+@ file.server.0.type=tcp,file.server.0.host=1.2.3.4,file.server.0.port=24007,
+@ file.server.1.type=unix,file.server.1.socket=/var/run/glusterd.socket
@end example
See also @url{http://www.gluster.org}.
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [Qemu-block] [PULL for-2.8 0/3] Block patches for -rc3
2016-12-05 21:31 [Qemu-devel] [PULL for-2.8 0/3] Block patches for -rc3 Jeff Cody
` (2 preceding siblings ...)
2016-12-05 21:32 ` [Qemu-devel] [PULL for-2.8 3/3] qemu-doc: update gluster protocol usage guide Jeff Cody
@ 2016-12-06 10:04 ` Stefan Hajnoczi
2016-12-06 15:11 ` Eric Blake
3 siblings, 1 reply; 9+ messages in thread
From: Stefan Hajnoczi @ 2016-12-06 10:04 UTC (permalink / raw)
To: Jeff Cody
Cc: qemu-block, peter.maydell, qemu-devel, stefanha,
Prasanna Kumar Kalever
[-- Attachment #1: Type: text/plain, Size: 1883 bytes --]
On Mon, Dec 05, 2016 at 04:31:58PM -0500, Jeff Cody wrote:
> The following changes since commit bc66cedb4141fb7588f2462c74310d8fb5dd4cf1:
>
> Merge remote-tracking branch 'yongbok/tags/mips-20161204' into staging (2016-12-05 10:56:45 +0000)
>
> are available in the git repository at:
>
> https://github.com/codyprime/qemu-kvm-jtc.git tags/block-pull-request
>
> for you to fetch changes up to 76b5550f709b975a7b04fb4c887f300b7bb731c2:
>
> qemu-doc: update gluster protocol usage guide (2016-12-05 16:30:29 -0500)
>
> ----------------------------------------------------------------
> Gluster block patches for -rc3
> ----------------------------------------------------------------
>
> Prasanna Kumar Kalever (3):
> block/gluster: fix QMP to match debug option
> block/nfs: fix QMP to match debug option
> qemu-doc: update gluster protocol usage guide
>
> block/gluster.c | 38 ++++++++++++++++-----------------
> block/nfs.c | 4 ++--
> qapi/block-core.json | 8 +++----
> qemu-doc.texi | 59 +++++++++++++++++++++++++++++++++++++++-------------
> qemu-options.hx | 25 ++++++++++++++++++++--
> 5 files changed, 93 insertions(+), 41 deletions(-)
BlockdevOptionsGluster.debug(-level) does not have "Added in 2.8" so I
had to dig through git-blame(1) to verify that it was indeed added in
the current release cycle.
In the future please make sure all QAPI changes are marked by version.
If there tricky changes you can include a statement showing you are
aware of QAPI backwards compatibility ("These new options were added in
the 2.8 release cycle and can therefore still be changed without
breaking backward compatibility"). This will make me confident that
you've checked the QAPI changes.
Thanks, applied to my staging tree:
https://github.com/stefanha/qemu/commits/staging
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [Qemu-block] [PULL for-2.8 0/3] Block patches for -rc3
2016-12-06 10:04 ` [Qemu-devel] [Qemu-block] [PULL for-2.8 0/3] Block patches for -rc3 Stefan Hajnoczi
@ 2016-12-06 15:11 ` Eric Blake
2016-12-06 15:43 ` Eric Blake
0 siblings, 1 reply; 9+ messages in thread
From: Eric Blake @ 2016-12-06 15:11 UTC (permalink / raw)
To: Stefan Hajnoczi, Jeff Cody
Cc: peter.maydell, Prasanna Kumar Kalever, stefanha, qemu-devel,
qemu-block
[-- Attachment #1: Type: text/plain, Size: 2027 bytes --]
On 12/06/2016 04:04 AM, Stefan Hajnoczi wrote:
>>
>> Prasanna Kumar Kalever (3):
>> block/gluster: fix QMP to match debug option
>> block/nfs: fix QMP to match debug option
>> qemu-doc: update gluster protocol usage guide
>>
>> block/gluster.c | 38 ++++++++++++++++-----------------
>> block/nfs.c | 4 ++--
>> qapi/block-core.json | 8 +++----
>> qemu-doc.texi | 59 +++++++++++++++++++++++++++++++++++++++-------------
>> qemu-options.hx | 25 ++++++++++++++++++++--
>> 5 files changed, 93 insertions(+), 41 deletions(-)
>
> BlockdevOptionsGluster.debug(-level) does not have "Added in 2.8" so I
> had to dig through git-blame(1) to verify that it was indeed added in
> the current release cycle.
Then that implies we should add yet one more patch that adds the
appropriate versioning information to all the gluster fields added for
2.8. My reviewed-by was given on the assumption that debug was in 2.7
and that this was a break from 2.7 behavior, but that we already KNOW
we're breaking blockdev-add between 2.7 and 2.8; while your argument is
that there is no backwards incompatibility because it was not in 2.7 to
begin with. I think both reasons are indeed acceptable, but it also
means that my reason was flawed because of the incomplete documentation.
>
> In the future please make sure all QAPI changes are marked by version.
Indeed, and I try to flag it in my reviews as often as I notice it.
> If there tricky changes you can include a statement showing you are
> aware of QAPI backwards compatibility ("These new options were added in
> the 2.8 release cycle and can therefore still be changed without
> breaking backward compatibility"). This will make me confident that
> you've checked the QAPI changes.
>
> Thanks, applied to my staging tree:
> https://github.com/stefanha/qemu/commits/staging
>
> Stefan
>
--
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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [Qemu-block] [PULL for-2.8 0/3] Block patches for -rc3
2016-12-06 15:11 ` Eric Blake
@ 2016-12-06 15:43 ` Eric Blake
2016-12-07 10:08 ` Kevin Wolf
0 siblings, 1 reply; 9+ messages in thread
From: Eric Blake @ 2016-12-06 15:43 UTC (permalink / raw)
To: Stefan Hajnoczi, Jeff Cody
Cc: peter.maydell, qemu-block, Prasanna Kumar Kalever, qemu-devel,
stefanha
[-- Attachment #1: Type: text/plain, Size: 2848 bytes --]
On 12/06/2016 09:11 AM, Eric Blake wrote:
>> BlockdevOptionsGluster.debug(-level) does not have "Added in 2.8" so I
>> had to dig through git-blame(1) to verify that it was indeed added in
>> the current release cycle.
>
> Then that implies we should add yet one more patch that adds the
> appropriate versioning information to all the gluster fields added for
> 2.8. My reviewed-by was given on the assumption that debug was in 2.7
> and that this was a break from 2.7 behavior, but that we already KNOW
> we're breaking blockdev-add between 2.7 and 2.8; while your argument is
> that there is no backwards incompatibility because it was not in 2.7 to
> begin with. I think both reasons are indeed acceptable, but it also
> means that my reason was flawed because of the incomplete documentation.
I checked again. 'git diff v2.7.0..origin qapi/*.json' shows:
##
-# @BlockdevOptionsGluster
+# @BlockdevOptionsGluster:
#
# Driver specific block device options for Gluster
#
@@ -2140,7 +2195,9 @@
#
# @server: gluster servers description
#
-# @debug-level: #optional libgfapi log level (default '4' which is Error)
+# @debug: #optional libgfapi log level (default '4' which is Error)
+#
+# @logfile: #optional libgfapi log file (default /dev/stderr)
(Since 2.8)
#
# Since: 2.7
##
@@ -2148,25 +2205,163 @@
'data': { 'volume': 'str',
'path': 'str',
'server': ['GlusterServer'],
- '*debug-level': 'int' } }
+ '*debug': 'int',
+ '*logfile': 'str' } }
So I was right after all - this IS a backwards-incompatible change (and
NOT something that was introduced only in 2.8) - but I still argue that
the change is appropriate NOW (but not later) because blockdev-add is
the only client of this type in QMP, and that command changed
backwards-incompatibly at the same time.
>
>>
>> In the future please make sure all QAPI changes are marked by version.
>
> Indeed, and I try to flag it in my reviews as often as I notice it.
>
>> If there tricky changes you can include a statement showing you are
>> aware of QAPI backwards compatibility ("These new options were added in
>> the 2.8 release cycle and can therefore still be changed without
>> breaking backward compatibility"). This will make me confident that
>> you've checked the QAPI changes.
>>
>> Thanks, applied to my staging tree:
>> https://github.com/stefanha/qemu/commits/staging
In my audit of all differences between 2.7 and current state of qapi
.json files, I only found one addition that was lacking versioning
information:
event DEVICE_TRAY_MOVED gained 'id' parameter
I'll submit a patch for that shortly.
--
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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [Qemu-block] [PULL for-2.8 0/3] Block patches for -rc3
2016-12-06 15:43 ` Eric Blake
@ 2016-12-07 10:08 ` Kevin Wolf
2016-12-07 15:45 ` Eric Blake
0 siblings, 1 reply; 9+ messages in thread
From: Kevin Wolf @ 2016-12-07 10:08 UTC (permalink / raw)
To: Eric Blake
Cc: Stefan Hajnoczi, Jeff Cody, stefanha, peter.maydell,
Prasanna Kumar Kalever, qemu-devel, qemu-block
[-- Attachment #1: Type: text/plain, Size: 2308 bytes --]
Am 06.12.2016 um 16:43 hat Eric Blake geschrieben:
> On 12/06/2016 09:11 AM, Eric Blake wrote:
>
> >> BlockdevOptionsGluster.debug(-level) does not have "Added in 2.8" so I
> >> had to dig through git-blame(1) to verify that it was indeed added in
> >> the current release cycle.
> >
> > Then that implies we should add yet one more patch that adds the
> > appropriate versioning information to all the gluster fields added for
> > 2.8. My reviewed-by was given on the assumption that debug was in 2.7
> > and that this was a break from 2.7 behavior, but that we already KNOW
> > we're breaking blockdev-add between 2.7 and 2.8; while your argument is
> > that there is no backwards incompatibility because it was not in 2.7 to
> > begin with. I think both reasons are indeed acceptable, but it also
> > means that my reason was flawed because of the incomplete documentation.
>
> I checked again. 'git diff v2.7.0..origin qapi/*.json' shows:
>
> ##
> -# @BlockdevOptionsGluster
> +# @BlockdevOptionsGluster:
> #
> # Driver specific block device options for Gluster
> #
> @@ -2140,7 +2195,9 @@
> #
> # @server: gluster servers description
> #
> -# @debug-level: #optional libgfapi log level (default '4' which is Error)
> +# @debug: #optional libgfapi log level (default '4' which is Error)
> +#
> +# @logfile: #optional libgfapi log file (default /dev/stderr)
> (Since 2.8)
> #
> # Since: 2.7
> ##
> @@ -2148,25 +2205,163 @@
> 'data': { 'volume': 'str',
> 'path': 'str',
> 'server': ['GlusterServer'],
> - '*debug-level': 'int' } }
> + '*debug': 'int',
> + '*logfile': 'str' } }
>
> So I was right after all - this IS a backwards-incompatible change (and
> NOT something that was introduced only in 2.8) - but I still argue that
> the change is appropriate NOW (but not later) because blockdev-add is
> the only client of this type in QMP, and that command changed
> backwards-incompatibly at the same time.
I don't completely understand the change anyway. 'debug-level' is the
better name, and if the command line doesn't agree, change that one or
grudgingly accept both. But I think we can just change the command line,
it's only a debug option.
Kevin
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [Qemu-block] [PULL for-2.8 0/3] Block patches for -rc3
2016-12-07 10:08 ` Kevin Wolf
@ 2016-12-07 15:45 ` Eric Blake
0 siblings, 0 replies; 9+ messages in thread
From: Eric Blake @ 2016-12-07 15:45 UTC (permalink / raw)
To: Kevin Wolf
Cc: Stefan Hajnoczi, Jeff Cody, stefanha, peter.maydell,
Prasanna Kumar Kalever, qemu-devel, qemu-block
[-- Attachment #1: Type: text/plain, Size: 1632 bytes --]
On 12/07/2016 04:08 AM, Kevin Wolf wrote:
>> # Since: 2.7
>> ##
>> @@ -2148,25 +2205,163 @@
>> 'data': { 'volume': 'str',
>> 'path': 'str',
>> 'server': ['GlusterServer'],
>> - '*debug-level': 'int' } }
>> + '*debug': 'int',
>> + '*logfile': 'str' } }
>>
>> So I was right after all - this IS a backwards-incompatible change (and
>> NOT something that was introduced only in 2.8) - but I still argue that
>> the change is appropriate NOW (but not later) because blockdev-add is
>> the only client of this type in QMP, and that command changed
>> backwards-incompatibly at the same time.
>
> I don't completely understand the change anyway. 'debug-level' is the
> better name,
Perhaps so, but 2.7 already used 'debug' on the command line, and
released libvirt already targets the command line. Changing the command
line to accept 'debug-level' in addition to 'debug' is the only way to
not break existing libvirt usage. What's more, we have several
instances of 'debug' in other spots of blockdev-add; either they should
all be consistently named 'debug-level', or none of them; we made the
switch to none of them, as it was easier.
> and if the command line doesn't agree, change that one or
> grudgingly accept both. But I think we can just change the command line,
> it's only a debug option.
It may only be a debug option, but for good or bad, libvirt is already
targetting it by its current spelling of 'debug'.
--
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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread