qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Max Reitz <mreitz@redhat.com>
To: Alberto Garcia <berto@igalia.com>
Cc: Kevin Wolf <kwolf@redhat.com>,
	qemu-devel@nongnu.org, Stefan Hajnoczi <stefanha@redhat.com>
Subject: Re: [Qemu-devel] [PATCH 3/3] block: allow BLOCK_IMAGE_CORRUPTED to have a node name
Date: Thu, 19 Mar 2015 17:52:52 -0400	[thread overview]
Message-ID: <550B4534.108@redhat.com> (raw)
In-Reply-To: <20150319214201.GA11212@igalia.com>

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

  reply	other threads:[~2015-03-19 21:53 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-19 15:43 [Qemu-devel] [PATCH 0/3] Add bdrv_get_device_or_node_name() Alberto Garcia
2015-03-19 15:43 ` [Qemu-devel] [PATCH 1/3] block: add bdrv_get_device_or_node_name() Alberto Garcia
2015-03-19 19:26   ` Max Reitz
2015-03-20  7:40   ` Markus Armbruster
2015-03-20  8:03     ` Alberto Garcia
2015-03-20  8:47       ` Markus Armbruster
2015-03-19 15:43 ` [Qemu-devel] [PATCH 2/3] block: use bdrv_get_device_or_node_name() in error messages Alberto Garcia
2015-03-19 19:37   ` Max Reitz
2015-03-20  7:52   ` Markus Armbruster
2015-03-19 15:43 ` [Qemu-devel] [PATCH 3/3] block: allow BLOCK_IMAGE_CORRUPTED to have a node name Alberto Garcia
2015-03-19 19:42   ` Max Reitz
2015-03-19 21:42     ` Alberto Garcia
2015-03-19 21:52       ` Max Reitz [this message]
2015-03-19 22:04         ` Alberto Garcia
2015-03-19 22:15       ` Eric Blake
2015-03-19 22:38         ` Alberto Garcia
2015-03-19 23:14           ` Eric Blake
2015-03-20  9:23             ` Alberto Garcia
2015-03-19 19:37 ` [Qemu-devel] [PATCH 0/3] Add bdrv_get_device_or_node_name() Max Reitz
2015-03-19 21:49   ` Alberto Garcia

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=550B4534.108@redhat.com \
    --to=mreitz@redhat.com \
    --cc=berto@igalia.com \
    --cc=kwolf@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=stefanha@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).