| Message ID | 20230421103207.845847-2-vsementsov@yandex-team.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pci hotplug tracking | expand |
On Fri, Apr 21, 2023 at 01:32:04PM +0300, Vladimir Sementsov-Ogievskiy wrote: > DEVICE_DELETED and DEVICE_UNPLUG_GUEST_ERROR has equal data, let's > refactor it to one structure. That also helps to add new events > consistently. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Can QAPI maintainers please review this patchset? It's been a month. > --- > qapi/qdev.json | 39 +++++++++++++++++++++++++++------------ > 1 file changed, 27 insertions(+), 12 deletions(-) > > diff --git a/qapi/qdev.json b/qapi/qdev.json > index 2708fb4e99..135cd81586 100644 > --- a/qapi/qdev.json > +++ b/qapi/qdev.json > @@ -114,16 +114,37 @@ > { 'command': 'device_del', 'data': {'id': 'str'} } > > ## > -# @DEVICE_DELETED: > +# @DeviceAndPath: > # > -# Emitted whenever the device removal completion is acknowledged by the guest. > -# At this point, it's safe to reuse the specified device ID. Device removal can > -# be initiated by the guest or by HMP/QMP commands. > +# In events we designate devices by both their ID (if the device has one) > +# and QOM path. > +# > +# Why we need ID? User specify ID in device_add command and in command line > +# and expects same identifier in the event data. > +# > +# Why we need QOM path? Some devices don't have ID and we still want to emit > +# events for them. > +# > +# So, we have a bit of redundancy, as QOM path for device that has ID is > +# always /machine/peripheral/ID. But that's hard to change keeping both > +# simple interface for most users and universality for the generic case. > # > # @device: the device's ID if it has one > # > # @path: the device's QOM path > # > +# Since: 8.0 > +## > +{ 'struct': 'DeviceAndPath', > + 'data': { '*device': 'str', 'path': 'str' } } > + Should be Since: 8.1 no? > +## > +# @DEVICE_DELETED: > +# > +# Emitted whenever the device removal completion is acknowledged by the guest. > +# At this point, it's safe to reuse the specified device ID. Device removal can > +# be initiated by the guest or by HMP/QMP commands. > +# > # Since: 1.5 > # > # Example: > @@ -134,18 +155,13 @@ > # "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } > # > ## > -{ 'event': 'DEVICE_DELETED', > - 'data': { '*device': 'str', 'path': 'str' } } > +{ 'event': 'DEVICE_DELETED', 'data': 'DeviceAndPath' } > > ## > # @DEVICE_UNPLUG_GUEST_ERROR: > # > # Emitted when a device hot unplug fails due to a guest reported error. > # > -# @device: the device's ID if it has one > -# > -# @path: the device's QOM path > -# > # Since: 6.2 > # > # Example: > @@ -156,5 +172,4 @@ > # "timestamp": { "seconds": 1615570772, "microseconds": 202844 } } > # > ## > -{ 'event': 'DEVICE_UNPLUG_GUEST_ERROR', > - 'data': { '*device': 'str', 'path': 'str' } } > +{ 'event': 'DEVICE_UNPLUG_GUEST_ERROR', 'data': 'DeviceAndPath' } > -- > 2.34.1
"Michael S. Tsirkin" <mst@redhat.com> writes: > On Fri, Apr 21, 2023 at 01:32:04PM +0300, Vladimir Sementsov-Ogievskiy wrote: >> DEVICE_DELETED and DEVICE_UNPLUG_GUEST_ERROR has equal data, let's >> refactor it to one structure. That also helps to add new events >> consistently. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > > Can QAPI maintainers please review this patchset? > It's been a month. It's been a busy month; sorry for the delay. >> --- >> qapi/qdev.json | 39 +++++++++++++++++++++++++++------------ >> 1 file changed, 27 insertions(+), 12 deletions(-) >> >> diff --git a/qapi/qdev.json b/qapi/qdev.json >> index 2708fb4e99..135cd81586 100644 >> --- a/qapi/qdev.json >> +++ b/qapi/qdev.json >> @@ -114,16 +114,37 @@ >> { 'command': 'device_del', 'data': {'id': 'str'} } >> >> ## >> -# @DEVICE_DELETED: >> +# @DeviceAndPath: >> # >> -# Emitted whenever the device removal completion is acknowledged by the guest. >> -# At this point, it's safe to reuse the specified device ID. Device removal can >> -# be initiated by the guest or by HMP/QMP commands. >> +# In events we designate devices by both their ID (if the device has one) >> +# and QOM path. >> +# >> +# Why we need ID? User specify ID in device_add command and in command line >> +# and expects same identifier in the event data. >> +# >> +# Why we need QOM path? Some devices don't have ID and we still want to emit >> +# events for them. >> +# >> +# So, we have a bit of redundancy, as QOM path for device that has ID is >> +# always /machine/peripheral/ID. But that's hard to change keeping both >> +# simple interface for most users and universality for the generic case. Hmm. I appreciate rationale, but I'm not sure it fits here. Would readers be worse off if we dropped it? >> # >> # @device: the device's ID if it has one >> # >> # @path: the device's QOM path >> # >> +# Since: 8.0 >> +## >> +{ 'struct': 'DeviceAndPath', >> + 'data': { '*device': 'str', 'path': 'str' } } >> + > > Should be Since: 8.1 no? Yes. Please format like ## # @DeviceAndPath: # # In events we designate devices by both their ID (if the device has # one) and QOM path. # # Why we need ID? User specify ID in device_add command and in # command line and expects same identifier in the event data. # # Why we need QOM path? Some devices don't have ID and we still want # to emit events for them. # # So, we have a bit of redundancy, as QOM path for device that has ID # is always /machine/peripheral/ID. But that's hard to change keeping # both simple interface for most users and universality for the # generic case. # # @device: the device's ID if it has one # # @path: the device's QOM path # # Since: 8.0 ## to blend in with recent commit a937b6aa739 (qapi: Reformat doc comments to conform to current conventions). >> +## >> +# @DEVICE_DELETED: >> +# >> +# Emitted whenever the device removal completion is acknowledged by the guest. >> +# At this point, it's safe to reuse the specified device ID. Device removal can >> +# be initiated by the guest or by HMP/QMP commands. >> +# Conflict resolution: # Emitted whenever the device removal completion is acknowledged by # the guest. At this point, it's safe to reuse the specified device # ID. Device removal can be initiated by the guest or by HMP/QMP # commands. >> # Since: 1.5 >> # >> # Example: >> @@ -134,18 +155,13 @@ >> # "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } >> # >> ## >> -{ 'event': 'DEVICE_DELETED', >> - 'data': { '*device': 'str', 'path': 'str' } } >> +{ 'event': 'DEVICE_DELETED', 'data': 'DeviceAndPath' } >> >> ## >> # @DEVICE_UNPLUG_GUEST_ERROR: >> # >> # Emitted when a device hot unplug fails due to a guest reported error. >> # >> -# @device: the device's ID if it has one >> -# >> -# @path: the device's QOM path >> -# >> # Since: 6.2 >> # >> # Example: >> @@ -156,5 +172,4 @@ >> # "timestamp": { "seconds": 1615570772, "microseconds": 202844 } } >> # >> ## >> -{ 'event': 'DEVICE_UNPLUG_GUEST_ERROR', >> - 'data': { '*device': 'str', 'path': 'str' } } >> +{ 'event': 'DEVICE_UNPLUG_GUEST_ERROR', 'data': 'DeviceAndPath' } >> -- >> 2.34.1
On 22.05.23 12:27, Markus Armbruster wrote: > "Michael S. Tsirkin" <mst@redhat.com> writes: > >> On Fri, Apr 21, 2023 at 01:32:04PM +0300, Vladimir Sementsov-Ogievskiy wrote: >>> DEVICE_DELETED and DEVICE_UNPLUG_GUEST_ERROR has equal data, let's >>> refactor it to one structure. That also helps to add new events >>> consistently. >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >> >> Can QAPI maintainers please review this patchset? >> It's been a month. > > It's been a busy month; sorry for the delay. > >>> --- >>> qapi/qdev.json | 39 +++++++++++++++++++++++++++------------ >>> 1 file changed, 27 insertions(+), 12 deletions(-) >>> >>> diff --git a/qapi/qdev.json b/qapi/qdev.json >>> index 2708fb4e99..135cd81586 100644 >>> --- a/qapi/qdev.json >>> +++ b/qapi/qdev.json >>> @@ -114,16 +114,37 @@ >>> { 'command': 'device_del', 'data': {'id': 'str'} } >>> >>> ## >>> -# @DEVICE_DELETED: >>> +# @DeviceAndPath: >>> # >>> -# Emitted whenever the device removal completion is acknowledged by the guest. >>> -# At this point, it's safe to reuse the specified device ID. Device removal can >>> -# be initiated by the guest or by HMP/QMP commands. >>> +# In events we designate devices by both their ID (if the device has one) >>> +# and QOM path. >>> +# >>> +# Why we need ID? User specify ID in device_add command and in command line >>> +# and expects same identifier in the event data. >>> +# >>> +# Why we need QOM path? Some devices don't have ID and we still want to emit >>> +# events for them. >>> +# >>> +# So, we have a bit of redundancy, as QOM path for device that has ID is >>> +# always /machine/peripheral/ID. But that's hard to change keeping both >>> +# simple interface for most users and universality for the generic case. > > Hmm. I appreciate rationale, but I'm not sure it fits here. Would > readers be worse off if we dropped it? Is there a syntax to add comment to the QAPI structure, which doesn't go into compiled public documentation? I agree that we don't need this in compiled documentation, but this place in the code really good for the rationale, to avoid starting the discussion from the beginning again. > >>> # >>> # @device: the device's ID if it has one >>> # >>> # @path: the device's QOM path >>> # >>> +# Since: 8.0 >>> +## >>> +{ 'struct': 'DeviceAndPath', >>> + 'data': { '*device': 'str', 'path': 'str' } } >>> + >> >> Should be Since: 8.1 no? > > Yes. > > Please format like > > ## > # @DeviceAndPath: > # > # In events we designate devices by both their ID (if the device has > # one) and QOM path. > # > # Why we need ID? User specify ID in device_add command and in > # command line and expects same identifier in the event data. > # > # Why we need QOM path? Some devices don't have ID and we still want > # to emit events for them. > # > # So, we have a bit of redundancy, as QOM path for device that has ID > # is always /machine/peripheral/ID. But that's hard to change keeping > # both simple interface for most users and universality for the > # generic case. > # > # @device: the device's ID if it has one > # > # @path: the device's QOM path > # > # Since: 8.0 > ## > > to blend in with recent commit a937b6aa739 (qapi: Reformat doc comments > to conform to current conventions). > >>> +## >>> +# @DEVICE_DELETED: >>> +# >>> +# Emitted whenever the device removal completion is acknowledged by the guest. >>> +# At this point, it's safe to reuse the specified device ID. Device removal can >>> +# be initiated by the guest or by HMP/QMP commands. >>> +# > > Conflict resolution: > > # Emitted whenever the device removal completion is acknowledged by > # the guest. At this point, it's safe to reuse the specified device > # ID. Device removal can be initiated by the guest or by HMP/QMP > # commands. > >>> # Since: 1.5 >>> # >>> # Example: >>> @@ -134,18 +155,13 @@ >>> # "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } >>> # >>> ## >>> -{ 'event': 'DEVICE_DELETED', >>> - 'data': { '*device': 'str', 'path': 'str' } } >>> +{ 'event': 'DEVICE_DELETED', 'data': 'DeviceAndPath' } >>> >>> ## >>> # @DEVICE_UNPLUG_GUEST_ERROR: >>> # >>> # Emitted when a device hot unplug fails due to a guest reported error. >>> # >>> -# @device: the device's ID if it has one >>> -# >>> -# @path: the device's QOM path >>> -# >>> # Since: 6.2 >>> # >>> # Example: >>> @@ -156,5 +172,4 @@ >>> # "timestamp": { "seconds": 1615570772, "microseconds": 202844 } } >>> # >>> ## >>> -{ 'event': 'DEVICE_UNPLUG_GUEST_ERROR', >>> - 'data': { '*device': 'str', 'path': 'str' } } >>> +{ 'event': 'DEVICE_UNPLUG_GUEST_ERROR', 'data': 'DeviceAndPath' } >>> -- >>> 2.34.1 >
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes: > On 22.05.23 12:27, Markus Armbruster wrote: >> "Michael S. Tsirkin" <mst@redhat.com> writes: >> >>> On Fri, Apr 21, 2023 at 01:32:04PM +0300, Vladimir Sementsov-Ogievskiy wrote: >>>> DEVICE_DELETED and DEVICE_UNPLUG_GUEST_ERROR has equal data, let's >>>> refactor it to one structure. That also helps to add new events >>>> consistently. >>>> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >>> >>> Can QAPI maintainers please review this patchset? >>> It's been a month. >> >> It's been a busy month; sorry for the delay. >> >>>> --- >>>> qapi/qdev.json | 39 +++++++++++++++++++++++++++------------ >>>> 1 file changed, 27 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/qapi/qdev.json b/qapi/qdev.json >>>> index 2708fb4e99..135cd81586 100644 >>>> --- a/qapi/qdev.json >>>> +++ b/qapi/qdev.json >>>> @@ -114,16 +114,37 @@ >>>> { 'command': 'device_del', 'data': {'id': 'str'} } >>>> >>>> ## >>>> -# @DEVICE_DELETED: >>>> +# @DeviceAndPath: >>>> # >>>> -# Emitted whenever the device removal completion is acknowledged by the guest. >>>> -# At this point, it's safe to reuse the specified device ID. Device removal can >>>> -# be initiated by the guest or by HMP/QMP commands. >>>> +# In events we designate devices by both their ID (if the device has one) >>>> +# and QOM path. >>>> +# >>>> +# Why we need ID? User specify ID in device_add command and in command line >>>> +# and expects same identifier in the event data. >>>> +# >>>> +# Why we need QOM path? Some devices don't have ID and we still want to emit >>>> +# events for them. >>>> +# >>>> +# So, we have a bit of redundancy, as QOM path for device that has ID is >>>> +# always /machine/peripheral/ID. But that's hard to change keeping both >>>> +# simple interface for most users and universality for the generic case. >> >> Hmm. I appreciate rationale, but I'm not sure it fits here. Would >> readers be worse off if we dropped it? > > Is there a syntax to add comment to the QAPI structure, which doesn't go into compiled public documentation? Yes! qapi-code-gen.rst: "A multi-line comment that starts and ends with a ``##`` line is a documentation comment." All other comments are not, and won't be included in generated documentation. Example: qapi/qapi-schema.json has { 'include': 'pragma.json' } # Documentation generated with qapi-gen.py is in source order, with # included sub-schemas inserted at the first include directive # (subsequent include directives have no effect). To get a sane and # stable order, it's best to include each sub-schema just once, or # include it first right here. { 'include': 'error.json' } Not a documentation comment, thus not included in generated documentation. Additionally, TODO sections in documentation comments are omitted from generated documentation. qapi-code-gen.rst again: "TODO" sections are not rendered at all (they are for developers, not users of QMP). > I agree that we don't need this in compiled documentation, but this place in the code really good for the rationale, to avoid starting the discussion from the beginning again. Saving rationale so we can refer to it later is good. We tend to use commit messages for that. I'd say use comments when the rationale needs to be more visible. [...]
On 22.05.23 15:13, Markus Armbruster wrote: > Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes: > >> On 22.05.23 12:27, Markus Armbruster wrote: >>> "Michael S. Tsirkin" <mst@redhat.com> writes: >>> >>>> On Fri, Apr 21, 2023 at 01:32:04PM +0300, Vladimir Sementsov-Ogievskiy wrote: >>>>> DEVICE_DELETED and DEVICE_UNPLUG_GUEST_ERROR has equal data, let's >>>>> refactor it to one structure. That also helps to add new events >>>>> consistently. >>>>> >>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >>>> >>>> Can QAPI maintainers please review this patchset? >>>> It's been a month. >>> >>> It's been a busy month; sorry for the delay. >>> >>>>> --- >>>>> qapi/qdev.json | 39 +++++++++++++++++++++++++++------------ >>>>> 1 file changed, 27 insertions(+), 12 deletions(-) >>>>> >>>>> diff --git a/qapi/qdev.json b/qapi/qdev.json >>>>> index 2708fb4e99..135cd81586 100644 >>>>> --- a/qapi/qdev.json >>>>> +++ b/qapi/qdev.json >>>>> @@ -114,16 +114,37 @@ >>>>> { 'command': 'device_del', 'data': {'id': 'str'} } >>>>> >>>>> ## >>>>> -# @DEVICE_DELETED: >>>>> +# @DeviceAndPath: >>>>> # >>>>> -# Emitted whenever the device removal completion is acknowledged by the guest. >>>>> -# At this point, it's safe to reuse the specified device ID. Device removal can >>>>> -# be initiated by the guest or by HMP/QMP commands. >>>>> +# In events we designate devices by both their ID (if the device has one) >>>>> +# and QOM path. >>>>> +# >>>>> +# Why we need ID? User specify ID in device_add command and in command line >>>>> +# and expects same identifier in the event data. >>>>> +# >>>>> +# Why we need QOM path? Some devices don't have ID and we still want to emit >>>>> +# events for them. >>>>> +# >>>>> +# So, we have a bit of redundancy, as QOM path for device that has ID is >>>>> +# always /machine/peripheral/ID. But that's hard to change keeping both >>>>> +# simple interface for most users and universality for the generic case. >>> >>> Hmm. I appreciate rationale, but I'm not sure it fits here. Would >>> readers be worse off if we dropped it? >> >> Is there a syntax to add comment to the QAPI structure, which doesn't go into compiled public documentation? > > Yes! qapi-code-gen.rst: "A multi-line comment that starts and ends with > a ``##`` line is a documentation comment." All other comments are not, > and won't be included in generated documentation. Good, thanks! > > Example: qapi/qapi-schema.json has > > { 'include': 'pragma.json' } > > # Documentation generated with qapi-gen.py is in source order, with > # included sub-schemas inserted at the first include directive > # (subsequent include directives have no effect). To get a sane and > # stable order, it's best to include each sub-schema just once, or > # include it first right here. > > { 'include': 'error.json' } > > Not a documentation comment, thus not included in generated > documentation. > > Additionally, TODO sections in documentation comments are omitted from > generated documentation. qapi-code-gen.rst again: "TODO" sections are > not rendered at all (they are for developers, not users of QMP). > >> I agree that we don't need this in compiled documentation, but this place in the code really good for the rationale, to avoid starting the discussion from the beginning again. > > Saving rationale so we can refer to it later is good. We tend to use > commit messages for that. I'd say use comments when the rationale needs > to be more visible. > Yes I can move this to the commit message. I just wasn't sure that it's an obvious place in our case, as that's not a commit that introduces new structure but just a no-logic-change refactoring.
diff --git a/qapi/qdev.json b/qapi/qdev.json index 2708fb4e99..135cd81586 100644 --- a/qapi/qdev.json +++ b/qapi/qdev.json @@ -114,16 +114,37 @@ { 'command': 'device_del', 'data': {'id': 'str'} } ## -# @DEVICE_DELETED: +# @DeviceAndPath: # -# Emitted whenever the device removal completion is acknowledged by the guest. -# At this point, it's safe to reuse the specified device ID. Device removal can -# be initiated by the guest or by HMP/QMP commands. +# In events we designate devices by both their ID (if the device has one) +# and QOM path. +# +# Why we need ID? User specify ID in device_add command and in command line +# and expects same identifier in the event data. +# +# Why we need QOM path? Some devices don't have ID and we still want to emit +# events for them. +# +# So, we have a bit of redundancy, as QOM path for device that has ID is +# always /machine/peripheral/ID. But that's hard to change keeping both +# simple interface for most users and universality for the generic case. # # @device: the device's ID if it has one # # @path: the device's QOM path # +# Since: 8.0 +## +{ 'struct': 'DeviceAndPath', + 'data': { '*device': 'str', 'path': 'str' } } + +## +# @DEVICE_DELETED: +# +# Emitted whenever the device removal completion is acknowledged by the guest. +# At this point, it's safe to reuse the specified device ID. Device removal can +# be initiated by the guest or by HMP/QMP commands. +# # Since: 1.5 # # Example: @@ -134,18 +155,13 @@ # "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } # ## -{ 'event': 'DEVICE_DELETED', - 'data': { '*device': 'str', 'path': 'str' } } +{ 'event': 'DEVICE_DELETED', 'data': 'DeviceAndPath' } ## # @DEVICE_UNPLUG_GUEST_ERROR: # # Emitted when a device hot unplug fails due to a guest reported error. # -# @device: the device's ID if it has one -# -# @path: the device's QOM path -# # Since: 6.2 # # Example: @@ -156,5 +172,4 @@ # "timestamp": { "seconds": 1615570772, "microseconds": 202844 } } # ## -{ 'event': 'DEVICE_UNPLUG_GUEST_ERROR', - 'data': { '*device': 'str', 'path': 'str' } } +{ 'event': 'DEVICE_UNPLUG_GUEST_ERROR', 'data': 'DeviceAndPath' }
DEVICE_DELETED and DEVICE_UNPLUG_GUEST_ERROR has equal data, let's refactor it to one structure. That also helps to add new events consistently. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> --- qapi/qdev.json | 39 +++++++++++++++++++++++++++------------ 1 file changed, 27 insertions(+), 12 deletions(-)