[Qemu-devel] [RFC] block: Is name of BlockBackend deprecated with -blockdev parameter?

[Qemu-devel] [RFC] block: Is name of BlockBackend deprecated with -blockdev parameter?

[Anton Kuchin]

Hello,

I'm trying to switch from -drive parameter to -blockdev + -device and 
having problems. Looks like with this option I have no way to set the 
name of  created BlockBackend, but some QMP and HMP commands are trying 
to find blk with blk_by_name() and fail to locate my device (e.g. 
hmp_commit, qmp_x_bloc_latency_histogram_set ...). Was it intentional 
and BB names are going to be deprecated?

This also seems to be a the case for all block devices hotplugged with 
QMP as they use the same code path.

As far as I understand all named backends are stored in 
monitor_block_backends list, but I can't get what is the point of having 
this list, and why parse_drive() function doesn't call monitor_add_blk() 
like blockdev_init() does with -drive option. Can someone give me a hint 
on this?

I also noticed that some commands fallback to search by qdev_id or 
BDS->node_name,  but at the creation time (both in bdrv_assing_node_name 
and monitor_add_blk) it is already checked that names are unique across 
these namespaces so may be it would be useful to introduce generic 
search function?

Thanks,
Anton

Re: [Qemu-devel] [RFC] block: Is name of BlockBackend deprecated with -blockdev parameter?

[Markus Armbruster]

I figure Kevin knows, but you typoed his e-mail address.  I fixed it for
you.

Anton Kuchin <antonkuchin@yandex-team.ru> writes:

> Hello,
>
> I'm trying to switch from -drive parameter to -blockdev + -device and
> having problems. Looks like with this option I have no way to set the
> name of  created BlockBackend, but some QMP and HMP commands are
> trying to find blk with blk_by_name() and fail to locate my device
> (e.g. hmp_commit, qmp_x_bloc_latency_histogram_set ...). Was it
> intentional and BB names are going to be deprecated?
>
> This also seems to be a the case for all block devices hotplugged with
> QMP as they use the same code path.
>
> As far as I understand all named backends are stored in
> monitor_block_backends list, but I can't get what is the point of
> having this list, and why parse_drive() function doesn't call
> monitor_add_blk() like blockdev_init() does with -drive option. Can
> someone give me a hint on this?
>
> I also noticed that some commands fallback to search by qdev_id or
> BDS->node_name,  but at the creation time (both in
> bdrv_assing_node_name and monitor_add_blk) it is already checked that
> names are unique across these namespaces so may be it would be useful
> to introduce generic search function?
>
> Thanks,
> Anton

Re: [Qemu-devel] [RFC] block: Is name of BlockBackend deprecated with -blockdev parameter?

[Kevin Wolf]

Am 11.12.2018 um 08:28 hat Markus Armbruster geschrieben:
> I figure Kevin knows, but you typoed his e-mail address.  I fixed it for
> you.
> 
> Anton Kuchin <antonkuchin@yandex-team.ru> writes:
> 
> > Hello,
> >
> > I'm trying to switch from -drive parameter to -blockdev + -device and
> > having problems. Looks like with this option I have no way to set the
> > name of  created BlockBackend, but some QMP and HMP commands are
> > trying to find blk with blk_by_name() and fail to locate my device
> > (e.g. hmp_commit, qmp_x_bloc_latency_histogram_set ...). Was it
> > intentional and BB names are going to be deprecated?
> >
> > This also seems to be a the case for all block devices hotplugged with
> > QMP as they use the same code path.
> >
> > As far as I understand all named backends are stored in
> > monitor_block_backends list, but I can't get what is the point of
> > having this list, and why parse_drive() function doesn't call
> > monitor_add_blk() like blockdev_init() does with -drive option. Can
> > someone give me a hint on this?
> >
> > I also noticed that some commands fallback to search by qdev_id or
> > BDS->node_name,  but at the creation time (both in
> > bdrv_assing_node_name and monitor_add_blk) it is already checked that
> > names are unique across these namespaces so may be it would be useful
> > to introduce generic search function?

Yes, BlockBackend names are not supposed to be used in the external
interfaces any more. If the QMP command is about the backend, it will
take a node name, and if it's about the guest device (which may not even
have a node attached with removable media), it will take a qdev ID.

The implementation of qmp_x_block_latency_histogram_set() is incorrect,
it shouldn't use blk_by_name(). I wonder where it got that pattern from
because it was added long after all other QMP commands had switched to
qmp_get_blk() or bdrv_lookup_bs().

hmp_commit() should indeed use bdrv_lookup_bs() to also accept node
names. I think we reviewed QMP to make sure we converted everything
and forgot about HMP commands that aren't mapped to QMP.

Kevin

Re: [Qemu-devel] [RFC] block: Is name of BlockBackend deprecated with -blockdev parameter?

[Anton Kuchin]

I want to make it consistent but I'm note sure I understand all aspects 
of current state and legacy clients we need to support.

The initial idea was to have single function blk_lookup(char* name) and 
search in all namespaces internally as they are guaranteed to have no 
intersection. This will work a little slower but I see no hot paths that 
will be affected. And this way transition will be transparent to users 
but I'm not sure this can be done in backward compatible way.
Do you have any suggestions how to do it correctly?

On 11/12/2018 12:47, Kevin Wolf wrote:
> [...]
>> Anton Kuchin <antonkuchin@yandex-team.ru> writes:
>> [...]
>>
>> As far as I understand all named backends are stored in
>> monitor_block_backends list, but I can't get what is the point of
>> having this list, and why parse_drive() function doesn't call
>> monitor_add_blk() like blockdev_init() does with -drive option. Can
>> someone give me a hint on this?

And what are monitor_block_backends used for? What does monitor 
reference mean in this context and how backends in this list differ from 
all others?

>>> I also noticed that some commands fallback to search by qdev_id or
>>> BDS->node_name,  but at the creation time (both in
>>> bdrv_assing_node_name and monitor_add_blk) it is already checked that
>>> names are unique across these namespaces so may be it would be useful
>>> to introduce generic search function?
> Yes, BlockBackend names are not supposed to be used in the external
> interfaces any more. If the QMP command is about the backend, it will
> take a node name, and if it's about the guest device (which may not even
> have a node attached with removable media), it will take a qdev ID.
>
> The implementation of qmp_x_block_latency_histogram_set() is incorrect,
> it shouldn't use blk_by_name(). I wonder where it got that pattern from
> because it was added long after all other QMP commands had switched to
> qmp_get_blk() or bdrv_lookup_bs().

Do we still need blk_by_name() to be public? May be we can replace it by 
new function like blk_lookup() and hide blk_by_name() for internal use 
only or fix its behavior to search by qdev_id first and if it fails 
fallback to old way of searching by device_id?

>
> hmp_commit() should indeed use bdrv_lookup_bs() to also accept node
> names.
But it also checks blk state before commit, so I'm not quite sure it can 
work correctly without blk.
> I think we reviewed QMP to make sure we converted everything
> and forgot about HMP commands that aren't mapped to QMP.
>
> Kevin
>
Anton