Re: [Qemu-devel] [PATCH v3 3/6] block QMP: Drop query-block member "type" (type= in info block)

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> On Thu, 12 May 2011 19:54:40 +0200
> Markus Armbruster <address@hidden> wrote:
>
>> Luiz Capitulino <address@hidden> writes:
>> 
>> > On Thu, 12 May 2011 19:12:56 +0200
>> > Markus Armbruster <address@hidden> wrote:
>> >
>> >> Luiz Capitulino <address@hidden> writes:
>> >> 
>> >> > On Thu, 12 May 2011 17:05:12 +0200
>> >> > Markus Armbruster <address@hidden> wrote:
>> >> >
>> >> >> Its value is unreliable: a block device used as floppy has type
>> >> >> "floppy" if created with if=floppy, but type "hd" if created with
>> >> >> if=none.
>> >> >> 
>> >> >> That's because with if=none, the type is at best a declaration of
>> >> >> intent: the drive can be connected to any guest device.  Its type is
>> >> >> really the guest device's business.  Reporting it here is wrong.
>> >> >
>> >> > It reports how the guest is using the device, right? I'd say that's what
>> >> > users/clients are interested in knowing.
>> >> 
>> >> The value is *unreliable*.  It may or may not match how the guest is
>> >> using the device.  I doubt users are interested in unreliable
>> >> information.
>> >
>> > Can't it be fixed? And how are users/clients supposed to find out how
>> > the guest is using its block devices?
>> 
>> To find out more about the guest's devices, examine the guest's devices:
>> info qtree.
>
> Which is not converted to QMP yet.

It's where the information is.  No use looking for it where it isn't.
If users need it, we better convert info qtree.

>> You don't expect to find the guest serial devices in in "info chardev",
>> either.  query-block's type member is a mistake, because it mixes up
>> guest device info with the host device info.  Dropping it is a bug fix.
>
> I understand why you're dropping it, what I don't want to do is to break
> working clients. For example, there might be clients out there using it
> in a way that it's expected to work (eg. if=floppy).
>
> Of course that I'm assuming that such a client exist.

A client new enough to use QMP, old enough to use legacy if=floppy,
silly enough to query for information it already knows (where did I put
that drive again?), and brittle enough to break hard when it gets no
type or type "unknown".

>> The fact that its value is unreliable is merely icing on the cake.
>> 
>> >> > Also, we can't just drop it from QMP. We should first note it's 
>> >> > deprecated.
>> >> 
>> >> Would you accept a change to the more honest value "unknown" for the
>> >> deprecation period?
>> >
>> > We have to avoid breaking the protocol. Changing something that has always
>> > been reported as 'cdrom' to 'unknown' will likely cause as many as damages
>> > as dropping the command.
>> 
>> I can cause damage only if somebody is using it.  Which I doubt.
>
> Me too and I'd agree with this patch if I was 100% sure. But it's impossible
> to be sure, unless we do it by trial and error which is harmful.
>
>> Remember, the value is unreliable.  It's a *lie*.  We can stop lying in
>> two ways: shut up (drop member "type"), or tell the truth (change the
>> value to "unknown", which is a documented value of "type").
>
> Can we set it to 'unknown' when if=none?

Maybe.  Makes query-block mix up host and guest information again.
Purging guest information from block.c is the point of this series.
Therefore, query-block can't be done in block.c anymore.  It needs to
move to blockdev.c, where the mixed-up-by-design DriveInfo is available.
It could move back when we finally clean up query-block.

Even with such compatibility gymnastics, it could still break your
hypothetical client.

>> > The best solution I can think of is noting in the documentation that the
>> > information is unreliable and explain what clients interested in knowing
>> > this info should do.
>> 
>> I'd be much more willing to jump through compatibility hoops if there
>> was *one* known user of this particular detail of QMP.
>
> Ideally, yes, but it's the type of thing we'll never know.
>
>> But if you insist on us continuing to lie, I'll find a way to continue
>> to lie.  I'm resisting it, because I think it's a disservice to our
>> users.
>
> I also want to do the best for our users and I don't want to ignore the bug,
> but we don't know whether there are clients using the field. If we drop it
> and a client does use it, then we'll have failed in doing the best.

Good enough is good enough.