| Message ID | b16c15112e3380cac0a63495440e4f7f8bd8a82d.1397568339.git.jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 15.04.2014 15:28, Jeff Cody wrote: > Some block drivers have multiple BlockDriver instances with identical > format_name fields (e.g. gluster, nbd). In those cases, the > protocol_name is usually the more unique identifier (e.g. gluster+tcp). > > Both qemu-img and qemu will use bdrv_iterate_format() to list the > supported formats when a help option is invoked. When just the > format_name is used, redundant listings of formats occur (e.g., > "Supported formats: ... gluster gluster gluster gluster ... "). > > If we prefer the protocol_name over the format_name (when the > protocol name exists), then that provides a more informative > help message: > > "Supported formats: ... gluster gluster+tcp gluster+unix > gluster+rdma ... " > > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > block.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) Reviewed-by: Max Reitz <mreitz@redhat.com>
Am 15.04.2014 um 15:28 hat Jeff Cody geschrieben: > Some block drivers have multiple BlockDriver instances with identical > format_name fields (e.g. gluster, nbd). In those cases, the > protocol_name is usually the more unique identifier (e.g. gluster+tcp). > > Both qemu-img and qemu will use bdrv_iterate_format() to list the > supported formats when a help option is invoked. When just the > format_name is used, redundant listings of formats occur (e.g., > "Supported formats: ... gluster gluster gluster gluster ... "). > > If we prefer the protocol_name over the format_name (when the > protocol name exists), then that provides a more informative > help message: > > "Supported formats: ... gluster gluster+tcp gluster+unix > gluster+rdma ... " > > Signed-off-by: Jeff Cody <jcody@redhat.com> On the other hand, it means that you can't take any driver name from here as use it as -drive driver=... value any more. The good thing is that most drivers stay in the list, so if anyone was checking the list to query whether a given driver can be used, it would still work after this patch. gluster/nbd/sheepdog all have a BlockDriver with a plain gluster/nbd/sheepdog protocol_name. The one driver that may cause trouble is vvfat, which would be changed to fat in the output. Not sure if we care. Let's wait a bit for more comments on this change before we apply it. I'm not against it per se, but it's not obviously good either. Kevin
On Tue, Apr 15, 2014 at 04:00:54PM +0200, Kevin Wolf wrote: > Am 15.04.2014 um 15:28 hat Jeff Cody geschrieben: > > Some block drivers have multiple BlockDriver instances with identical > > format_name fields (e.g. gluster, nbd). In those cases, the > > protocol_name is usually the more unique identifier (e.g. gluster+tcp). > > > > Both qemu-img and qemu will use bdrv_iterate_format() to list the > > supported formats when a help option is invoked. When just the > > format_name is used, redundant listings of formats occur (e.g., > > "Supported formats: ... gluster gluster gluster gluster ... "). > > > > If we prefer the protocol_name over the format_name (when the > > protocol name exists), then that provides a more informative > > help message: > > > > "Supported formats: ... gluster gluster+tcp gluster+unix > > gluster+rdma ... " > > > > Signed-off-by: Jeff Cody <jcody@redhat.com> > > On the other hand, it means that you can't take any driver name from > here as use it as -drive driver=... value any more. I think this change is problematic because of this. It doesn't make sense that "Supported formats" lists names that actually bdrv_find_format() is unable to find. blockdev_init()'s format= code will break if we make this change. How about we change bdrv_iterate_format() to process unique format names only? It doesn't make sense to process duplicate format names since the callback function has no way of identifying the specific BlockDriver from just the duplicate format name argument. Stefan
On Thu, Apr 24, 2014 at 02:34:19PM +0200, Stefan Hajnoczi wrote: > On Tue, Apr 15, 2014 at 04:00:54PM +0200, Kevin Wolf wrote: > > Am 15.04.2014 um 15:28 hat Jeff Cody geschrieben: > > > Some block drivers have multiple BlockDriver instances with identical > > > format_name fields (e.g. gluster, nbd). In those cases, the > > > protocol_name is usually the more unique identifier (e.g. gluster+tcp). > > > > > > Both qemu-img and qemu will use bdrv_iterate_format() to list the > > > supported formats when a help option is invoked. When just the > > > format_name is used, redundant listings of formats occur (e.g., > > > "Supported formats: ... gluster gluster gluster gluster ... "). > > > > > > If we prefer the protocol_name over the format_name (when the > > > protocol name exists), then that provides a more informative > > > help message: > > > > > > "Supported formats: ... gluster gluster+tcp gluster+unix > > > gluster+rdma ... " > > > > > > Signed-off-by: Jeff Cody <jcody@redhat.com> > > > > On the other hand, it means that you can't take any driver name from > > here as use it as -drive driver=... value any more. > > I think this change is problematic because of this. It doesn't make > sense that "Supported formats" lists names that actually > bdrv_find_format() is unable to find. > > blockdev_init()'s format= code will break if we make this change. > > How about we change bdrv_iterate_format() to process unique format names > only? It doesn't make sense to process duplicate format names since the > callback function has no way of identifying the specific BlockDriver > from just the duplicate format name argument. > Yes, given the concern that blockdev_init()'s help output would be misleading with this change, I think that is probably the best method to go about it.
diff --git a/block.c b/block.c index 990a754..0587257 100644 --- a/block.c +++ b/block.c @@ -3576,7 +3576,13 @@ void bdrv_iterate_format(void (*it)(void *opaque, const char *name), BlockDriver *drv; QLIST_FOREACH(drv, &bdrv_drivers, list) { - it(opaque, drv->format_name); + /* format_name may be duplicate, if multiple protocols exist for a + * format. If there is a protocol, use that more unique name instead */ + if (drv->protocol_name) { + it(opaque, drv->protocol_name); + } else { + it(opaque, drv->format_name); + } } }
Some block drivers have multiple BlockDriver instances with identical format_name fields (e.g. gluster, nbd). In those cases, the protocol_name is usually the more unique identifier (e.g. gluster+tcp). Both qemu-img and qemu will use bdrv_iterate_format() to list the supported formats when a help option is invoked. When just the format_name is used, redundant listings of formats occur (e.g., "Supported formats: ... gluster gluster gluster gluster ... "). If we prefer the protocol_name over the format_name (when the protocol name exists), then that provides a more informative help message: "Supported formats: ... gluster gluster+tcp gluster+unix gluster+rdma ... " Signed-off-by: Jeff Cody <jcody@redhat.com> --- block.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)