| Message ID | 1386263703-19292-5-git-send-email-benoit@irqsave.net |
|---|---|
| State | New |
| Headers | show |
On Thu, 5 Dec 2013 18:15:00 +0100 Benoît Canet <benoit@irqsave.net> wrote: > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -1675,7 +1675,11 @@ > # determine which ones are encrypted, set the passwords with this command, and > # then start the guest with the @cont command. > # > -# @device: the name of the device to set the password on > +# Either @device or @node-name must be set but not both. > +# > +# @device: #optional the name of the block backend device to set the password on > +# > +# @node-name: #optional graph node name to set the password on (Since 2.0) > # > # @password: the password to use for the device > # > @@ -1689,7 +1693,8 @@ > # > # Since: 0.14.0 > ## > -{ 'command': 'block_passwd', 'data': {'device': 'str', 'password': 'str'} } > +{ 'command': 'block_passwd', 'data': {'*device': 'str', > + '*node-name': 'str', 'password': 'str'} } What about: { 'command': 'block_passwd', 'data': {'device': 'str', '*device-is-node': 'bool', 'password': 'str'} }
On 12/06/2013 07:27 AM, Luiz Capitulino wrote: > On Thu, 5 Dec 2013 18:15:00 +0100 > Benoît Canet <benoit@irqsave.net> wrote: >> -{ 'command': 'block_passwd', 'data': {'device': 'str', 'password': 'str'} } >> +{ 'command': 'block_passwd', 'data': {'*device': 'str', >> + '*node-name': 'str', 'password': 'str'} } > > What about: > > { 'command': 'block_passwd', 'data': {'device': 'str', > '*device-is-node': 'bool', 'password': 'str'} } That would also work; the naming is a bit more awkward, but then you don't have the issue of mutually-exclusive optional arguments where exactly one of the two arguments is required. I'm leaning slightly towards the approach that Benoît took, if only for the naming aspect (that is, I also thought of the idea of a bool flag, but didn't suggest it because I didn't like the implications on the naming). But I can live with either approach, if anyone else has a strong opinion.
On Fri, 06 Dec 2013 08:24:33 -0700 Eric Blake <eblake@redhat.com> wrote: > On 12/06/2013 07:27 AM, Luiz Capitulino wrote: > > On Thu, 5 Dec 2013 18:15:00 +0100 > > Benoît Canet <benoit@irqsave.net> wrote: > > >> -{ 'command': 'block_passwd', 'data': {'device': 'str', 'password': 'str'} } > >> +{ 'command': 'block_passwd', 'data': {'*device': 'str', > >> + '*node-name': 'str', 'password': 'str'} } > > > > What about: > > > > { 'command': 'block_passwd', 'data': {'device': 'str', > > '*device-is-node': 'bool', 'password': 'str'} } > > That would also work; the naming is a bit more awkward, but then you > don't have the issue of mutually-exclusive optional arguments where > exactly one of the two arguments is required. Yes, and I dislike that very much. Btw, can anyone remind me why we can't have new commands instead? > I'm leaning slightly towards the approach that Benoît took, if only for > the naming aspect (that is, I also thought of the idea of a bool flag, > but didn't suggest it because I didn't like the implications on the > naming). But I can live with either approach, if anyone else has a > strong opinion. Well, we can pick up any descriptive name 'treat-device-as-a-node', 'device-is-a-graph-node'...
Le Friday 06 Dec 2013 à 11:52:15 (-0500), Luiz Capitulino a écrit : > On Fri, 06 Dec 2013 08:24:33 -0700 > Eric Blake <eblake@redhat.com> wrote: > > > On 12/06/2013 07:27 AM, Luiz Capitulino wrote: > > > On Thu, 5 Dec 2013 18:15:00 +0100 > > > Benoît Canet <benoit@irqsave.net> wrote: > > > > >> -{ 'command': 'block_passwd', 'data': {'device': 'str', 'password': 'str'} } > > >> +{ 'command': 'block_passwd', 'data': {'*device': 'str', > > >> + '*node-name': 'str', 'password': 'str'} } > > > > > > What about: > > > > > > { 'command': 'block_passwd', 'data': {'device': 'str', > > > '*device-is-node': 'bool', 'password': 'str'} } > > > > That would also work; the naming is a bit more awkward, but then you > > don't have the issue of mutually-exclusive optional arguments where > > exactly one of the two arguments is required. > > Yes, and I dislike that very much. Luiz, I will rewrite these patch using your boolean. Best regards Benoît > > Btw, can anyone remind me why we can't have new commands instead? That would mean doubling the whole QMP block command set once the work is done. > > > I'm leaning slightly towards the approach that Benoît took, if only for > > the naming aspect (that is, I also thought of the idea of a bool flag, > > but didn't suggest it because I didn't like the implications on the > > naming). But I can live with either approach, if anyone else has a > > strong opinion. > > Well, we can pick up any descriptive name 'treat-device-as-a-node', > 'device-is-a-graph-node'... >
Am 06.12.2013 um 17:52 hat Luiz Capitulino geschrieben: > On Fri, 06 Dec 2013 08:24:33 -0700 > Eric Blake <eblake@redhat.com> wrote: > > > On 12/06/2013 07:27 AM, Luiz Capitulino wrote: > > > On Thu, 5 Dec 2013 18:15:00 +0100 > > > Benoît Canet <benoit@irqsave.net> wrote: > > > > >> -{ 'command': 'block_passwd', 'data': {'device': 'str', 'password': 'str'} } > > >> +{ 'command': 'block_passwd', 'data': {'*device': 'str', > > >> + '*node-name': 'str', 'password': 'str'} } > > > > > > What about: > > > > > > { 'command': 'block_passwd', 'data': {'device': 'str', > > > '*device-is-node': 'bool', 'password': 'str'} } > > > > That would also work; the naming is a bit more awkward, but then you > > don't have the issue of mutually-exclusive optional arguments where > > exactly one of the two arguments is required. > > Yes, and I dislike that very much. > > Btw, can anyone remind me why we can't have new commands instead? Because having to maintain two commands for the same functionality is bad. > > I'm leaning slightly towards the approach that Benoît took, if only for > > the naming aspect (that is, I also thought of the idea of a bool flag, > > but didn't suggest it because I didn't like the implications on the > > naming). But I can live with either approach, if anyone else has a > > strong opinion. > > Well, we can pick up any descriptive name 'treat-device-as-a-node', > 'device-is-a-graph-node'... All devices are represented by nodes, so that doesn't make sense. If anything, 'interpret-device-name-as-node-name', which at the same time makes it pretty clear that we're abusing a field for something it wasn't meant for. Kevin
On Mon, 9 Dec 2013 17:23:09 +0100 Kevin Wolf <kwolf@redhat.com> wrote: > > > I'm leaning slightly towards the approach that Benoît took, if only for > > > the naming aspect (that is, I also thought of the idea of a bool flag, > > > but didn't suggest it because I didn't like the implications on the > > > naming). But I can live with either approach, if anyone else has a > > > strong opinion. > > > > Well, we can pick up any descriptive name 'treat-device-as-a-node', > > 'device-is-a-graph-node'... > > All devices are represented by nodes, so that doesn't make sense. > If anything, 'interpret-device-name-as-node-name', which at the same > time makes it pretty clear that we're abusing a field for something it > wasn't meant for. Having two optionals where they can't be specified at the same time and can't be left off at the same time is a clear abuse as well. The truth is, both proposals are bad. This makes me think that maybe we should introduce a block API 2.0 and deprecate the current one (partly or completely).
Le Monday 09 Dec 2013 à 11:41:09 (-0500), Luiz Capitulino a écrit : > On Mon, 9 Dec 2013 17:23:09 +0100 > Kevin Wolf <kwolf@redhat.com> wrote: > > > > > I'm leaning slightly towards the approach that Benoît took, if only for > > > > the naming aspect (that is, I also thought of the idea of a bool flag, > > > > but didn't suggest it because I didn't like the implications on the > > > > naming). But I can live with either approach, if anyone else has a > > > > strong opinion. > > > > > > Well, we can pick up any descriptive name 'treat-device-as-a-node', > > > 'device-is-a-graph-node'... > > > > All devices are represented by nodes, so that doesn't make sense. > > If anything, 'interpret-device-name-as-node-name', which at the same > > time makes it pretty clear that we're abusing a field for something it > > wasn't meant for. > > Having two optionals where they can't be specified at the same time > and can't be left off at the same time is a clear abuse as well. > > The truth is, both proposals are bad. This makes me think that maybe > we should introduce a block API 2.0 and deprecate the current one > (partly or completely). > It took me one year to go from the block filters and block backend requirement to the state where my customer allows me to work on block filters. Now if we add to this the new requirement of block API 2.0 I think I will soon have time to concentrate myself on non qemu projects :( Best regards Benoît
On Mon, 9 Dec 2013 17:48:50 +0100 Benoît Canet <benoit.canet@irqsave.net> wrote: > Le Monday 09 Dec 2013 à 11:41:09 (-0500), Luiz Capitulino a écrit : > > On Mon, 9 Dec 2013 17:23:09 +0100 > > Kevin Wolf <kwolf@redhat.com> wrote: > > > > > > > I'm leaning slightly towards the approach that Benoît took, if only for > > > > > the naming aspect (that is, I also thought of the idea of a bool flag, > > > > > but didn't suggest it because I didn't like the implications on the > > > > > naming). But I can live with either approach, if anyone else has a > > > > > strong opinion. > > > > > > > > Well, we can pick up any descriptive name 'treat-device-as-a-node', > > > > 'device-is-a-graph-node'... > > > > > > All devices are represented by nodes, so that doesn't make sense. > > > If anything, 'interpret-device-name-as-node-name', which at the same > > > time makes it pretty clear that we're abusing a field for something it > > > wasn't meant for. > > > > Having two optionals where they can't be specified at the same time > > and can't be left off at the same time is a clear abuse as well. > > > > The truth is, both proposals are bad. This makes me think that maybe > > we should introduce a block API 2.0 and deprecate the current one > > (partly or completely). > > > > It took me one year to go from the block filters and block backend > requirement to the state where my customer allows me to work on block filters. > > Now if we add to this the new requirement of block API 2.0 I think I will soon > have time to concentrate myself on non qemu projects :( I don't think it would be something major as far as code is concerned. What can take a lot of time and energy is to define the API. The QMP commands implementation would probably be a wrapper around a single (or a set of) block functions. Again, I can live with what I suggested because I find it simpler than your original proposal: no existing field is changed, only one field is added, and clients can happily omit it if they don't know what it's about.
Le Monday 09 Dec 2013 à 12:03:26 (-0500), Luiz Capitulino a écrit : > On Mon, 9 Dec 2013 17:48:50 +0100 > Benoît Canet <benoit.canet@irqsave.net> wrote: > > > Le Monday 09 Dec 2013 à 11:41:09 (-0500), Luiz Capitulino a écrit : > > > On Mon, 9 Dec 2013 17:23:09 +0100 > > > Kevin Wolf <kwolf@redhat.com> wrote: > > > > > > > > > I'm leaning slightly towards the approach that Benoît took, if only for > > > > > > the naming aspect (that is, I also thought of the idea of a bool flag, > > > > > > but didn't suggest it because I didn't like the implications on the > > > > > > naming). But I can live with either approach, if anyone else has a > > > > > > strong opinion. > > > > > > > > > > Well, we can pick up any descriptive name 'treat-device-as-a-node', > > > > > 'device-is-a-graph-node'... > > > > > > > > All devices are represented by nodes, so that doesn't make sense. > > > > If anything, 'interpret-device-name-as-node-name', which at the same > > > > time makes it pretty clear that we're abusing a field for something it > > > > wasn't meant for. > > > > > > Having two optionals where they can't be specified at the same time > > > and can't be left off at the same time is a clear abuse as well. > > > > > > The truth is, both proposals are bad. This makes me think that maybe > > > we should introduce a block API 2.0 and deprecate the current one > > > (partly or completely). > > > > > > > It took me one year to go from the block filters and block backend > > requirement to the state where my customer allows me to work on block filters. > > > > Now if we add to this the new requirement of block API 2.0 I think I will soon > > have time to concentrate myself on non qemu projects :( > > I don't think it would be something major as far as code is concerned. > What can take a lot of time and energy is to define the API. The QMP > commands implementation would probably be a wrapper around a single (or > a set of) block functions. > > Again, I can live with what I suggested because I find it simpler > than your original proposal: no existing field is changed, only one > field is added, and clients can happily omit it if they don't know > what it's about. > I already have rewritten the patches to support your version of the commands. I will let you people decide which version qemu will merge. Best regards Benoît
Am 09.12.2013 um 17:41 hat Luiz Capitulino geschrieben: > On Mon, 9 Dec 2013 17:23:09 +0100 > Kevin Wolf <kwolf@redhat.com> wrote: > > > > > I'm leaning slightly towards the approach that Benoît took, if only for > > > > the naming aspect (that is, I also thought of the idea of a bool flag, > > > > but didn't suggest it because I didn't like the implications on the > > > > naming). But I can live with either approach, if anyone else has a > > > > strong opinion. > > > > > > Well, we can pick up any descriptive name 'treat-device-as-a-node', > > > 'device-is-a-graph-node'... > > > > All devices are represented by nodes, so that doesn't make sense. > > If anything, 'interpret-device-name-as-node-name', which at the same > > time makes it pretty clear that we're abusing a field for something it > > wasn't meant for. > > Having two optionals where they can't be specified at the same time > and can't be left off at the same time is a clear abuse as well. Is it? If you wanted to express this in the schema, we'd need to extend the QAPI generator, but until now we never have. I don't think this is the first time that optional fields are not completely independent, but may be required/forbidden based on values of other fields. Documenting it should be enough, in my opinion. > The truth is, both proposals are bad. This makes me think that maybe > we should introduce a block API 2.0 and deprecate the current one > (partly or completely). Nice try, but of course this is equivalent to the "new command" solution. Deprecating the old version doesn't get you rid of it, you still need to support it for compatibility. And then you're back to square one. For what it's worth, I think what Benoît implemented is the outcome of discussions of the Block BOF on KVM Forum that included both block layer developers and API users (i.e. libvirt), after considering and dismissing other options (which, of course, included separate commands). Kevin
On Tue, 10 Dec 2013 10:57:50 +0100 Kevin Wolf <kwolf@redhat.com> wrote: > Am 09.12.2013 um 17:41 hat Luiz Capitulino geschrieben: > > On Mon, 9 Dec 2013 17:23:09 +0100 > > Kevin Wolf <kwolf@redhat.com> wrote: > > > > > > > I'm leaning slightly towards the approach that Benoît took, if only for > > > > > the naming aspect (that is, I also thought of the idea of a bool flag, > > > > > but didn't suggest it because I didn't like the implications on the > > > > > naming). But I can live with either approach, if anyone else has a > > > > > strong opinion. > > > > > > > > Well, we can pick up any descriptive name 'treat-device-as-a-node', > > > > 'device-is-a-graph-node'... > > > > > > All devices are represented by nodes, so that doesn't make sense. > > > If anything, 'interpret-device-name-as-node-name', which at the same > > > time makes it pretty clear that we're abusing a field for something it > > > wasn't meant for. > > > > Having two optionals where they can't be specified at the same time > > and can't be left off at the same time is a clear abuse as well. > > Is it? If you wanted to express this in the schema, we'd need to extend > the QAPI generator, but until now we never have. I don't think this is > the first time that optional fields are not completely independent, but > may be required/forbidden based on values of other fields. Documenting > it should be enough, in my opinion. We disagree here, and what makes my objection strong is that I provided an alternative which I believe is less worse because it makes less changes to the command. > > The truth is, both proposals are bad. This makes me think that maybe > > we should introduce a block API 2.0 and deprecate the current one > > (partly or completely). > > Nice try, but of course this is equivalent to the "new command" > solution. Deprecating the old version doesn't get you rid of it, you > still need to support it for compatibility. And then you're back to > square one. We can't get rid of anything in QMP. Deprecating means that the command is still available but a better replacement exists and should be used in new implementations. > For what it's worth, I think what Benoît implemented is the outcome of > discussions of the Block BOF on KVM Forum that included both block layer > developers and API users (i.e. libvirt), after considering and > dismissing other options (which, of course, included separate commands). I appreciate those discussions, but patch review and acceptance happens on upstream lists.
Am 10.12.2013 um 15:06 hat Luiz Capitulino geschrieben: > On Tue, 10 Dec 2013 10:57:50 +0100 > Kevin Wolf <kwolf@redhat.com> wrote: > > > Am 09.12.2013 um 17:41 hat Luiz Capitulino geschrieben: > > > On Mon, 9 Dec 2013 17:23:09 +0100 > > > Kevin Wolf <kwolf@redhat.com> wrote: > > > > > > > > > I'm leaning slightly towards the approach that Benoît took, if only for > > > > > > the naming aspect (that is, I also thought of the idea of a bool flag, > > > > > > but didn't suggest it because I didn't like the implications on the > > > > > > naming). But I can live with either approach, if anyone else has a > > > > > > strong opinion. > > > > > > > > > > Well, we can pick up any descriptive name 'treat-device-as-a-node', > > > > > 'device-is-a-graph-node'... > > > > > > > > All devices are represented by nodes, so that doesn't make sense. > > > > If anything, 'interpret-device-name-as-node-name', which at the same > > > > time makes it pretty clear that we're abusing a field for something it > > > > wasn't meant for. > > > > > > Having two optionals where they can't be specified at the same time > > > and can't be left off at the same time is a clear abuse as well. > > > > Is it? If you wanted to express this in the schema, we'd need to extend > > the QAPI generator, but until now we never have. I don't think this is > > the first time that optional fields are not completely independent, but > > may be required/forbidden based on values of other fields. Documenting > > it should be enough, in my opinion. > > We disagree here, and what makes my objection strong is that I > provided an alternative which I believe is less worse because it > makes less changes to the command. My objection to your approach is strong because Benoît already sent an alternative which I believe is less worse because with it, arguments actually mean what their names tell instead of having additional bools for "oh, and I said A, but I didn't mean it, I really want B". > > > The truth is, both proposals are bad. This makes me think that maybe > > > we should introduce a block API 2.0 and deprecate the current one > > > (partly or completely). > > > > Nice try, but of course this is equivalent to the "new command" > > solution. Deprecating the old version doesn't get you rid of it, you > > still need to support it for compatibility. And then you're back to > > square one. > > We can't get rid of anything in QMP. Deprecating means that the command > is still available but a better replacement exists and should be used > in new implementations. At least one thing on which we agree. However, suggesting to deprecate the old version would mean that we think that referencing by device name is a bad thing that you shouldn't be doing any more. I don't think it's true. > > For what it's worth, I think what Benoît implemented is the outcome of > > discussions of the Block BOF on KVM Forum that included both block layer > > developers and API users (i.e. libvirt), after considering and > > dismissing other options (which, of course, included separate commands). > > I appreciate those discussions, but patch review and acceptance happens > on upstream lists. Okay. So you're going to block an interface that block layer developers and block layer users have agreed on because it doesn't match your aesthetic demands - and you wrote some parts of the infrastructure we're using, so you have an inherent right to have them met? I love productive discussions like this. Kevin
On Tue, 10 Dec 2013 15:25:07 +0100 Kevin Wolf <kwolf@redhat.com> wrote: > My objection to your approach is strong because Benoît already sent an > alternative which I believe is less worse because with it, arguments > actually mean what their names tell instead of having additional bools > for "oh, and I said A, but I didn't mean it, I really want B". Current proposal: { 'command': 'block_passwd', 'data': {'*device': 'str', '*node-name': 'str', 'password': 'str'} } When I look at it, I ask myself: - What happens when device=NULL? - What happens when node-name=NULL? - What happens when device=NULL and node-name=NULL? - What happens when device != NULL and node-node != NULL? - What happens when device != NULL but node-node=NULL? - What happens when device=NULL but node-node != NULL? My proposal: { 'command': 'block_passwd', 'data': {'device': 'str', '*device-is-node': 'bool', 'password': 'str'} } - What happens when device-is-node=NULL? - What happens when device-is-node != NULL? PS: I'm not NACKing anything. My review to this series started with a "what about" question. I did voice my objections against this proposal, but didn't NACK it. Besides you're a maintainer as much as I am, so I could NACK this as much as you could push this through your tree ignoring review.
Am 10.12.2013 um 16:16 hat Luiz Capitulino geschrieben: > On Tue, 10 Dec 2013 15:25:07 +0100 > Kevin Wolf <kwolf@redhat.com> wrote: > > > My objection to your approach is strong because Benoît already sent an > > alternative which I believe is less worse because with it, arguments > > actually mean what their names tell instead of having additional bools > > for "oh, and I said A, but I didn't mean it, I really want B". > > Current proposal: > > { 'command': 'block_passwd', 'data': {'*device': 'str', > '*node-name': 'str', 'password': 'str'} } > > When I look at it, I ask myself: > > - What happens when device=NULL? > > - What happens when node-name=NULL? > > - What happens when device=NULL and node-name=NULL? > > - What happens when device != NULL and node-node != NULL? > > - What happens when device != NULL but node-node=NULL? > > - What happens when device=NULL but node-node != NULL? Looks pretty long, but the latter four are just the combination of the former two. I think it's relevant in what context you are looking at it. As a user, the question I'm really asking is: Hm, okay, passing just a password can't be enough, so... - ...do I need to specificy device, and if so, with which value? - ...do I need to specificy node-name, and if so, with which value? Looking at the documentation comment would make it clear rather quickly that I should use only one of them and what the right value is. For the implementation of the command, I actually need to think about all the corner cases. However, even there a comment "device and node-name may never be specified at the same time" makes it pretty clear what I'm supposed to implement. For debugging, suppose I read a QMP command in a log file, I read either "device" or "node-name" and I know exactly what is meant. > My proposal: > > { 'command': 'block_passwd', 'data': {'device': 'str', > '*device-is-node': 'bool', 'password': 'str'} } > > - What happens when device-is-node=NULL? > > - What happens when device-is-node != NULL? Okay, as a user, I see that need to pass a device name and a password, fine. Hm, I really wanted to pass a node name instead... Okay, a second look shows me the optional flag that changes the meaning of "device", so I use that, fine. Implementing the functionality in qemu, this can't be hard. I just need to check the flag when I search the BlockDriverState so that I search by node-name or by device name, depending on what is requested. Hopefully nobody will even use the 'device' parameter outside my if block because it's overloaded with different meanings, so they would introduce a bug. Debugging a QMP command in a log file, I see the 'device' key and am happy because now I know the name of the device that caused trouble. Oh, there was the "I didn't really mean 'device'" flag set? Whoops... So perhaps in some cases you have to ask yourself more questions with the separate fields, but the "whoops" cases of misunderstanding the overloaded field will mean that people might not even think of asking a question, but rather just do the wrong thing. > PS: I'm not NACKing anything. My review to this series started with a > "what about" question. I did voice my objections against this > proposal, but didn't NACK it. Besides you're a maintainer as much > as I am, so I could NACK this as much as you could push this > through your tree ignoring review. I hope we agree that edit wars aren't where we're going to go. Kevin
On 12/10/2013 08:16 AM, Luiz Capitulino wrote: > On Tue, 10 Dec 2013 15:25:07 +0100 > Kevin Wolf <kwolf@redhat.com> wrote: > >> My objection to your approach is strong because Benoît already sent an >> alternative which I believe is less worse because with it, arguments >> actually mean what their names tell instead of having additional bools >> for "oh, and I said A, but I didn't mean it, I really want B". > > Current proposal: > > { 'command': 'block_passwd', 'data': {'*device': 'str', > '*node-name': 'str', 'password': 'str'} } > > When I look at it, I ask myself: > > - What happens when device=NULL? Then, per docs, node-name must be non-NULL. > > - What happens when node-name=NULL? Then, per docs, device must be non-NULL. > > - What happens when device=NULL and node-name=NULL? Error; violates docs. > > - What happens when device != NULL and node-node != NULL? Error; violates docs. > > - What happens when device != NULL but node-node=NULL? Operate on the device. > > - What happens when device=NULL but node-node != NULL? Operate on the node-name. > > My proposal: > > { 'command': 'block_passwd', 'data': {'device': 'str', > '*device-is-node': 'bool', 'password': 'str'} } > > - What happens when device-is-node=NULL? Operate on the device (same as if device-is-node were false) > > - What happens when device-is-node != NULL? The state of the bool determines whether we are operating on device or on node. > > PS: I'm not NACKing anything. My review to this series started with a > "what about" question. I did voice my objections against this > proposal, but didn't NACK it. Besides you're a maintainer as much > as I am, so I could NACK this as much as you could push this > through your tree ignoring review. And I appreciate the critical review - if we capture design decisions like this in the commit message, then a year from now when someone asks "why didn't you do it this alternative way" we can say "look at the commit message which explores the tradeoffs, and why we settled for what we did" rather than saying "oops, we painted ourselves into a corner because we didn't think about that". The more this goes on, the more I like the mutually-exclusive device[str]/node-name[str] arguments, and the less I like the device[str]/device-is-node[bool] solution, but I can still live with either approach from libvirt's point of view.
On 2013年12月10日 23:16, Luiz Capitulino wrote: > On Tue, 10 Dec 2013 15:25:07 +0100 > Kevin Wolf <kwolf@redhat.com> wrote: > >> My objection to your approach is strong because Benoît already sent an >> alternative which I believe is less worse because with it, arguments >> actually mean what their names tell instead of having additional bools >> for "oh, and I said A, but I didn't mean it, I really want B". > > Current proposal: > > { 'command': 'block_passwd', 'data': {'*device': 'str', > '*node-name': 'str', 'password': 'str'} } > I vote for this. > When I look at it, I ask myself: > > - What happens when device=NULL? > > - What happens when node-name=NULL? > > - What happens when device=NULL and node-name=NULL? > > - What happens when device != NULL and node-node != NULL? > > - What happens when device != NULL but node-node=NULL? > > - What happens when device=NULL but node-node != NULL? > > My proposal: > > { 'command': 'block_passwd', 'data': {'device': 'str', > '*device-is-node': 'bool', 'password': 'str'} } > > - What happens when device-is-node=NULL? > > - What happens when device-is-node != NULL? > I think our starting point is that device names and node names are separate name spaces. For this reason we should avoid connecting them by reusing one field. From a user's view, "device-is-node", or anything meaning this, just doesn't sound interesting, because we also told them "device-is-not-node". Fam
On Wed, 11 Dec 2013 11:52:28 +0800 Fam Zheng <famz@redhat.com> wrote: > On 2013年12月10日 23:16, Luiz Capitulino wrote: > > On Tue, 10 Dec 2013 15:25:07 +0100 > > Kevin Wolf <kwolf@redhat.com> wrote: > > > >> My objection to your approach is strong because Benoît already sent an > >> alternative which I believe is less worse because with it, arguments > >> actually mean what their names tell instead of having additional bools > >> for "oh, and I said A, but I didn't mean it, I really want B". > > > > Current proposal: > > > > { 'command': 'block_passwd', 'data': {'*device': 'str', > > '*node-name': 'str', 'password': 'str'} } > > > > I vote for this. Ok. As it's clear that I've failed to demonstrate how I think we're going to move all those commands into the wrong direction, I think it's time to withdrawn my suggestion.
diff --git a/block.c b/block.c index 372aa3b..e53f24f 100644 --- a/block.c +++ b/block.c @@ -3194,6 +3194,38 @@ strList *bdrv_named_nodes_list(void) return str_list; } +BlockDriverState *bdrv_lookup_bs(bool has_device, const char *device, + bool has_node_name, const char *node_name, + Error **errp) +{ + BlockDriverState *bs = NULL; + + if (has_device == has_node_name) { + error_setg(errp, "Use either device or node-name but not both"); + return NULL; + } + + if (has_device) { + bs = bdrv_find(device); + + if (!bs) { + error_set(errp, QERR_DEVICE_NOT_FOUND, device); + return NULL; + } + + return bs; + } + + bs = bdrv_find_node(node_name); + + if (!bs) { + error_set(errp, QERR_DEVICE_NOT_FOUND, node_name); + return NULL; + } + + return bs; +} + BlockDriverState *bdrv_next(BlockDriverState *bs) { if (!bs) { diff --git a/blockdev.c b/blockdev.c index 1eb4639..5abf303 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1474,14 +1474,19 @@ void qmp_eject(const char *device, bool has_force, bool force, Error **errp) eject_device(bs, force, errp); } -void qmp_block_passwd(const char *device, const char *password, Error **errp) +void qmp_block_passwd(bool has_device, const char *device, + bool has_node_name, const char *node_name, + const char *password, Error **errp) { + Error *local_err = NULL; BlockDriverState *bs; int err; - bs = bdrv_find(device); - if (!bs) { - error_set(errp, QERR_DEVICE_NOT_FOUND, device); + bs = bdrv_lookup_bs(has_device, device, + has_node_name, node_name, + &local_err); + if (error_is_set(&local_err)) { + error_propagate(errp, local_err); return; } diff --git a/hmp.c b/hmp.c index 32ee285..3820fbe 100644 --- a/hmp.c +++ b/hmp.c @@ -870,7 +870,7 @@ void hmp_block_passwd(Monitor *mon, const QDict *qdict) const char *password = qdict_get_str(qdict, "password"); Error *errp = NULL; - qmp_block_passwd(device, password, &errp); + qmp_block_passwd(true, device, false, NULL, password, &errp); hmp_handle_error(mon, &errp); } diff --git a/include/block/block.h b/include/block/block.h index fe642d9..49dab83 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -372,6 +372,9 @@ const char *bdrv_get_format_name(BlockDriverState *bs); BlockDriverState *bdrv_find(const char *name); BlockDriverState *bdrv_find_node(const char *node_name); strList *bdrv_named_nodes_list(void); +BlockDriverState *bdrv_lookup_bs(bool has_device, const char *device, + bool has_node_name, const char *node_name, + Error **errp); BlockDriverState *bdrv_next(BlockDriverState *bs); void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs), void *opaque); diff --git a/qapi-schema.json b/qapi-schema.json index 3a4a4c8..ddd0dbd 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1675,7 +1675,11 @@ # determine which ones are encrypted, set the passwords with this command, and # then start the guest with the @cont command. # -# @device: the name of the device to set the password on +# Either @device or @node-name must be set but not both. +# +# @device: #optional the name of the block backend device to set the password on +# +# @node-name: #optional graph node name to set the password on (Since 2.0) # # @password: the password to use for the device # @@ -1689,7 +1693,8 @@ # # Since: 0.14.0 ## -{ 'command': 'block_passwd', 'data': {'device': 'str', 'password': 'str'} } +{ 'command': 'block_passwd', 'data': {'*device': 'str', + '*node-name': 'str', 'password': 'str'} } ## # @balloon: diff --git a/qmp-commands.hx b/qmp-commands.hx index 107795b..19a7eb2 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1452,7 +1452,7 @@ EQMP { .name = "block_passwd", - .args_type = "device:B,password:s", + .args_type = "device:s?,node-name:s?,password:s", .mhandler.cmd_new = qmp_marshal_input_block_passwd, }, @@ -1465,6 +1465,7 @@ Set the password of encrypted block devices. Arguments: - "device": device name (json-string) +- "node-name": name in the block driver state graph (json-string) - "password": password (json-string) Example:
Signed-off-by: Benoit Canet <benoit@irqsave.net> --- block.c | 32 ++++++++++++++++++++++++++++++++ blockdev.c | 13 +++++++++---- hmp.c | 2 +- include/block/block.h | 3 +++ qapi-schema.json | 9 +++++++-- qmp-commands.hx | 3 ++- 6 files changed, 54 insertions(+), 8 deletions(-)