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

2011-05-13 Thread Markus Armbruster 
Luiz Capitulino lcapitul...@redhat.com writes:

 On Thu, 12 May 2011 19:54:40 +0200
 Markus Armbruster arm...@redhat.com wrote:

 Luiz Capitulino lcapitul...@redhat.com writes:
 
  On Thu, 12 May 2011 19:12:56 +0200
  Markus Armbruster arm...@redhat.com wrote:
 
  Luiz Capitulino lcapitul...@redhat.com writes:
  
   On Thu, 12 May 2011 17:05:12 +0200
   Markus Armbruster arm...@redhat.com 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.

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

2011-05-13 Thread Luiz Capitulino 
On Fri, 13 May 2011 10:26:38 +0200
Markus Armbruster arm...@redhat.com wrote:

 Luiz Capitulino lcapitul...@redhat.com writes:
 
  On Thu, 12 May 2011 19:54:40 +0200
  Markus Armbruster arm...@redhat.com wrote:
 
  Luiz Capitulino lcapitul...@redhat.com writes:
  
   On Thu, 12 May 2011 19:12:56 +0200
   Markus Armbruster arm...@redhat.com wrote:
  
   Luiz Capitulino lcapitul...@redhat.com writes:
   
On Thu, 12 May 2011 17:05:12 +0200
Markus Armbruster arm...@redhat.com 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

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

2011-05-13 Thread Markus Armbruster 
Luiz Capitulino lcapitul...@redhat.com writes:

 On Fri, 13 May 2011 10:26:38 +0200
 Markus Armbruster arm...@redhat.com wrote:

 Luiz Capitulino lcapitul...@redhat.com writes:
 
  On Thu, 12 May 2011 19:54:40 +0200
  Markus Armbruster arm...@redhat.com wrote:
 
  Luiz Capitulino lcapitul...@redhat.com writes:
  
   On Thu, 12 May 2011 19:12:56 +0200
   Markus Armbruster arm...@redhat.com wrote:
  
   Luiz Capitulino lcapitul...@redhat.com writes:
[...]
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.

My point is: even your deprecation policy cannot protect your
hypothetical client.

The only way to ensure not even the silliest hypothetical client breaks
is not to change anything.  Which means we cannot fix query-block not to
lie about the type, period.  Which means we have to deprecate
query-block wholesale to fix that bug.

Kevin, please apply patches 1+2+6.  Feel free to drop 3-5.

[...]

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

2011-05-13 Thread Luiz Capitulino 
On Fri, 13 May 2011 16:13:55 +0200
Markus Armbruster arm...@redhat.com wrote:

 Luiz Capitulino lcapitul...@redhat.com writes:
 
  On Fri, 13 May 2011 10:26:38 +0200
  Markus Armbruster arm...@redhat.com wrote:
 
  Luiz Capitulino lcapitul...@redhat.com writes:
  
   On Thu, 12 May 2011 19:54:40 +0200
   Markus Armbruster arm...@redhat.com wrote:
  
   Luiz Capitulino lcapitul...@redhat.com writes:
   
On Thu, 12 May 2011 19:12:56 +0200
Markus Armbruster arm...@redhat.com wrote:
   
Luiz Capitulino lcapitul...@redhat.com writes:
 [...]
 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.
 
 My point is: even your deprecation policy cannot protect your
 hypothetical client.
 
 The only way to ensure not even the silliest hypothetical client breaks
 is not to change anything.  Which means we cannot fix query-block not to
 lie about the type, period.  Which means we have to deprecate
 query-block wholesale to fix that bug.

Why don't we force the known broken cases (like if=none) to 'unknown',
note in the doc that the field is unreliable and specify that it can be
dropped in 2 or 3 releases?

 
 Kevin, please apply patches 1+2+6.  Feel free to drop 3-5.
 
 [...]

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

2011-05-12 Thread Markus Armbruster 
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.

Signed-off-by: Markus Armbruster arm...@redhat.com
---
 block.c |   20 +++-
 qmp-commands.hx |6 --
 2 files changed, 3 insertions(+), 23 deletions(-)

diff --git a/block.c b/block.c
index f731c7a..c6dc42a 100644
--- a/block.c
+++ b/block.c
@@ -1704,9 +1704,8 @@ static void bdrv_print_dict(QObject *obj, void *opaque)
 
 bs_dict = qobject_to_qdict(obj);
 
-monitor_printf(mon, %s: type=%s removable=%d,
+monitor_printf(mon, %s: removable=%d,
 qdict_get_str(bs_dict, device),
-qdict_get_str(bs_dict, type),
 qdict_get_bool(bs_dict, removable));
 
 if (qdict_get_bool(bs_dict, removable)) {
@@ -1747,23 +1746,10 @@ void bdrv_info(Monitor *mon, QObject **ret_data)
 
 QTAILQ_FOREACH(bs, bdrv_states, list) {
 QObject *bs_obj;
-const char *type = unknown;
-
-switch(bs-type) {
-case BDRV_TYPE_HD:
-type = hd;
-break;
-case BDRV_TYPE_CDROM:
-type = cdrom;
-break;
-case BDRV_TYPE_FLOPPY:
-type = floppy;
-break;
-}
 
-bs_obj = qobject_from_jsonf({ 'device': %s, 'type': %s, 
+bs_obj = qobject_from_jsonf({ 'device': %s, 
 'removable': %i, 'locked': %i },
-bs-device_name, type, bs-removable,
+bs-device_name, bs-removable,
 bs-locked);
 
 if (bs-drv) {
diff --git a/qmp-commands.hx b/qmp-commands.hx
index fbd98ee..b1c8206 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1038,8 +1038,6 @@ is a json-array of all devices.
 Each json-object contain the following:
 
 - device: device name (json-string)
-- type: device type (json-string)
- - Possible values: hd, cdrom, floppy, unknown
 - removable: true if the device is removable, false otherwise (json-bool)
 - locked: true if the device is locked, false otherwise (json-bool)
 - inserted: only present if the device is inserted, it is a json-object
@@ -1070,25 +1068,21 @@ Example:
encrypted:false,
file:disks/test.img
 },
-type:hd
  },
  {
 device:ide1-cd0,
 locked:false,
 removable:true,
-type:cdrom
  },
  {
 device:floppy0,
 locked:false,
 removable:true,
-type: floppy
  },
  {
 device:sd0,
 locked:false,
 removable:true,
-type:floppy
  }
   ]
}
-- 
1.7.2.3

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

2011-05-12 Thread Luiz Capitulino 
On Thu, 12 May 2011 17:05:12 +0200
Markus Armbruster arm...@redhat.com 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.

Also, we can't just drop it from QMP. We should first note it's deprecated.

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

2011-05-12 Thread Markus Armbruster 
Luiz Capitulino lcapitul...@redhat.com writes:

 On Thu, 12 May 2011 17:05:12 +0200
 Markus Armbruster arm...@redhat.com 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.

 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?

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

2011-05-12 Thread Luiz Capitulino 
On Thu, 12 May 2011 19:12:56 +0200
Markus Armbruster arm...@redhat.com wrote:

 Luiz Capitulino lcapitul...@redhat.com writes:
 
  On Thu, 12 May 2011 17:05:12 +0200
  Markus Armbruster arm...@redhat.com 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?

  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.

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.

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

2011-05-12 Thread Markus Armbruster 
Luiz Capitulino lcapitul...@redhat.com writes:

 On Thu, 12 May 2011 19:12:56 +0200
 Markus Armbruster arm...@redhat.com wrote:

 Luiz Capitulino lcapitul...@redhat.com writes:
 
  On Thu, 12 May 2011 17:05:12 +0200
  Markus Armbruster arm...@redhat.com 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.

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

 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.

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.

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

2011-05-12 Thread Luiz Capitulino 
On Thu, 12 May 2011 19:54:40 +0200
Markus Armbruster arm...@redhat.com wrote:

 Luiz Capitulino lcapitul...@redhat.com writes:
 
  On Thu, 12 May 2011 19:12:56 +0200
  Markus Armbruster arm...@redhat.com wrote:
 
  Luiz Capitulino lcapitul...@redhat.com writes:
  
   On Thu, 12 May 2011 17:05:12 +0200
   Markus Armbruster arm...@redhat.com 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.

 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.

 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?

  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.