| Message ID | 9d3f0e0ee6fcfc6300e165f79b46a4af0ffdc37d.1426779661.git.berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
On 2015-03-19 at 11:43, Alberto Garcia wrote: > Since this event can occur in nodes that don't have a device name > associated, use the node name as fallback in those cases. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > block/qcow2.c | 5 +++-- > docs/qmp/qmp-events.txt | 2 +- > qapi/block-core.json | 2 +- > 3 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/block/qcow2.c b/block/qcow2.c > index 168006b..d808c70 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -2832,8 +2832,9 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset, > "corruption events will be suppressed\n", message); > } > > - qapi_event_send_block_image_corrupted(bdrv_get_device_name(bs), message, > - offset >= 0, offset, size >= 0, size, > + qapi_event_send_block_image_corrupted(bdrv_get_device_or_node_name(bs), > + message, offset >= 0, offset, > + size >= 0, size, > fatal, &error_abort); > g_free(message); > > diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt > index d759d19..75f3e68 100644 > --- a/docs/qmp/qmp-events.txt > +++ b/docs/qmp/qmp-events.txt > @@ -35,7 +35,7 @@ Emitted when a disk image is being marked corrupt. > > Data: > > -- "device": Device name (json-string) > +- "device": Device name, or node name if not present (json-string) > - "msg": Informative message (e.g., reason for the corruption) (json-string) > - "offset": If the corruption resulted from an image access, this is the access > offset into the image (json-int) > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 42c8850..3b51c68 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -1751,7 +1751,7 @@ > # > # Emitted when a corruption has been detected in a disk image > # > -# @device: device name > +# @device: device name, or node name if not present > # > # @msg: informative message for human consumption, such as the kind of > # corruption being detected. It should not be parsed by machine as it is Basically the same as my reply to patch 2, but here it's a formal question as well: Normally, if a field in QMP is designed @device, it contains a device name. We do have combined device/node name fields, though (as of John's incremental backup series, at least), but those are named @node (which I proposed for patch 2, too). But renaming the field here will lead to breaking backwards compatibility. I think just adding a @node-name field and keeping @device as it is should be good enough here. Max
(I forgot to Cc Eric in this series, doing it now) On Thu, Mar 19, 2015 at 03:42:35PM -0400, Max Reitz wrote: > > # Emitted when a corruption has been detected in a disk image > > # > >-# @device: device name > >+# @device: device name, or node name if not present > > Normally, if a field in QMP is designed @device, it contains a > device name. We do have combined device/node name fields, though (as > of John's incremental backup series, at least), but those are named > @node (which I proposed for patch 2, too). > > But renaming the field here will lead to breaking backwards > compatibility. I think just adding a @node-name field and keeping > @device as it is should be good enough here. I was doing the same that we discussed for BlockJobInfo here, where option b) seemed to have a bit more support: https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg03651.html But yeah I personally don't mind extending the event with a new field. Would we make 'device' optional in this case? Berto
On 2015-03-19 at 17:42, Alberto Garcia wrote: > (I forgot to Cc Eric in this series, doing it now) > > On Thu, Mar 19, 2015 at 03:42:35PM -0400, Max Reitz wrote: >>> # Emitted when a corruption has been detected in a disk image >>> # >>> -# @device: device name >>> +# @device: device name, or node name if not present >> Normally, if a field in QMP is designed @device, it contains a >> device name. We do have combined device/node name fields, though (as >> of John's incremental backup series, at least), but those are named >> @node (which I proposed for patch 2, too). >> >> But renaming the field here will lead to breaking backwards >> compatibility. I think just adding a @node-name field and keeping >> @device as it is should be good enough here. > I was doing the same that we discussed for BlockJobInfo here, where > option b) seemed to have a bit more support: > > https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg03651.html > > But yeah I personally don't mind extending the event with a new field. > Would we make 'device' optional in this case? No, I think we'd need to keep it. It isn't optional right now so any software using the monitor will expect it to be present (even if it's empty). Regarding the BlockJobInfo discussion: One argument I can see there is "Particularly if we don't have two parameters for starting the job, then we don't need two parameters for reporting it"/"If you're going to reuse 'device' on the creation, then reuse it on the reporting", which does not apply here (there is no command corresponding to this event, it just pops up on its own), so there will not be any asymmetry here. Other than that, the only argument I can see is "it will work with libvirt, so it is fine", but that's not really a reason to prefer b) over a)... So in this case here I don't really see a benefit of reusing @device instead of just adding @node-name, whereas adding @node-name will have the benefit of not affecting anybody and (in my opinion) being more explicit. However, if others tend to think otherwise (the @node-name vs. @node vs. @device is a constant point of dissent over naming...), I'm happy to be convinced otherwise. In the end it doesn't really matter after all, it's a machine-readable protocol. If software can work with it, it's fine. Max
On Thu, Mar 19, 2015 at 05:52:52PM -0400, Max Reitz wrote: > So in this case here I don't really see a benefit of reusing @device > instead of just adding @node-name, whereas adding @node-name will > have the benefit of not affecting anybody and (in my opinion) being > more explicit. However, if others tend to think otherwise (the > @node-name vs. @node vs. @device is a constant point of dissent > over naming...), I'm happy to be convinced otherwise. In the end it > doesn't really matter after all, it's a machine-readable protocol. > If software can work with it, it's fine. It's fine with me as well, I just had the impression that there was a consensus towards not extending the API if possible, but in my case I'm happy to implement whichever alternative. Berto
On 03/19/2015 03:42 PM, Alberto Garcia wrote: > (I forgot to Cc Eric in this series, doing it now) > > On Thu, Mar 19, 2015 at 03:42:35PM -0400, Max Reitz wrote: >>> # Emitted when a corruption has been detected in a disk image >>> # >>> -# @device: device name >>> +# @device: device name, or node name if not present >> >> Normally, if a field in QMP is designed @device, it contains a >> device name. We do have combined device/node name fields, though (as >> of John's incremental backup series, at least), but those are named >> @node (which I proposed for patch 2, too). >> >> But renaming the field here will lead to breaking backwards >> compatibility. I think just adding a @node-name field and keeping >> @device as it is should be good enough here. > > I was doing the same that we discussed for BlockJobInfo here, where > option b) seemed to have a bit more support: > > https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg03651.html > > But yeah I personally don't mind extending the event with a new field. > Would we make 'device' optional in this case? How hard is it to output both 'device' and 'node' in the same event, if both are available? And does it add anything? I could live with the simplicity of just returning a device name where we have one (back-compat for older clients that only ever start jobs on a device) and a node name where we don't (such an event can only be triggered by a job started by a client new enough to know how to start a job by node name), and just always use the 'device' field while documenting that it is not the best field name for what its contents represent. On the other hand, if libvirt starts using node names everywhere, then returning a device name instead of a node name makes libvirt have to do a bit more work to map a device name down to a node name (not the end of the world, because the device name is still unambiguous); whereas returning both device AND node name at once may make libvirt's life easier. And for this particular event, which is not tied to block jobs but to generic block operation, isn't it possible that we could be reporting a corrupted backing chain where we have neither a device name (it is not the active layer) nor a node name (if we don't add Jeff's patch to auto-name all nodes)? In such a case, I don't know that we can do much better anyways.
On Thu, Mar 19, 2015 at 04:15:49PM -0600, Eric Blake wrote: > >>> -# @device: device name > >>> +# @device: device name, or node name if not present > >> > >> I think just adding a @node-name field and keeping @device as it > >> is should be good enough here. > > > > I was doing the same that we discussed for BlockJobInfo here, where > > option b) seemed to have a bit more support: > > > > https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg03651.html > > > > But yeah I personally don't mind extending the event with a new field. > > Would we make 'device' optional in this case? > > How hard is it to output both 'device' and 'node' in the same event, > if both are available? It's a trivial change, there's no problem at all. I assume that, for compatibility reasons, 'device' would continue to be present even if it's empty, but would you prefer to have 'node-name' as an optional field? > And for this particular event, which is not tied to block jobs but > to generic block operation, isn't it possible that we could be > reporting a corrupted backing chain where we have neither a device > name (it is not the active layer) nor a node name (if we don't add > Jeff's patch to auto-name all nodes)? In such a case, I don't know > that we can do much better anyways. Yes, it is perfectly possible. I guess any software that wants to handle those scenarios probably wants to give names to all nodes. From the QEMU side, apart from giving automatic names to all nodes I don't see any other solution. Berto
On 03/19/2015 04:38 PM, Alberto Garcia wrote: >> And for this particular event, which is not tied to block jobs but >> to generic block operation, isn't it possible that we could be >> reporting a corrupted backing chain where we have neither a device >> name (it is not the active layer) nor a node name (if we don't add >> Jeff's patch to auto-name all nodes)? In such a case, I don't know >> that we can do much better anyways. > > Yes, it is perfectly possible. I guess any software that wants to > handle those scenarios probably wants to give names to all nodes. > > From the QEMU side, apart from giving automatic names to all nodes I > don't see any other solution. In the context of block commit, libvirt found that reporting device name to the end user was nicer than file name (since there are two events emitted - one when the active layer has become synced with the lower layer, and therefore where the node associated with device is still 'top'; and the other when the active layer has pivoted to the base layer and top is no longer valid, therefore where the node associated with device is now 'base' - since the two events are related but have different file [node] names, having the device name made life easier). But note that we still haven't exposed 'node' information in any of the block job events (BLOCK_JOB_COMPLETED, BLOCK_JOB_CANCELLED, BLOCK_JOB_ERROR), and it might still make sense to do so, even if libvirt doesn't need to expose those node names on to the end user. But this is a counter-example - knowing just the device name does NOT tell you which node is corrupted, if there is any possibility of a snapshot or active commit changing the active node associated with the device. That's because there might be a race between the event announcing corruption and some other action modifying the chain, such that the client's notion of the active element of the chain is different from the active element at the time the event was reported. So the more I think about it, the more I'd like for this event to output both 'device' (mandatory, with an empty string if we can't easily tie the BDS to a single device) and 'node' (where 'node' can be optional, and omitted if the BDS does not have a node name [if we ever add generated node names, then we could make 'node' mandatory at that time]).
On Thu, Mar 19, 2015 at 05:14:24PM -0600, Eric Blake wrote: > So the more I think about it, the more I'd like for this event to > output both 'device' (mandatory, with an empty string if we can't > easily tie the BDS to a single device) and 'node' (where 'node' can > be optional, and omitted if the BDS does not have a node name [if we > ever add generated node names, then we could make 'node' mandatory > at that time]). Ok, I will change that. On a related note, the QUORUM_FAILURE event does have this mixed field already, called "reference", which is the device name if present, else the node name. But I'm not planning to change it unless you think it's a good idea. That would be a thing for a different patch anyway. Berto
diff --git a/block/qcow2.c b/block/qcow2.c index 168006b..d808c70 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2832,8 +2832,9 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset, "corruption events will be suppressed\n", message); } - qapi_event_send_block_image_corrupted(bdrv_get_device_name(bs), message, - offset >= 0, offset, size >= 0, size, + qapi_event_send_block_image_corrupted(bdrv_get_device_or_node_name(bs), + message, offset >= 0, offset, + size >= 0, size, fatal, &error_abort); g_free(message); diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt index d759d19..75f3e68 100644 --- a/docs/qmp/qmp-events.txt +++ b/docs/qmp/qmp-events.txt @@ -35,7 +35,7 @@ Emitted when a disk image is being marked corrupt. Data: -- "device": Device name (json-string) +- "device": Device name, or node name if not present (json-string) - "msg": Informative message (e.g., reason for the corruption) (json-string) - "offset": If the corruption resulted from an image access, this is the access offset into the image (json-int) diff --git a/qapi/block-core.json b/qapi/block-core.json index 42c8850..3b51c68 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1751,7 +1751,7 @@ # # Emitted when a corruption has been detected in a disk image # -# @device: device name +# @device: device name, or node name if not present # # @msg: informative message for human consumption, such as the kind of # corruption being detected. It should not be parsed by machine as it is
Since this event can occur in nodes that don't have a device name associated, use the node name as fallback in those cases. Signed-off-by: Alberto Garcia <berto@igalia.com> --- block/qcow2.c | 5 +++-- docs/qmp/qmp-events.txt | 2 +- qapi/block-core.json | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-)