| Message ID | 1299460984-15849-20-git-send-email-aliguori@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 03/07/2011 03:23 AM, Anthony Liguori wrote: > This is needed for libqmp to support events. put-event is used to disconnect > from signals. > > Signed-off-by: Anthony Liguori<aliguori@us.ibm.com> > > diff --git a/qmp-schema.json b/qmp-schema.json > index 3f2dd4e..a13885f 100644 > --- a/qmp-schema.json > +++ b/qmp-schema.json > @@ -58,3 +58,18 @@ > # Since: 0.14.0 > ## > [ 'qmp_capabilities', {}, {}, 'none' ] > + > +## > +# @put_event: > +# > +# Disconnect a signal. This command is used to disconnect from a signal based > +# on the handle returned by a signal accessor. > +# > +# @tag: the handle returned by a signal accessor. > +# > +# Returns: Nothing on success. > +# If @tag is not a valid handle, InvalidParameterValue > +# > +# Since: 0.15.0 > +## > +[ 'put-event', {'tag': 'int'}, {}, 'none' ] Why is tag an int? don't we use strings for command ids and similar? Also could be better named, disconnect-event or unlisten-event.
On 03/09/2011 07:31 AM, Avi Kivity wrote: > On 03/07/2011 03:23 AM, Anthony Liguori wrote: >> This is needed for libqmp to support events. put-event is used to >> disconnect >> from signals. >> >> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com> >> >> diff --git a/qmp-schema.json b/qmp-schema.json >> index 3f2dd4e..a13885f 100644 >> --- a/qmp-schema.json >> +++ b/qmp-schema.json >> @@ -58,3 +58,18 @@ >> # Since: 0.14.0 >> ## >> [ 'qmp_capabilities', {}, {}, 'none' ] >> + >> +## >> +# @put_event: >> +# >> +# Disconnect a signal. This command is used to disconnect from a >> signal based >> +# on the handle returned by a signal accessor. >> +# >> +# @tag: the handle returned by a signal accessor. >> +# >> +# Returns: Nothing on success. >> +# If @tag is not a valid handle, InvalidParameterValue >> +# >> +# Since: 0.15.0 >> +## >> +[ 'put-event', {'tag': 'int'}, {}, 'none' ] > > Why is tag an int? It's a handle so the type doesn't matter as long as I can make sure values are unique. ints are easier to work with because they don't require memory allocation. > don't we use strings for command ids and similar? id's can be any valid JSON value. But a handle is not the same thing as an id. > Also could be better named, disconnect-event or unlisten-event. I was going for symmetry with the signal accessors which are typically in the format 'get-block-io-error-event'. Maybe it would be better to do 'connect-block-io-error-event' and 'disconnect-event'? Regards, Anthony Liguori
On 03/09/2011 03:48 PM, Anthony Liguori wrote: >>> +[ 'put-event', {'tag': 'int'}, {}, 'none' ] >> >> Why is tag an int? > +## > > It's a handle so the type doesn't matter as long as I can make sure > values are unique. ints are easier to work with because they don't > require memory allocation. I think it's nicer for the client to use a string. Instead of a global ID allocator, it can use unique IDs or unique prefixes + local IDs. Should also aid a little in debugging. > >> don't we use strings for command ids and similar? > > id's can be any valid JSON value. > > But a handle is not the same thing as an id. Why not? I hope handles are client-provided? > >> Also could be better named, disconnect-event or unlisten-event. > > I was going for symmetry with the signal accessors which are typically > in the format 'get-block-io-error-event'. > > Maybe it would be better to do 'connect-block-io-error-event' and > 'disconnect-event'? Yes. But I'm confused, do we have a per-event command on the wire? Or just C stubs?
On 03/09/2011 07:58 AM, Avi Kivity wrote: > On 03/09/2011 03:48 PM, Anthony Liguori wrote: >>>> +[ 'put-event', {'tag': 'int'}, {}, 'none' ] >>> >>> Why is tag an int? >> +## >> >> It's a handle so the type doesn't matter as long as I can make sure >> values are unique. ints are easier to work with because they don't >> require memory allocation. > > I think it's nicer for the client to use a string. Instead of a > global ID allocator, it can use unique IDs or unique prefixes + local > IDs. Should also aid a little in debugging. handle's are opaque to clients. I don't have a huge objection to using strings and it may make sense to do a prefix like you're suggesting. I won't do that initially but since handles are opaque, we can make that change later. >>> don't we use strings for command ids and similar? >> >> id's can be any valid JSON value. >> >> But a handle is not the same thing as an id. > > Why not? > > I hope handles are client-provided? No, they are generated by the server. It makes sense because really a handle is a marshalled version of a signal. The signal accessor looks something like this: { 'BLOCK_IO_ERROR': { 'device': 'str', 'action': 'str', 'operation': 'str' } } [ 'get-block-io-error-event': {}, 'BLOCK_IO_ERROR' } The way we marshal a 'BLOCK_IO_ERROR' type is by generating a unique handle and returning that. While this looks like an int on the wire, at both the server and libqmp level, it looks like a BlockIoErrorEvent object. So in QEMU: BlockIoErrorEvent *qmp_get_block_io_error_event(Error **errp) { } And in libqmp: BlockIoErrorEvent *libqmp_get_block_io_error_event(QmpSession *sess, Error **errp) { } >>> Also could be better named, disconnect-event or unlisten-event. >> >> I was going for symmetry with the signal accessors which are >> typically in the format 'get-block-io-error-event'. >> >> Maybe it would be better to do 'connect-block-io-error-event' and >> 'disconnect-event'? > > Yes. > > But I'm confused, do we have a per-event command on the wire? Or just > C stubs? Ignoring default events, you'll never see an event until you execute a signal accessor function. When you execute this function, you will start receiving the events and those events will carry a tag containing the handle returned by the signal accessor. Within libqmp, any time you execute a signal accessor, a new signal object is created of the appropriate type. When that object is destroyed, you send a put-event to stop receiving the signal. When you connect to a signal object (via libqmp), you don't execute the signal accessor because the object is already receiving the signal. Default events (which exist to preserve compatibility) are a set of events that are automatically connected to after qmp_capabilities is executed. Because these connections are implicit, they arrive without a handle in the event object. At this point, libqmp just ignores default events. In the future, I'd like to add a command that can be executed before qmp_capabilities that will avoid connecting to default events. Regards, Anthony Liguori
On 03/09/2011 04:26 PM, Anthony Liguori wrote: > On 03/09/2011 07:58 AM, Avi Kivity wrote: >> On 03/09/2011 03:48 PM, Anthony Liguori wrote: >>>>> +[ 'put-event', {'tag': 'int'}, {}, 'none' ] >>>> >>>> Why is tag an int? >>> +## >>> >>> It's a handle so the type doesn't matter as long as I can make sure >>> values are unique. ints are easier to work with because they don't >>> require memory allocation. >> >> I think it's nicer for the client to use a string. Instead of a >> global ID allocator, it can use unique IDs or unique prefixes + local >> IDs. Should also aid a little in debugging. > > handle's are opaque to clients. > > I don't have a huge objection to using strings and it may make sense > to do a prefix like you're suggesting. I won't do that initially but > since handles are opaque, we can make that change later. What I mean is that the client should specify the handle, like it does for everything else it gives a name (netdevs, blockdevs, SCM_RIGHT fds, etc). { execute: listen-event, arguments: { event: blah, id: blah00001 } } { execute: unlisten-event arguments: { id: blah00001 } } > >>>> don't we use strings for command ids and similar? >>> >>> id's can be any valid JSON value. >>> >>> But a handle is not the same thing as an id. >> >> Why not? >> >> I hope handles are client-provided? > > No, they are generated by the server. It makes sense because really a > handle is a marshalled version of a signal. The signal accessor looks > something like this: > > { 'BLOCK_IO_ERROR': { 'device': 'str', 'action': 'str', 'operation': > 'str' } } > [ 'get-block-io-error-event': {}, 'BLOCK_IO_ERROR' } > > The way we marshal a 'BLOCK_IO_ERROR' type is by generating a unique > handle and returning that. I don't follow at all. Where's the handle here? Why don't we return the BLOCK_IO_ERROR as an object, on the wire? > > While this looks like an int on the wire, at both the server and > libqmp level, it looks like a BlockIoErrorEvent object. So in QEMU: > > BlockIoErrorEvent *qmp_get_block_io_error_event(Error **errp) > { > } > > And in libqmp: > > BlockIoErrorEvent *libqmp_get_block_io_error_event(QmpSession *sess, > Error **errp) > { > } What would the wire exchange look like? > >>>> Also could be better named, disconnect-event or unlisten-event. >>> >>> I was going for symmetry with the signal accessors which are >>> typically in the format 'get-block-io-error-event'. >>> >>> Maybe it would be better to do 'connect-block-io-error-event' and >>> 'disconnect-event'? >> >> Yes. >> >> But I'm confused, do we have a per-event command on the wire? Or >> just C stubs? > > Ignoring default events, you'll never see an event until you execute a > signal accessor function. When you execute this function, you will > start receiving the events and those events will carry a tag > containing the handle returned by the signal accessor. A "signal accessor" is a command to start listening to a signal? So why not have the signal accessor provide the tag? Like execute: blah provides a tag? > > Within libqmp, any time you execute a signal accessor, a new signal > object is created of the appropriate type. When that object is > destroyed, you send a put-event to stop receiving the signal. > > When you connect to a signal object (via libqmp), you don't execute > the signal accessor because the object is already receiving the signal. > > Default events (which exist to preserve compatibility) are a set of > events that are automatically connected to after qmp_capabilities is > executed. Because these connections are implicit, they arrive without > a handle in the event object. > > At this point, libqmp just ignores default events. In the future, I'd > like to add a command that can be executed before qmp_capabilities > that will avoid connecting to default events. I'm really confused. Part of that is because the conversation mixes libqmp, server API, and wire protocol. I'd like to understand the wire protocol first, everything else follows from that.
On 03/10/2011 06:39 AM, Avi Kivity wrote: > What I mean is that the client should specify the handle, like it does > for everything else it gives a name (netdevs, blockdevs, SCM_RIGHT > fds, etc). > > { execute: listen-event, arguments: { event: blah, id: blah00001 } } > { execute: unlisten-event arguments: { id: blah00001 } } Yeah, I understand, it just doesn't fit the model quite as well of signal accessors. Normally, in a signal/slot event model, you'd have some notion of an object model and/or hierarchy. For instance, with dbus, you'd do something like: bus = dbus.SystemBus() hal = # magic to get a hal object device = hal.FindDeviceByCapability('media.storage') device.connect_to_signal('media-changed', fn) We don't have objects and I don't mean to introduce them, but I like the idea of treating signals as objects and returning them via an accessor function. So the idea is that the handle is a marshalled form of the signal object. >> { 'BLOCK_IO_ERROR': { 'device': 'str', 'action': 'str', 'operation': >> 'str' } } >> [ 'get-block-io-error-event': {}, 'BLOCK_IO_ERROR' } >> >> The way we marshal a 'BLOCK_IO_ERROR' type is by generating a unique >> handle and returning that. > > I don't follow at all. Where's the handle here? Why don't we return > the BLOCK_IO_ERROR as an object, on the wire? How we marshal an object depends on the RPC layer. We marshal events on the wire as an integer handle. That is only a concept within the wire protocol. We could just as easily return an object but without diving into JSON class hinting, it'd be pretty meaningless because we'd just return "{ 'handle': 32}" instead of "32". >> While this looks like an int on the wire, at both the server and >> libqmp level, it looks like a BlockIoErrorEvent object. So in QEMU: >> >> BlockIoErrorEvent *qmp_get_block_io_error_event(Error **errp) >> { >> } >> >> And in libqmp: >> >> BlockIoErrorEvent *libqmp_get_block_io_error_event(QmpSession *sess, >> Error **errp) >> { >> } > > What would the wire exchange look like? > { 'execute': 'get-block-io-error-event' } < { 'return' : 32 } ... < { 'event': 'BLOCK_IO_ERROR', 'data': { 'action': 'stop', 'device': 'ide0-hd0', 'operation': 'read' }, 'tag': 32 } ... > { 'execute': 'put-event', 'arguments': { 'tag': 32 } } >> Ignoring default events, you'll never see an event until you execute >> a signal accessor function. When you execute this function, you will >> start receiving the events and those events will carry a tag >> containing the handle returned by the signal accessor. > > A "signal accessor" is a command to start listening to a signal? Yes, it basically enables the signal for the session. > So why not have the signal accessor provide the tag? Like execute: > blah provides a tag? How would this map to a C API? You'd either have to completely drop the notion of signal objects and use a separate mechanism to register callbacks against a tag (and lose type safety) or do some major munging to have the C API take a radically different signature than the wire protocol. >> >> Within libqmp, any time you execute a signal accessor, a new signal >> object is created of the appropriate type. When that object is >> destroyed, you send a put-event to stop receiving the signal. >> >> When you connect to a signal object (via libqmp), you don't execute >> the signal accessor because the object is already receiving the signal. >> >> Default events (which exist to preserve compatibility) are a set of >> events that are automatically connected to after qmp_capabilities is >> executed. Because these connections are implicit, they arrive >> without a handle in the event object. >> >> At this point, libqmp just ignores default events. In the future, >> I'd like to add a command that can be executed before >> qmp_capabilities that will avoid connecting to default events. > > I'm really confused. Part of that is because the conversation mixes > libqmp, server API, and wire protocol. I'd like to understand the > wire protocol first, everything else follows from that. No, it's the opposite for me. We design a good C API and then figure out how to make it work well as a wire protocol. The whole point of this effort is to build an API that we can consume within QEMU such that we can start breaking large chunks of code out of the main executable. Regards, Anthony Liguori
On 03/10/2011 04:12 PM, Anthony Liguori wrote: > On 03/10/2011 06:39 AM, Avi Kivity wrote: >> What I mean is that the client should specify the handle, like it >> does for everything else it gives a name (netdevs, blockdevs, >> SCM_RIGHT fds, etc). >> >> { execute: listen-event, arguments: { event: blah, id: blah00001 } } >> { execute: unlisten-event arguments: { id: blah00001 } } > > Yeah, I understand, it just doesn't fit the model quite as well of > signal accessors. > > Normally, in a signal/slot event model, you'd have some notion of an > object model and/or hierarchy. For instance, with dbus, you'd do > something like: > > bus = dbus.SystemBus() > hal = # magic to get a hal object > device = hal.FindDeviceByCapability('media.storage') > > device.connect_to_signal('media-changed', fn) > > We don't have objects and I don't mean to introduce them, but I like > the idea of treating signals as objects and returning them via an > accessor function. > > So the idea is that the handle is a marshalled form of the signal object. It's not a marshalled form, it's an opaque handle. A marshalled form doesn't destroy information. Anyway it would work with a client-provided tag just as well. connect_to_signal() could manufacture one and provide it to the server. > >>> { 'BLOCK_IO_ERROR': { 'device': 'str', 'action': 'str', 'operation': >>> 'str' } } >>> [ 'get-block-io-error-event': {}, 'BLOCK_IO_ERROR' } >>> >>> The way we marshal a 'BLOCK_IO_ERROR' type is by generating a unique >>> handle and returning that. >> >> I don't follow at all. Where's the handle here? Why don't we return >> the BLOCK_IO_ERROR as an object, on the wire? > > How we marshal an object depends on the RPC layer. > > We marshal events on the wire as an integer handle. That is only a > concept within the wire protocol. I don't think it's an accurate description. We marshall an event as a json object according to the schema above (BLOCK_IO_ERROR). How we marshall a subscription to an event, or an unsubscription to an event, depends on how we specify the protocol. It has nothing to do with client or server internal rpc stubs. > > We could just as easily return an object but without diving into JSON > class hinting, it'd be pretty meaningless because we'd just return "{ > 'handle': 32}" instead of "32". Right, I suggest we return nothing at all. Instead the client provides the handle. > >>> While this looks like an int on the wire, at both the server and >>> libqmp level, it looks like a BlockIoErrorEvent object. So in QEMU: >>> >>> BlockIoErrorEvent *qmp_get_block_io_error_event(Error **errp) >>> { >>> } >>> >>> And in libqmp: >>> >>> BlockIoErrorEvent *libqmp_get_block_io_error_event(QmpSession *sess, >>> Error **errp) >>> { >>> } >> >> What would the wire exchange look like? > > > { 'execute': 'get-block-io-error-event' } > < { 'return' : 32 } > ... > < { 'event': 'BLOCK_IO_ERROR', 'data': { 'action': 'stop', 'device': > 'ide0-hd0', 'operation': 'read' }, 'tag': 32 } > ... > > { 'execute': 'put-event', 'arguments': { 'tag': 32 } } Well, I may be biased, but I prefer my variant. btw, it's good to decree that a subscription is immediately followed by an event with the current state (which means events have to provide state and be idempotent); so the subscribe-and-query pattern is provided automatically. btw2, I now nominate subscribe and unsubscribe as replacements for get and put. > >>> Ignoring default events, you'll never see an event until you execute >>> a signal accessor function. When you execute this function, you >>> will start receiving the events and those events will carry a tag >>> containing the handle returned by the signal accessor. >> >> A "signal accessor" is a command to start listening to a signal? > > Yes, it basically enables the signal for the session. Okay, the subscription command. > >> So why not have the signal accessor provide the tag? Like execute: >> blah provides a tag? > > How would this map to a C API? You'd either have to completely drop > the notion of signal objects and use a separate mechanism to register > callbacks against a tag (and lose type safety) or do some major > munging to have the C API take a radically different signature than > the wire protocol. A C API could create and maintain tags under the covers (an int that keeps increasing would do). > >>> >>> Within libqmp, any time you execute a signal accessor, a new signal >>> object is created of the appropriate type. When that object is >>> destroyed, you send a put-event to stop receiving the signal. >>> >>> When you connect to a signal object (via libqmp), you don't execute >>> the signal accessor because the object is already receiving the signal. >>> >>> Default events (which exist to preserve compatibility) are a set of >>> events that are automatically connected to after qmp_capabilities is >>> executed. Because these connections are implicit, they arrive >>> without a handle in the event object. >>> >>> At this point, libqmp just ignores default events. In the future, >>> I'd like to add a command that can be executed before >>> qmp_capabilities that will avoid connecting to default events. >> >> I'm really confused. Part of that is because the conversation mixes >> libqmp, server API, and wire protocol. I'd like to understand the >> wire protocol first, everything else follows from that. > > No, it's the opposite for me. We design a good C API and then figure > out how to make it work well as a wire protocol. The whole point of > this effort is to build an API that we can consume within QEMU such > that we can start breaking large chunks of code out of the main > executable. That makes a C centric wire protocol. Clients don't have to be C.
On 03/10/2011 04:24 PM, Avi Kivity wrote: >>> What would the wire exchange look like? >> >> > { 'execute': 'get-block-io-error-event' } >> < { 'return' : 32 } >> ... >> < { 'event': 'BLOCK_IO_ERROR', 'data': { 'action': 'stop', 'device': >> 'ide0-hd0', 'operation': 'read' }, 'tag': 32 } >> ... >> > { 'execute': 'put-event', 'arguments': { 'tag': 32 } } > > > Well, I may be biased, but I prefer my variant. > > btw, it's good to decree that a subscription is immediately followed > by an event with the current state (which means events have to provide > state and be idempotent); so the subscribe-and-query pattern is > provided automatically. > > btw2, I now nominate subscribe and unsubscribe as replacements for get > and put. I also think it should be at the protocol layer: > { execute: some-command, id: foo, arguments: { ... } } < { result: { ... }, id: foo } > { subscribe: block-io-error, id: bar, arguments: { ... } } < { result: { ... } id: bar } < { event: block-io-error, id: bar, data : { ... } } > { unsubscribe: block-io-error, id: bar } < { result: { ... } id: bar } So events are now protocol-level pieces like commands, and the use of tags is uniform.
On 03/10/2011 08:24 AM, Avi Kivity wrote: > On 03/10/2011 04:12 PM, Anthony Liguori wrote: >> On 03/10/2011 06:39 AM, Avi Kivity wrote: >>> What I mean is that the client should specify the handle, like it >>> does for everything else it gives a name (netdevs, blockdevs, >>> SCM_RIGHT fds, etc). >>> >>> { execute: listen-event, arguments: { event: blah, id: blah00001 } } >>> { execute: unlisten-event arguments: { id: blah00001 } } >> >> Yeah, I understand, it just doesn't fit the model quite as well of >> signal accessors. >> >> Normally, in a signal/slot event model, you'd have some notion of an >> object model and/or hierarchy. For instance, with dbus, you'd do >> something like: >> >> bus = dbus.SystemBus() >> hal = # magic to get a hal object >> device = hal.FindDeviceByCapability('media.storage') >> >> device.connect_to_signal('media-changed', fn) >> >> We don't have objects and I don't mean to introduce them, but I like >> the idea of treating signals as objects and returning them via an >> accessor function. >> >> So the idea is that the handle is a marshalled form of the signal >> object. > > It's not a marshalled form, it's an opaque handle. A marshalled form > doesn't destroy information. > > Anyway it would work with a client-provided tag just as well. > connect_to_signal() could manufacture one and provide it to the server. It's all just bits, right? :-) We pretty much need to keep the QEMU signature the same. That would mean an internal signature of: BlockIoErrorEvent *qmp_connect_block_io_error_event(Error **errp) { } So the marshal function would then need to do something like: void qmp_marshal_connect_block_io_error_event(QmpState *state, QDict *args, QObject **ret_data, Error **errp) { BlockIoErrorEvent *ev; QObject *tag = qdict_get_obj(args, "tag"); ev = qmp_connect_block_io_error_event(errp); qmp_state_add_connection(state, ev->signal, tag); } That's not too bad, but would the schema be: [ 'connect-block-io-error-event', {}, 'BLOCK_IO_ERROR' ] Or would it be: [ 'connect-block-io-error-event', { 'tag': 'variant' }, 'none' ] I'm really opposed to adding a variant type to the schema. I'm also not a big fan of there not being a 1-1 relationship to the wire protocol and the C API. I think it's easy to rationalize 'this is how events are marshalled' vs. 'for events, a totally different declaration is generated'. >>>> { 'BLOCK_IO_ERROR': { 'device': 'str', 'action': 'str', >>>> 'operation': 'str' } } >>>> [ 'get-block-io-error-event': {}, 'BLOCK_IO_ERROR' } >>>> >>>> The way we marshal a 'BLOCK_IO_ERROR' type is by generating a >>>> unique handle and returning that. >>> >>> I don't follow at all. Where's the handle here? Why don't we >>> return the BLOCK_IO_ERROR as an object, on the wire? >> >> How we marshal an object depends on the RPC layer. >> >> We marshal events on the wire as an integer handle. That is only a >> concept within the wire protocol. > > I don't think it's an accurate description. We marshall an event as a > json object according to the schema above (BLOCK_IO_ERROR). How we > marshall a subscription to an event, or an unsubscription to an event, > depends on how we specify the protocol. It's not really a subscription to an event. It is an event. Maybe signal accessor is the wrong word. Maybe signal factory conveys a better notion of what it is. >> >> > { 'execute': 'get-block-io-error-event' } >> < { 'return' : 32 } >> ... >> < { 'event': 'BLOCK_IO_ERROR', 'data': { 'action': 'stop', 'device': >> 'ide0-hd0', 'operation': 'read' }, 'tag': 32 } >> ... >> > { 'execute': 'put-event', 'arguments': { 'tag': 32 } } > > Well, I may be biased, but I prefer my variant. > > btw, it's good to decree that a subscription is immediately followed > by an event with the current state (which means events have to provide > state and be idempotent); so the subscribe-and-query pattern is > provided automatically. Not all events are updates of data. BLOCK_IO_ERROR is a great example of this. There is no useful information that can be sent after a connection. > btw2, I now nominate subscribe and unsubscribe as replacements for get > and put. Subscribe implies sub/pub in my mind and we're not publishing events so I don't think it fits the model. A pub/sub event model would be interesting to think through but without a global namespace and object model, I don't think we can make it fit well. That's why I'm using signal/slots. It's much more conducive to a procedural model. I'm really confused. Part of that is because the conversation mixes libqmp, server API, and wire protocol. I'd like to understand the wire protocol first, everything else follows from that. >> No, it's the opposite for me. We design a good C API and then figure >> out how to make it work well as a wire protocol. The whole point of >> this effort is to build an API that we can consume within QEMU such >> that we can start breaking large chunks of code out of the main >> executable. > > That makes a C centric wire protocol. Clients don't have to be C. But a C client is by far the most important of all clients--QEMU. If we use QMP extensively internally, then we guarantee that the API is expressive and robust. If we build the API only for third-party clients, we end up with pretty much what we have today. An API with good intentions but that's more or less impossible to use in practice. Regards, Anthony Liguori
On 03/10/2011 09:30 AM, Avi Kivity wrote: > On 03/10/2011 04:24 PM, Avi Kivity wrote: >>>> What would the wire exchange look like? >>> >>> > { 'execute': 'get-block-io-error-event' } >>> < { 'return' : 32 } >>> ... >>> < { 'event': 'BLOCK_IO_ERROR', 'data': { 'action': 'stop', 'device': >>> 'ide0-hd0', 'operation': 'read' }, 'tag': 32 } >>> ... >>> > { 'execute': 'put-event', 'arguments': { 'tag': 32 } } >> >> >> Well, I may be biased, but I prefer my variant. >> >> btw, it's good to decree that a subscription is immediately followed >> by an event with the current state (which means events have to >> provide state and be idempotent); so the subscribe-and-query pattern >> is provided automatically. >> >> btw2, I now nominate subscribe and unsubscribe as replacements for >> get and put. > > I also think it should be at the protocol layer: > > > { execute: some-command, id: foo, arguments: { ... } } > < { result: { ... }, id: foo } > > { subscribe: block-io-error, id: bar, arguments: { ... } } > < { result: { ... } id: bar } > < { event: block-io-error, id: bar, data : { ... } } > > { unsubscribe: block-io-error, id: bar } > < { result: { ... } id: bar } > > So events are now protocol-level pieces like commands, and the use of > tags is uniform. Maybe for QMPv2, but for QMPv1, this is going to introduce an extremely incompatible change. Actually, we missed the json-rpc boat in designing QMP. It should look like: > { method: some-command, id: foo, params: { ... } } < { result: { ... }, id: foo, params: { ... }, error: null } > { method: connect-block-io-error, id: bar, params: { ... } } < { result: { ... }, id: bar, error: null } < { method: block-io-error, id: null, params: { ... } } Keys are different and null is passed instead of not including a tag. Events are sent exactly like methods but id is null. A result is never sent to the server for an event. One of the good things about using a code generator and IDL though is that we can add a -qmp dev,protocol=json-rpc that encodes appropriately. We can probably do this translation at the QObject level really. But in the future, we can also add -qmp dev,protocol=xml-rpc, -qmp dev,protocol=rest, or -qmp dev,protocol=asn1-rpc Regards, Anthony Liguori
On 03/10/2011 05:33 PM, Anthony Liguori wrote: > > We pretty much need to keep the QEMU signature the same. That would > mean an internal signature of: > > BlockIoErrorEvent *qmp_connect_block_io_error_event(Error **errp) > { > } > > So the marshal function would then need to do something like: > > void qmp_marshal_connect_block_io_error_event(QmpState *state, QDict > *args, QObject **ret_data, Error **errp) > { > BlockIoErrorEvent *ev; > QObject *tag = qdict_get_obj(args, "tag"); > > ev = qmp_connect_block_io_error_event(errp); > qmp_state_add_connection(state, ev->signal, tag); > } That can be mashed around however we like. > > That's not too bad, but would the schema be: > > [ 'connect-block-io-error-event', {}, 'BLOCK_IO_ERROR' ] > > Or would it be: > > [ 'connect-block-io-error-event', { 'tag': 'variant' }, 'none' ] > > I'm really opposed to adding a variant type to the schema. I'm also > not a big fan of there not being a 1-1 relationship to the wire > protocol and the C API. > > I think it's easy to rationalize 'this is how events are marshalled' > vs. 'for events, a totally different declaration is generated'. Under my latest proposal it wouldn't be in the schema at all (like command tags are not in the schema) since it's a protocol-level entity. >> I don't think it's an accurate description. We marshall an event as >> a json object according to the schema above (BLOCK_IO_ERROR). How we >> marshall a subscription to an event, or an unsubscription to an >> event, depends on how we specify the protocol. > > It's not really a subscription to an event. It is an event. No, the event happens (potentially) multiple times. Or zero. You don't "get" the event, you ask qemu to notify you. > > Maybe signal accessor is the wrong word. Maybe signal factory conveys > a better notion of what it is. It's even more confusing to me. > >>> >>> > { 'execute': 'get-block-io-error-event' } >>> < { 'return' : 32 } >>> ... >>> < { 'event': 'BLOCK_IO_ERROR', 'data': { 'action': 'stop', 'device': >>> 'ide0-hd0', 'operation': 'read' }, 'tag': 32 } >>> ... >>> > { 'execute': 'put-event', 'arguments': { 'tag': 32 } } >> >> Well, I may be biased, but I prefer my variant. >> >> btw, it's good to decree that a subscription is immediately followed >> by an event with the current state (which means events have to >> provide state and be idempotent); so the subscribe-and-query pattern >> is provided automatically. > > Not all events are updates of data. BLOCK_IO_ERROR is a great example > of this. There is no useful information that can be sent after a > connection. You could say the blockdev's state is fine, or it has encountered an error. How do you detect if there's an error if you've crashed (you=client in this case)? > >> btw2, I now nominate subscribe and unsubscribe as replacements for >> get and put. > > Subscribe implies sub/pub in my mind and we're not publishing events > so I don't think it fits the model. > > A pub/sub event model would be interesting to think through but > without a global namespace and object model, I don't think we can make > it fit well. I feel we're still not communicating. What does 'get-*-event' mean? I think you're using some nomenclature that is unfamiliar to me. > That's why I'm using signal/slots. It's much more conducive to a > procedural model. I still don't follow. We have a connection, over which we ask the other side to let us know when something happens, then that other side lets us know when it happens, then we ask it to stop, then it stops. There are no signals or slots anywhere. If there are in the code, let's not mix it up with the protocol. >> That makes a C centric wire protocol. Clients don't have to be C. > > But a C client is by far the most important of all clients--QEMU. If > we use QMP extensively internally, then we guarantee that the API is > expressive and robust. No, internally we have the most scope to fix mistakes. > > If we build the API only for third-party clients, we end up with > pretty much what we have today. An API with good intentions but > that's more or less impossible to use in practice. Or we have something that's nice for C but hard to use or inconsistent with whatever language a management client is written in.
On 03/10/2011 05:41 PM, Anthony Liguori wrote: >> I also think it should be at the protocol layer: >> >> > { execute: some-command, id: foo, arguments: { ... } } >> < { result: { ... }, id: foo } >> > { subscribe: block-io-error, id: bar, arguments: { ... } } >> < { result: { ... } id: bar } >> < { event: block-io-error, id: bar, data : { ... } } >> > { unsubscribe: block-io-error, id: bar } >> < { result: { ... } id: bar } >> >> So events are now protocol-level pieces like commands, and the use of >> tags is uniform. > > > Maybe for QMPv2, but for QMPv1, this is going to introduce an > extremely incompatible change. Why? It's 100% backwards compatible. > > Actually, we missed the json-rpc boat in designing QMP. It should > look like: > IIRC I looked at it and it didn't impress me. > > { method: some-command, id: foo, params: { ... } } > < { result: { ... }, id: foo, params: { ... }, error: null } > > { method: connect-block-io-error, id: bar, params: { ... } } > < { result: { ... }, id: bar, error: null } > < { method: block-io-error, id: null, params: { ... } } > > Keys are different and null is passed instead of not including a tag. > Events are sent exactly like methods but id is null. A result is > never sent to the server for an event. That means that we need a per-event command, instead of making it protocol-level. > > One of the good things about using a code generator and IDL though is > that we can add a -qmp dev,protocol=json-rpc that encodes > appropriately. We can probably do this translation at the QObject > level really. But in the future, we can also add -qmp > dev,protocol=xml-rpc, -qmp dev,protocol=rest, or -qmp > dev,protocol=asn1-rpc > Let's get one protocol that works first.
On 03/10/2011 09:45 AM, Avi Kivity wrote: >> >>> btw2, I now nominate subscribe and unsubscribe as replacements for >>> get and put. >> >> Subscribe implies sub/pub in my mind and we're not publishing events >> so I don't think it fits the model. >> >> A pub/sub event model would be interesting to think through but >> without a global namespace and object model, I don't think we can >> make it fit well. > > I feel we're still not communicating. What does 'get-*-event' mean? > > I think you're using some nomenclature that is unfamiliar to me. No, I'm just defending something that I think fundamentally sucks. I very purposefully am trying to avoid heavy protocol visible changes at this stage. The only reason I added signal accessors is that the current event model is unusable from a C API. I am in full agreement that the current signal model needs to be rethought and should be changed at the protocol level. I just don't want to do that right now because there are a ton of internal improvements that can be made by without doing that. The signal accessors are ugly but they're just a handful of commands that can be deprecated over time. We should revisit events but we should take the time to design it and plan for a protocol addition for 0.16. > >> That's why I'm using signal/slots. It's much more conducive to a >> procedural model. > > I still don't follow. We have a connection, over which we ask the > other side to let us know when something happens, then that other side > lets us know when it happens, then we ask it to stop, then it stops. > There are no signals or slots anywhere. If there are in the code, > let's not mix it up with the protocol. Dropping my legacy baggage, here's what I'd like to see us do from a protocol perspective. I'd like to first introduce class hinting and switch all of the string handles that we use today to class handles. So: { execute: query-block } { return: [ { __jsonclass__: 'BlockDevice', id=ide0-hd0 }, { __jsonclass__: 'BlockDevice', id=ide1-cd0 } ] } { execute: connect, arguments: { 'obj': { __jsonclass__: BlockDevice, id=ide0-hd0 }, 'signal': 'io-error' } } { return: { __jsonclass__: Connection, id=1 } } { signal: 'io-error', connection: { __jsonclass__: Connection, id=1 }, arguments: { action='stop', ... } } { execute: disconnect, arguments: { 'connection': { __jsonclass__: Connection, id=1 } } } { return: null } The advantages here are many. You get much stronger typing in C. If the schema is done right, it trivially maps to an object model in Python. Regards, Anthony Liguori >>> That makes a C centric wire protocol. Clients don't have to be C. >> >> But a C client is by far the most important of all clients--QEMU. If >> we use QMP extensively internally, then we guarantee that the API is >> expressive and robust. > > No, internally we have the most scope to fix mistakes. > >> >> If we build the API only for third-party clients, we end up with >> pretty much what we have today. An API with good intentions but >> that's more or less impossible to use in practice. > > Or we have something that's nice for C but hard to use or inconsistent > with whatever language a management client is written in. >
On 03/10/2011 09:49 AM, Avi Kivity wrote: > On 03/10/2011 05:41 PM, Anthony Liguori wrote: >>> I also think it should be at the protocol layer: >>> >>> > { execute: some-command, id: foo, arguments: { ... } } >>> < { result: { ... }, id: foo } >>> > { subscribe: block-io-error, id: bar, arguments: { ... } } >>> < { result: { ... } id: bar } >>> < { event: block-io-error, id: bar, data : { ... } } >>> > { unsubscribe: block-io-error, id: bar } >>> < { result: { ... } id: bar } >>> >>> So events are now protocol-level pieces like commands, and the use >>> of tags is uniform. >> >> >> Maybe for QMPv2, but for QMPv1, this is going to introduce an >> extremely incompatible change. > > Why? It's 100% backwards compatible. It's a very significant change for clients. While technical compatible, it would require a change to the client infrastructure to support the new feature. I'm not saying we shouldn't make a change like this, but we should minimize these type of changes. Regards, Anthony Liguori
On 03/10/2011 06:42 PM, Anthony Liguori wrote: >>> Maybe for QMPv2, but for QMPv1, this is going to introduce an >>> extremely incompatible change. >> >> Why? It's 100% backwards compatible. > > > It's a very significant change for clients. While technical > compatible, it would require a change to the client infrastructure to > support the new feature. Yes. That's generally a good thing, you implement something at the infrastructure level once instead of at a higher level N times. > I'm not saying we shouldn't make a change like this, but we should > minimize these type of changes. I agree, so we should design the protocol well, without planned obsolescence. We'll make some mistakes, but at least they'll be unplanned.
On 03/10/2011 06:04 PM, Anthony Liguori wrote: > On 03/10/2011 09:45 AM, Avi Kivity wrote: >>> >>>> btw2, I now nominate subscribe and unsubscribe as replacements for >>>> get and put. >>> >>> Subscribe implies sub/pub in my mind and we're not publishing events >>> so I don't think it fits the model. >>> >>> A pub/sub event model would be interesting to think through but >>> without a global namespace and object model, I don't think we can >>> make it fit well. >> >> I feel we're still not communicating. What does 'get-*-event' mean? >> >> I think you're using some nomenclature that is unfamiliar to me. > > No, I'm just defending something that I think fundamentally sucks. > > I very purposefully am trying to avoid heavy protocol visible changes > at this stage. The only reason I added signal accessors is that the > current event model is unusable from a C API. > > I am in full agreement that the current signal model needs to be > rethought and should be changed at the protocol level. I just don't > want to do that right now because there are a ton of internal > improvements that can be made by without doing that. > > The signal accessors are ugly but they're just a handful of commands > that can be deprecated over time. We should revisit events but we > should take the time to design it and plan for a protocol addition for > 0.16. It would be much better to avoid introducing deprecated commands. They're a maintenance burden for both server and client, and a testing burden as well. I feel it's better to spend more time to get something we truly like. > >> >>> That's why I'm using signal/slots. It's much more conducive to a >>> procedural model. >> >> I still don't follow. We have a connection, over which we ask the >> other side to let us know when something happens, then that other >> side lets us know when it happens, then we ask it to stop, then it >> stops. There are no signals or slots anywhere. If there are in the >> code, let's not mix it up with the protocol. > > Dropping my legacy baggage, here's what I'd like to see us do from a > protocol perspective. I'd like to first introduce class hinting and > switch all of the string handles that we use today to class handles. > So: > > { execute: query-block } > { return: [ { __jsonclass__: 'BlockDevice', id=ide0-hd0 }, { > __jsonclass__: 'BlockDevice', id=ide1-cd0 } ] } > > { execute: connect, arguments: { 'obj': { __jsonclass__: BlockDevice, > id=ide0-hd0 }, 'signal': 'io-error' } } > { return: { __jsonclass__: Connection, id=1 } } > > { signal: 'io-error', connection: { __jsonclass__: Connection, id=1 }, > arguments: { action='stop', ... } } > > { execute: disconnect, arguments: { 'connection': { __jsonclass__: > Connection, id=1 } } } > { return: null } > > The advantages here are many. You get much stronger typing in C. If > the schema is done right, it trivially maps to an object model in Python. I note that connect is not protocol level (i.e. execute: connect, signal: io-error instead of connect; io-error). Regarding class hinting, I don't object to that, but I feel that the schema provides all of the benefits already.
On 03/12/2011 02:42 PM, Avi Kivity wrote: > On 03/10/2011 06:04 PM, Anthony Liguori wrote: >> On 03/10/2011 09:45 AM, Avi Kivity wrote: >>>> >>>>> btw2, I now nominate subscribe and unsubscribe as replacements for >>>>> get and put. >>>> >>>> Subscribe implies sub/pub in my mind and we're not publishing >>>> events so I don't think it fits the model. >>>> >>>> A pub/sub event model would be interesting to think through but >>>> without a global namespace and object model, I don't think we can >>>> make it fit well. >>> >>> I feel we're still not communicating. What does 'get-*-event' mean? >>> >>> I think you're using some nomenclature that is unfamiliar to me. >> >> No, I'm just defending something that I think fundamentally sucks. >> >> I very purposefully am trying to avoid heavy protocol visible changes >> at this stage. The only reason I added signal accessors is that the >> current event model is unusable from a C API. >> >> I am in full agreement that the current signal model needs to be >> rethought and should be changed at the protocol level. I just don't >> want to do that right now because there are a ton of internal >> improvements that can be made by without doing that. >> >> The signal accessors are ugly but they're just a handful of commands >> that can be deprecated over time. We should revisit events but we >> should take the time to design it and plan for a protocol addition >> for 0.16. > > It would be much better to avoid introducing deprecated commands. > They're a maintenance burden for both server and client, and a testing > burden as well. I feel it's better to spend more time to get > something we truly like. I'm really not interested in getting something I like. We're at a deadlock right now with QMP. It's essentially not usable on its own because there are so few commands implemented. As we speak, the libvirt guys are adding new features based on monitor commands because they've waited too long for the QMP versions to show up. And more monitor commands keep getting added often without a QMP version. There's almost no incentive to care about QMP because it's not that useful. We need to break the deadlock and make QMP useful. That's the goal of this effort. The only reason I even touched events is because the current event infrastructure is so broken. They cannot be consumed internally in any meaningful way which means they fall into the not useful bucket. If there was an obvious alternative, I'd switch to it. But it's not as easy as it sounds. If you have a protocol level connect, then you need a way to expose events at the protocol level to a client. You could use an accessor and return a signal object but then you have a complicated life cycle. When you return a signal object to a QMP client, the object needs to persist until 1) the session goes away 2) the object is explicitly released by the client. On top of this, you also have connections which introduce a second life cycle. In reality, do you ever want to get a handle to a signal without connecting to it? I doubt it. So if you make connecting to an signal implicit in accessing it, then you only have to deal with one life cycle, the signal connection. This is the current implementation. There are nicer models but they really require having an object model whereas the signals are properties of the objects. That means you don't have to deal with an extra life cycle (because there is no explicit signal object). But really, this is one of the least important things we need to worry about. The number of commands we have with extremely hard to use semantics are alarming. The change command caused us a CVE because it's semantics are so subtle. device_del has caused device detach to be fundamentally broken in libvirt because it's semantics are subtle and undocumented. These are the important problems to solve. It's not as sexy as a JSON RPC signalling model, but it's far more important. Regards, Anthony Liguori
diff --git a/qmp-schema.json b/qmp-schema.json index 3f2dd4e..a13885f 100644 --- a/qmp-schema.json +++ b/qmp-schema.json @@ -58,3 +58,18 @@ # Since: 0.14.0 ## [ 'qmp_capabilities', {}, {}, 'none' ] + +## +# @put_event: +# +# Disconnect a signal. This command is used to disconnect from a signal based +# on the handle returned by a signal accessor. +# +# @tag: the handle returned by a signal accessor. +# +# Returns: Nothing on success. +# If @tag is not a valid handle, InvalidParameterValue +# +# Since: 0.15.0 +## +[ 'put-event', {'tag': 'int'}, {}, 'none' ]
This is needed for libqmp to support events. put-event is used to disconnect from signals. Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>