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

[Luiz Capitulino]

On Fri, 13 May 2011 10:26:38 +0200
Markus Armbruster <address@hidden> wrote:

> 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.

Sure, we should do that.

> >> 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,

Legacy option != unsupported option. I wouldn't blame anyone for doing weird
mixtures with our options, as our external interfaces are not easy to use.

And that's *our* problem, not user's. If the user did something stupid, it
probably means *we* did something stupid.

One could argue that this is an argument in favor of this change, and I'd
promptly agree, as my main point is not about not dropping it, but respecting
our deprecation policy.

> silly enough to query for information it already knows (where did I put
> that drive again?),

This assumes the client itself has started the VM, or it knows the VM
beforehand. What if it doesn't? Don't we support connecting to an already
started VM then?

> and brittle enough to break hard when it gets no
> type or type "unknown".

I said that changing something that always reported 'cdrom' to 'unknown'
is likely to cause damage. I can't tell whether it's going to break hard
or not.

> >> 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.

It's temporary, just to respect our deprecation policy and give us time
to provide a viable alternative.

> 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.

Our deprecation policy is not hypothetical. There isn't case by case. Either
we respect it or we don't.

> >> > 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.

Breaking our deprecation policy is not what I'd call good enough.