| Message ID | 1468502894-18098-12-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. > > Note that there is a semantic difference between using a BlockBackend > name and the node name of its root: In the former case, the NBD server > is closed on eject; in the latter case, the NBD server doesn't drop its > reference and keeps the image file open this way. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > blockdev-nbd.c | 21 +++++++++------------ > qapi/block.json | 4 ++-- > 2 files changed, 11 insertions(+), 14 deletions(-) > > diff --git a/blockdev-nbd.c b/blockdev-nbd.c > index c437d32..ca41cc6 100644 > --- a/blockdev-nbd.c > +++ b/blockdev-nbd.c > @@ -145,7 +145,8 @@ void qmp_nbd_server_start(SocketAddress *addr, > void qmp_nbd_server_add(const char *device, bool has_writable, bool writable, > Error **errp) > { > - BlockBackend *blk; > + BlockDriverState *bs = NULL; > + BlockBackend *on_eject_blk; > NBDExport *exp; > > if (!nbd_server) { > @@ -158,26 +159,22 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable, > return; 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?
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. >> >> Note that there is a semantic difference between using a BlockBackend >> name and the node name of its root: In the former case, the NBD server >> is closed on eject; in the latter case, the NBD server doesn't drop its >> reference and keeps the image file open this way. >> >> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >> --- >> blockdev-nbd.c | 21 +++++++++------------ >> qapi/block.json | 4 ++-- >> 2 files changed, 11 insertions(+), 14 deletions(-) >> >> diff --git a/blockdev-nbd.c b/blockdev-nbd.c >> index c437d32..ca41cc6 100644 >> --- a/blockdev-nbd.c >> +++ b/blockdev-nbd.c >> @@ -145,7 +145,8 @@ void qmp_nbd_server_start(SocketAddress *addr, >> void qmp_nbd_server_add(const char *device, bool has_writable, bool writable, >> Error **errp) >> { >> - BlockBackend *blk; >> + BlockDriverState *bs = NULL; >> + BlockBackend *on_eject_blk; >> NBDExport *exp; >> >> if (!nbd_server) { >> @@ -158,26 +159,22 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable, >> return; > > 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. 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, ...) So, yeah, I think we just need the new op-blockers for this, I don't think the current op blockers cover this. Max
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). > > 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. > > 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.
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
diff --git a/blockdev-nbd.c b/blockdev-nbd.c index c437d32..ca41cc6 100644 --- a/blockdev-nbd.c +++ b/blockdev-nbd.c @@ -145,7 +145,8 @@ void qmp_nbd_server_start(SocketAddress *addr, void qmp_nbd_server_add(const char *device, bool has_writable, bool writable, Error **errp) { - BlockBackend *blk; + BlockDriverState *bs = NULL; + BlockBackend *on_eject_blk; NBDExport *exp; if (!nbd_server) { @@ -158,26 +159,22 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable, return; } - blk = blk_by_name(device); - if (!blk) { - error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, - "Device '%s' not found", device); - return; - } - if (!blk_is_inserted(blk)) { - error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, device); + on_eject_blk = blk_by_name(device); + + bs = bdrv_lookup_bs(device, device, errp); + if (!bs) { return; } if (!has_writable) { writable = false; } - if (blk_is_read_only(blk)) { + if (bdrv_is_read_only(bs)) { writable = false; } - exp = nbd_export_new(blk_bs(blk), 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY, - NULL, false, blk, errp); + exp = nbd_export_new(bs, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY, + NULL, false, on_eject_blk, errp); if (!exp) { return; } diff --git a/qapi/block.json b/qapi/block.json index db05169..8b08bd2 100644 --- a/qapi/block.json +++ b/qapi/block.json @@ -161,9 +161,9 @@ ## # @nbd-server-add: # -# Export a device to QEMU's embedded NBD server. +# Export a block node to QEMU's embedded NBD server. # -# @device: Block device to be exported +# @device: The device name or node name of the node to be exported # # @writable: Whether clients should be able to write to the device via the # NBD connection (default false). #optional
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. Note that there is a semantic difference between using a BlockBackend name and the node name of its root: In the former case, the NBD server is closed on eject; in the latter case, the NBD server doesn't drop its reference and keeps the image file open this way. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- blockdev-nbd.c | 21 +++++++++------------ qapi/block.json | 4 ++-- 2 files changed, 11 insertions(+), 14 deletions(-)