Re: [Qemu-devel] [PATCH v4 11/11] nbd-server: Allow node name for nbd-server-add

[Max Reitz]

On 15.07.2016 17:18, Eric Blake wrote:
> On 07/15/2016 07:36 AM, Max Reitz wrote:
>> On 14.07.2016 23:36, Eric Blake wrote:
>>> On 07/14/2016 07:28 AM, Kevin Wolf wrote:
>>>> There is no reason why an NBD server couldn't be started for any node,
>>>> even if it's not on the top level. This converts nbd-server-add to
>>>> accept a node-name.
>>>>
> 
>>> Do we want to do any sanity checking that writing should only be
>>> permitted on a root, and that when using a node name that is not a root
>>> that writable must be false so as not to negatively change the BDS out
>>> of under the feet of the other root?  Do op-blockers already cover that?
>>
>> Well, one could argue that it's possible to create an NBD server on a
>> non-root node today anyway, since creating BBs is not restricted to root
>> nodes:
>>
>> blockdev-add(id=foo, other arguments...)
>> blockdev-add(id=bar, backing=foo, other arguments...)
>>
>> And then you can create an NBD server on bar. I agree that this is not
>> how it should be, though. However, I think that the fact that you need
>> to specify a BB name for now deters people from doing stuff like that.
>> If you can specify a node name, people will think it's completely fine
>> to do so.
> 
> Creating a server on bar doesn't change the contents of foo, so I see
> that as safe (foo can still be in use by other chains, and the server on
> bar won't invalidate those chains).

Errr, I meant foo instead of bar. Curse me for using placeholder names.

>> Also note that only allowing NBD servers to be created on a root node
>> doesn't really help you:
>>
>> blockdev-add(node-name=foo, ...)
>> nbd-server-add(device=foo)
>> blockdev-add(id=bar, backing=foo, ...)
> 
> But THAT is indeed unsafe, if the server allows writes, because now the
> contents of bar are at risk of being silently changed by any edits made
> to foo.
> 
> So the real restriction we want is that if foo is owned by a read-write
> BB (the NBD server in this case), then creating another BDS bar that
> uses foo as a backing is undesirable.

Well, it's exactly what the new op blockers should be for.

>> So, yeah, I think we just need the new op-blockers for this, I don't
>> think the current op blockers cover this.
> 
> I'm not sure either, which is why we're discussing it on list to make
> sure we think about the restrictions and their implications.

Considering that Kevin is on PTO as of today, I think the best course of
action would be put the last two patches of this series off until after
2.7 is released.

Max

signature.asc
Description: OpenPGP digital signature