[Qemu-devel] Re: [RFC][PATCH v1 05/12] qapi: fix handling for null-return async callbacks
On 03/28/2011 11:47 AM, Luiz Capitulino wrote:
On Fri, 25 Mar 2011 16:22:16 -0500
Anthony Liguorialigu...@us.ibm.com wrote:
On 03/25/2011 02:47 PM, Michael Roth wrote:
Async commands like 'guest-ping' have NULL retvals. Handle these by
inserting an empty dictionary in the response's return field.
Signed-off-by: Michael Rothmdr...@linux.vnet.ibm.com
---
qmp-core.c |5 -
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/qmp-core.c b/qmp-core.c
index e33f7a4..9f3d182 100644
--- a/qmp-core.c
+++ b/qmp-core.c
@@ -922,9 +922,12 @@ void qmp_async_complete_command(QmpCommandState *cmd,
QObject *retval, Error *er
rsp = qdict_new();
if (err) {
qdict_put_obj(rsp, error, error_get_qobject(err));
-} else {
+} else if (retval) {
qobject_incref(retval);
qdict_put_obj(rsp, return, retval);
+} else {
+/* add empty return dict, this is the standard for NULL returns */
+qdict_put_obj(rsp, return, QOBJECT(qdict_new()));
Luiz, I know we decided to return empty dicts because it lets us extend
things better, but did we want to rule out the use of a 'null' return
value entirely?
For asynchronous commands you mean? No we didn't.
No, nothing to do with asynchronous commands. Just in general.
The question is, is it legal for a command to return 'null'. It's
certain valid JSON, but is it valid QMP?
Regards,
Anthony Liguori
*iirc*, what happens today is that no command using this api is truly async,
for two reasons. First, changing from sync to async can break clients (that
happened to query-balloon). Second, although I can't remember the exact
details, the api that exists in the tree today is limited.
But for a new thing, like QAPI, having different semantics for async commands
seems the right thing to be done (ie. delaying the response).
For a command like this, I can't imagine ever wanting to extend the
return value...
Regards,
Anthony Liguori
}
if (cmd-tag) {
qdict_put_obj(rsp, tag, cmd-tag);
[Qemu-devel] Re: [RFC][PATCH v1 05/12] qapi: fix handling for null-return async callbacks
On Mon, 28 Mar 2011 12:01:16 -0500
Anthony Liguori aligu...@us.ibm.com wrote:
On 03/28/2011 11:47 AM, Luiz Capitulino wrote:
On Fri, 25 Mar 2011 16:22:16 -0500
Anthony Liguorialigu...@us.ibm.com wrote:
On 03/25/2011 02:47 PM, Michael Roth wrote:
Async commands like 'guest-ping' have NULL retvals. Handle these by
inserting an empty dictionary in the response's return field.
Signed-off-by: Michael Rothmdr...@linux.vnet.ibm.com
---
qmp-core.c |5 -
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/qmp-core.c b/qmp-core.c
index e33f7a4..9f3d182 100644
--- a/qmp-core.c
+++ b/qmp-core.c
@@ -922,9 +922,12 @@ void qmp_async_complete_command(QmpCommandState
*cmd, QObject *retval, Error *er
rsp = qdict_new();
if (err) {
qdict_put_obj(rsp, error, error_get_qobject(err));
-} else {
+} else if (retval) {
qobject_incref(retval);
qdict_put_obj(rsp, return, retval);
+} else {
+/* add empty return dict, this is the standard for NULL
returns */
+qdict_put_obj(rsp, return, QOBJECT(qdict_new()));
Luiz, I know we decided to return empty dicts because it lets us extend
things better, but did we want to rule out the use of a 'null' return
value entirely?
For asynchronous commands you mean? No we didn't.
No, nothing to do with asynchronous commands. Just in general.
The question is, is it legal for a command to return 'null'. It's
certain valid JSON, but is it valid QMP?
No, it's not valid.
Regards,
Anthony Liguori
*iirc*, what happens today is that no command using this api is truly async,
for two reasons. First, changing from sync to async can break clients (that
happened to query-balloon). Second, although I can't remember the exact
details, the api that exists in the tree today is limited.
But for a new thing, like QAPI, having different semantics for async
commands
seems the right thing to be done (ie. delaying the response).
For a command like this, I can't imagine ever wanting to extend the
return value...
Regards,
Anthony Liguori
}
if (cmd-tag) {
qdict_put_obj(rsp, tag, cmd-tag);
Re: [Qemu-devel] Re: [RFC][PATCH v1 05/12] qapi: fix handling for null-return async callbacks
On 03/28/2011 12:06 PM, Luiz Capitulino wrote:
On Mon, 28 Mar 2011 12:01:16 -0500
Anthony Liguorialigu...@us.ibm.com wrote:
On 03/28/2011 11:47 AM, Luiz Capitulino wrote:
On Fri, 25 Mar 2011 16:22:16 -0500
Anthony Liguorialigu...@us.ibm.com wrote:
On 03/25/2011 02:47 PM, Michael Roth wrote:
Async commands like 'guest-ping' have NULL retvals. Handle these by
inserting an empty dictionary in the response's return field.
Signed-off-by: Michael Rothmdr...@linux.vnet.ibm.com
---
qmp-core.c |5 -
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/qmp-core.c b/qmp-core.c
index e33f7a4..9f3d182 100644
--- a/qmp-core.c
+++ b/qmp-core.c
@@ -922,9 +922,12 @@ void qmp_async_complete_command(QmpCommandState *cmd,
QObject *retval, Error *er
rsp = qdict_new();
if (err) {
qdict_put_obj(rsp, error, error_get_qobject(err));
-} else {
+} else if (retval) {
qobject_incref(retval);
qdict_put_obj(rsp, return, retval);
+} else {
+/* add empty return dict, this is the standard for NULL returns */
+qdict_put_obj(rsp, return, QOBJECT(qdict_new()));
Luiz, I know we decided to return empty dicts because it lets us extend
things better, but did we want to rule out the use of a 'null' return
value entirely?
For asynchronous commands you mean? No we didn't.
No, nothing to do with asynchronous commands. Just in general.
The question is, is it legal for a command to return 'null'. It's
certain valid JSON, but is it valid QMP?
No, it's not valid.
Do we have a reason for this?
Regards,
Anthony Liguori
Re: [Qemu-devel] Re: [RFC][PATCH v1 05/12] qapi: fix handling for null-return async callbacks
On Mon, 28 Mar 2011 12:19:53 -0500
Anthony Liguori anth...@codemonkey.ws wrote:
On 03/28/2011 12:06 PM, Luiz Capitulino wrote:
On Mon, 28 Mar 2011 12:01:16 -0500
Anthony Liguorialigu...@us.ibm.com wrote:
On 03/28/2011 11:47 AM, Luiz Capitulino wrote:
On Fri, 25 Mar 2011 16:22:16 -0500
Anthony Liguorialigu...@us.ibm.com wrote:
On 03/25/2011 02:47 PM, Michael Roth wrote:
Async commands like 'guest-ping' have NULL retvals. Handle these by
inserting an empty dictionary in the response's return field.
Signed-off-by: Michael Rothmdr...@linux.vnet.ibm.com
---
qmp-core.c |5 -
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/qmp-core.c b/qmp-core.c
index e33f7a4..9f3d182 100644
--- a/qmp-core.c
+++ b/qmp-core.c
@@ -922,9 +922,12 @@ void qmp_async_complete_command(QmpCommandState
*cmd, QObject *retval, Error *er
rsp = qdict_new();
if (err) {
qdict_put_obj(rsp, error, error_get_qobject(err));
-} else {
+} else if (retval) {
qobject_incref(retval);
qdict_put_obj(rsp, return, retval);
+} else {
+/* add empty return dict, this is the standard for NULL
returns */
+qdict_put_obj(rsp, return, QOBJECT(qdict_new()));
Luiz, I know we decided to return empty dicts because it lets us extend
things better, but did we want to rule out the use of a 'null' return
value entirely?
For asynchronous commands you mean? No we didn't.
No, nothing to do with asynchronous commands. Just in general.
The question is, is it legal for a command to return 'null'. It's
certain valid JSON, but is it valid QMP?
No, it's not valid.
Do we have a reason for this?
We had to make a choice. We chose the current 'return' response. Iirc, one of
my first suggestions was { 'return': 'null' } but you refused to have a 'null'
object, our parser doesn't even support it afaik.
But what's the problem with the current format?
Re: [Qemu-devel] Re: [RFC][PATCH v1 05/12] qapi: fix handling for null-return async callbacks
On 03/28/2011 12:27 PM, Luiz Capitulino wrote:
We had to make a choice. We chose the current 'return' response. Iirc, one of
my first suggestions was { 'return': 'null' }
It would be:
{ 'return': null }
That's the valid JSON version.
but you refused to have a 'null'
object, our parser doesn't even support it afaik.
It doesn't but that's because there isn't a QNone.
But what's the problem with the current format?
Nothing, I was mostly curious as I only vaguely remember this discussion
previously.
Regards,
Anthony Liguori
[Qemu-devel] Re: [RFC][PATCH v1 05/12] qapi: fix handling for null-return async callbacks
On Fri, 25 Mar 2011 16:22:16 -0500
Anthony Liguori aligu...@us.ibm.com wrote:
On 03/25/2011 02:47 PM, Michael Roth wrote:
Async commands like 'guest-ping' have NULL retvals. Handle these by
inserting an empty dictionary in the response's return field.
Signed-off-by: Michael Rothmdr...@linux.vnet.ibm.com
---
qmp-core.c |5 -
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/qmp-core.c b/qmp-core.c
index e33f7a4..9f3d182 100644
--- a/qmp-core.c
+++ b/qmp-core.c
@@ -922,9 +922,12 @@ void qmp_async_complete_command(QmpCommandState *cmd,
QObject *retval, Error *er
rsp = qdict_new();
if (err) {
qdict_put_obj(rsp, error, error_get_qobject(err));
-} else {
+} else if (retval) {
qobject_incref(retval);
qdict_put_obj(rsp, return, retval);
+} else {
+/* add empty return dict, this is the standard for NULL returns
*/
+qdict_put_obj(rsp, return, QOBJECT(qdict_new()));
Luiz, I know we decided to return empty dicts because it lets us extend
things better, but did we want to rule out the use of a 'null' return
value entirely?
For asynchronous commands you mean? No we didn't.
*iirc*, what happens today is that no command using this api is truly async,
for two reasons. First, changing from sync to async can break clients (that
happened to query-balloon). Second, although I can't remember the exact
details, the api that exists in the tree today is limited.
But for a new thing, like QAPI, having different semantics for async commands
seems the right thing to be done (ie. delaying the response).
For a command like this, I can't imagine ever wanting to extend the
return value...
Regards,
Anthony Liguori
}
if (cmd-tag) {
qdict_put_obj(rsp, tag, cmd-tag);
[Qemu-devel] Re: [RFC][PATCH v1 05/12] qapi: fix handling for null-return async callbacks
On 03/28/2011 11:47 AM, Luiz Capitulino wrote:
On Fri, 25 Mar 2011 16:22:16 -0500
Anthony Liguorialigu...@us.ibm.com wrote:
On 03/25/2011 02:47 PM, Michael Roth wrote:
Async commands like 'guest-ping' have NULL retvals. Handle these by
inserting an empty dictionary in the response's return field.
Signed-off-by: Michael Rothmdr...@linux.vnet.ibm.com
---
qmp-core.c |5 -
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/qmp-core.c b/qmp-core.c
index e33f7a4..9f3d182 100644
--- a/qmp-core.c
+++ b/qmp-core.c
@@ -922,9 +922,12 @@ void qmp_async_complete_command(QmpCommandState *cmd,
QObject *retval, Error *er
rsp = qdict_new();
if (err) {
qdict_put_obj(rsp, error, error_get_qobject(err));
-} else {
+} else if (retval) {
qobject_incref(retval);
qdict_put_obj(rsp, return, retval);
+} else {
+/* add empty return dict, this is the standard for NULL returns */
+qdict_put_obj(rsp, return, QOBJECT(qdict_new()));
Luiz, I know we decided to return empty dicts because it lets us extend
things better, but did we want to rule out the use of a 'null' return
value entirely?
For asynchronous commands you mean? No we didn't.
*iirc*, what happens today is that no command using this api is truly async,
for two reasons. First, changing from sync to async can break clients (that
happened to query-balloon). Second, although I can't remember the exact
details, the api that exists in the tree today is limited.
But for a new thing, like QAPI, having different semantics for async commands
seems the right thing to be done (ie. delaying the response).
For a command like this, I can't imagine ever wanting to extend the
return value...
I think this is another topic, but also one we should hash out a bit better.
Currently the plan is that the C API not expose asynchronicity,
underneath the covers the library will issue the request, then do a
blocking read for the response. So the API call will block till
completion, and no other command's will be serviced through the same
underlying session until it is completed or cancelled.
For the JSON-based clients, the behavior is different. You have an
optional tag you can pass in for an async command, and after issuing
one, you can immediately begin issuing other async or non-async
commands. As a result, the responses you receive will not necessarily be
in FIFO order.
The upside to this is you can implement async commands on the client
side without using separate threads, and can exploit some level of
concurrency being able to do work within a session while a slower
host-guest command completes. The downsides are that:
1) There is some inconsistency between this and the C API semantics.
2) The optional tags are basically required tags, at least for async
commands, unless the client side does something to force synchronicity.
One option would be to disable the QMP session's read handler once a
JSON object is received, and not re-enable it until we send the
response. This would enforce FIFO-ordering. It might also add reduce the
potential for a client being able to blow up our TX buffers by issuing
lots of requests and not handling the responses in a timely enough
manner (have seen this just from piping responses to stdout).
Thoughts?
Re: [Qemu-devel] Re: [RFC][PATCH v1 05/12] qapi: fix handling for null-return async callbacks
On 03/28/2011 12:59 PM, Michael Roth wrote: For a command like this, I can't imagine ever wanting to extend the return value... I think this is another topic, but also one we should hash out a bit better. Currently the plan is that the C API not expose asynchronicity, underneath the covers the library will issue the request, then do a blocking read for the response. So the API call will block till completion, and no other command's will be serviced through the same underlying session until it is completed or cancelled. No, that's just the patches as they stand today. The medium term goal is to have twin APIs--one API that is synchronous and another that is asynchronous. The asynchronous version will mirror how asynchronous commands are dispatched within QEMU. That is, for query-block, you'll have: typedef void (*QueryBlockFunc)(void *opaque, BlockInfo *retval, Error *err); void libqmp_async_query_block(QmpSession *sess, Error **errp, QueryBlockFunc *cb, void *opaque); The challenge with async library commands is that you need to think carefully about how you interact with the socket. You can make a glib mainloop be a prerequisite, or you can have a set of function pointers that let you implement your own main loop. But since there isn't a pressing need for this in the short term, it's easier to just delay this. For the JSON-based clients, the behavior is different. You have an optional tag you can pass in for an async command, and after issuing one, you can immediately begin issuing other async or non-async commands. As a result, the responses you receive will not necessarily be in FIFO order. There is no such thing as a JSON-based client. QMP is not self describing enough to implement this with a pure transport client. Another language implementation (which I think you mean) would still need to solve the same problem as libqmp. The upside to this is you can implement async commands on the client side without using separate threads, and can exploit some level of concurrency being able to do work within a session while a slower host-guest command completes. The downsides are that: 1) There is some inconsistency between this and the C API semantics. You still need to pass a continuation to implement an event-based interface regardless of the language you're using. The function pointer/opaque parameter is a continuation in C. 2) The optional tags are basically required tags, at least for async commands, unless the client side does something to force synchronicity. One option would be to disable the QMP session's read handler once a JSON object is received, and not re-enable it until we send the response. This would enforce FIFO-ordering. It might also add reduce the potential for a client being able to blow up our TX buffers by issuing lots of requests and not handling the responses in a timely enough manner (have seen this just from piping responses to stdout). No, we're mixing up wire semantics with client/server semantics. These are all completely different things. The wire semantics are: 1) All commands are tagged. Untagged commands have an implicit tag (let's refer to it as the psuedo-tag). 2) Until a command is completed, a tag cannot be reused. This is also true for the psuedo-tag. 3) There is no guarantee about command completion order. 4) If a client happens to use the same tag for all commands, the client ends up enforcing a completion order because the server is only ever processing one command at a time. The server semantics are: 1) All tags are preserved including the psuedo-tag. This is required by the protocol. 2) Most commands are implemented by immediately dispatching a function and then computing the return value and immediately putting on the socket buffer. 3) Some commands are implemented by delaying the computation of the return value. When this happens (which is an indeterminate amount of time later), the data will be put on the socket buffer. 4) Which commands are handled by (2) vs. (3) are transparent to the client. The (current) client semantics are: 1) All commands are tagged with the psuedo-tag which enforces that only one command can be in flight at a time. In the future, this interface will support threading and use different tags such that two threads can be used to send simultaneous commands. 2) A second interface will be implemented that provides an event-based interface whereas each command is passed a continuation. Commands will use different tags to support this interface. 3) The reason to have both interfaces is to support the two most common models of concurrency, event-based concurrency and thread based concurrency. Notice that I said nothing about 'C' in the above. It's equally true to a C or Python client. Regards, Anthony Liguori Thoughts?
Re: [Qemu-devel] Re: [RFC][PATCH v1 05/12] qapi: fix handling for null-return async callbacks
On 03/28/2011 01:27 PM, Anthony Liguori wrote: On 03/28/2011 12:59 PM, Michael Roth wrote: For a command like this, I can't imagine ever wanting to extend the return value... I think this is another topic, but also one we should hash out a bit better. Currently the plan is that the C API not expose asynchronicity, underneath the covers the library will issue the request, then do a blocking read for the response. So the API call will block till completion, and no other command's will be serviced through the same underlying session until it is completed or cancelled. No, that's just the patches as they stand today. The medium term goal is to have twin APIs--one API that is synchronous and another that is asynchronous. The asynchronous version will mirror how asynchronous commands are dispatched within QEMU. That is, for query-block, you'll have: typedef void (*QueryBlockFunc)(void *opaque, BlockInfo *retval, Error *err); void libqmp_async_query_block(QmpSession *sess, Error **errp, QueryBlockFunc *cb, void *opaque); The challenge with async library commands is that you need to think carefully about how you interact with the socket. You can make a glib mainloop be a prerequisite, or you can have a set of function pointers that let you implement your own main loop. But since there isn't a pressing need for this in the short term, it's easier to just delay this. For the JSON-based clients, the behavior is different. You have an optional tag you can pass in for an async command, and after issuing one, you can immediately begin issuing other async or non-async commands. As a result, the responses you receive will not necessarily be in FIFO order. There is no such thing as a JSON-based client. QMP is not self describing enough to implement this with a pure transport client. Another language implementation (which I think you mean) would still need to solve the same problem as libqmp. By JSON-based I mean interacting directly with the QMP server socket via, say, `socat unix-connect:/tmp/qmp.sock readline`. I think this is what you're describing as being the wire protocol. What I mean is that the wire protocol currently supports more than the libqmp C API does in terms of being able to handle multiple in-flight requests. My thought was simply that if the C API provided by libqmp (and other language bindings) would always be synchronous, then the wire protocol, and the server-side handling of it, could potentially be simplified by enforcing FIFO ordering and not needing to handle multiple in-flight requests. But if the long-term goal is to provide for asynchronous APIs then that probably doesn't make any sense. The upside to this is you can implement async commands on the client side without using separate threads, and can exploit some level of concurrency being able to do work within a session while a slower host-guest command completes. The downsides are that: 1) There is some inconsistency between this and the C API semantics. You still need to pass a continuation to implement an event-based interface regardless of the language you're using. The function pointer/opaque parameter is a continuation in C. 2) The optional tags are basically required tags, at least for async commands, unless the client side does something to force synchronicity. One option would be to disable the QMP session's read handler once a JSON object is received, and not re-enable it until we send the response. This would enforce FIFO-ordering. It might also add reduce the potential for a client being able to blow up our TX buffers by issuing lots of requests and not handling the responses in a timely enough manner (have seen this just from piping responses to stdout). No, we're mixing up wire semantics with client/server semantics. These are all completely different things. The wire semantics are: 1) All commands are tagged. Untagged commands have an implicit tag (let's refer to it as the psuedo-tag). 2) Until a command is completed, a tag cannot be reused. This is also true for the psuedo-tag. 3) There is no guarantee about command completion order. 4) If a client happens to use the same tag for all commands, the client ends up enforcing a completion order because the server is only ever processing one command at a time. Is this supposed to be the current behavior? In early testing I noticed that not including a tag, and issuing an async command that never completed, still allowed for me to get responses for subsequent, tagless/pseudo-tagged requests. The server semantics are: 1) All tags are preserved including the psuedo-tag. This is required by the protocol. 2) Most commands are implemented by immediately dispatching a function and then computing the return value and immediately putting on the socket buffer. 3) Some commands are implemented by delaying the computation of the return value. When this happens (which is an indeterminate amount of time later), the data will be
Re: [Qemu-devel] Re: [RFC][PATCH v1 05/12] qapi: fix handling for null-return async callbacks
On 03/28/2011 03:42 PM, Michael Roth wrote: Is this supposed to be the current behavior? In early testing I noticed that not including a tag, and issuing an async command that never completed, still allowed for me to get responses for subsequent, tagless/pseudo-tagged requests. I don't think the server enforces tag-reuse today as that's really something a client should be checking for. It certainly doesn't hurt for the server to complain if a client sends a tag that's already in use though. Regards, Anthony Liguori
[Qemu-devel] Re: [RFC][PATCH v1 05/12] qapi: fix handling for null-return async callbacks
On 03/25/2011 02:47 PM, Michael Roth wrote:
Async commands like 'guest-ping' have NULL retvals. Handle these by
inserting an empty dictionary in the response's return field.
Signed-off-by: Michael Rothmdr...@linux.vnet.ibm.com
---
qmp-core.c |5 -
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/qmp-core.c b/qmp-core.c
index e33f7a4..9f3d182 100644
--- a/qmp-core.c
+++ b/qmp-core.c
@@ -922,9 +922,12 @@ void qmp_async_complete_command(QmpCommandState *cmd,
QObject *retval, Error *er
rsp = qdict_new();
if (err) {
qdict_put_obj(rsp, error, error_get_qobject(err));
-} else {
+} else if (retval) {
qobject_incref(retval);
qdict_put_obj(rsp, return, retval);
+} else {
+/* add empty return dict, this is the standard for NULL returns */
+qdict_put_obj(rsp, return, QOBJECT(qdict_new()));
Luiz, I know we decided to return empty dicts because it lets us extend
things better, but did we want to rule out the use of a 'null' return
value entirely?
For a command like this, I can't imagine ever wanting to extend the
return value...
Regards,
Anthony Liguori
}
if (cmd-tag) {
qdict_put_obj(rsp, tag, cmd-tag);
