[Qemu-devel] [PATCH 0/4] A few cleanups of qdev users

[Qemu-devel] [PATCH 0/4] A few cleanups of qdev users

[Markus Armbruster]

Markus Armbruster (4):
  usb-ccid: Drop unused CCIDCardInfo callback print()
  virtio-serial: Clean up virtser_bus_dev_print() output
  virtio-serial: Turn props any virtio-serial-bus device must have into
    bus props
  ide: Turn properties any IDE device must have into bus properties

 hw/ccid.h              |    1 -
 hw/ide/qdev.c          |    5 ++++-
 hw/usb-ccid.c          |   11 -----------
 hw/virtio-console.c    |    4 ----
 hw/virtio-serial-bus.c |   18 ++++++++++--------
 5 files changed, 14 insertions(+), 25 deletions(-)

-- 
1.7.2.3

[Qemu-devel] [PATCH 1/4] usb-ccid: Drop unused CCIDCardInfo callback print()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ccid.h     |    1 -
 hw/usb-ccid.c |   11 -----------
 2 files changed, 0 insertions(+), 12 deletions(-)

diff --git a/hw/ccid.h b/hw/ccid.h
index dbfc13c..d3e0371 100644
--- a/hw/ccid.h
+++ b/hw/ccid.h
@@ -29,7 +29,6 @@ struct CCIDCardState {
  */
 struct CCIDCardInfo {
     DeviceInfo qdev;
-    void (*print)(Monitor *mon, CCIDCardState *card, int indent);
     const uint8_t *(*get_atr)(CCIDCardState *card, uint32_t *len);
     void (*apdu_from_guest)(CCIDCardState *card,
                             const uint8_t *apdu,
diff --git a/hw/usb-ccid.c b/hw/usb-ccid.c
index 079b4a2..7772fe7 100644
--- a/hw/usb-ccid.c
+++ b/hw/usb-ccid.c
@@ -1103,16 +1103,6 @@ static Answer *ccid_peek_next_answer(USBCCIDState *s)
         : &s->pending_answers[s->pending_answers_start % PENDING_ANSWERS_NUM];
 }
 
-static void ccid_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent)
-{
-    CCIDCardState *card = DO_UPCAST(CCIDCardState, qdev, qdev);
-    CCIDCardInfo *info = DO_UPCAST(CCIDCardInfo, qdev, qdev->info);
-
-    if (info->print) {
-        info->print(mon, card, indent);
-    }
-}
-
 struct CCIDBus {
     BusState qbus;
 };
@@ -1120,7 +1110,6 @@ struct CCIDBus {
 static struct BusInfo ccid_bus_info = {
     .name = "ccid-bus",
     .size = sizeof(CCIDBus),
-    .print_dev = ccid_bus_dev_print,
     .props = (Property[]) {
         DEFINE_PROP_UINT32("slot", struct CCIDCardState, slot, 0),
         DEFINE_PROP_END_OF_LIST(),
-- 
1.7.2.3

[Qemu-devel] [PATCH 2/4] virtio-serial: Clean up virtser_bus_dev_print() output

[Markus Armbruster]

Old version looks like this in info qtree (last four lines):

          dev: virtconsole, id ""
            dev-prop: is_console = 1
            dev-prop: nr = 0
            dev-prop: chardev = <null>
            dev-prop: name = <null>
             dev-prop-int: id: 0
             dev-prop-int: guest_connected: 1
             dev-prop-int: host_connected: 0
             dev-prop-int: throttled: 0

Indentation is off, and "dev-prop-int" suggests these are properties
you can configure with -device, which isn't the case.  The other
buses' print_dev() callbacks don't do that.  For instance, PCI's
output looks like this:

        class Ethernet controller, addr 00:03.0, pci id 1af4:1000 (sub 1af4:0001)
        bar 0: i/o at 0xffffffffffffffff [0x1e]
        bar 1: mem at 0xffffffffffffffff [0xffe]
        bar 6: mem at 0xffffffffffffffff [0xfffe]

Change virtser_bus_dev_print() to that style.  Result:

          dev: virtconsole, id ""
            dev-prop: is_console = 1
            dev-prop: nr = 0
            dev-prop: chardev = <null>
            dev-prop: name = <null>
            port 0, guest on, host off, throttle off

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/virtio-serial-bus.c |   13 +++++--------
 1 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index f10d48f..bd3121e 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -658,14 +658,11 @@ static void virtser_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent)
 {
     VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, qdev);
 
-    monitor_printf(mon, "%*s dev-prop-int: id: %u\n",
-                   indent, "", port->id);
-    monitor_printf(mon, "%*s dev-prop-int: guest_connected: %d\n",
-                   indent, "", port->guest_connected);
-    monitor_printf(mon, "%*s dev-prop-int: host_connected: %d\n",
-                   indent, "", port->host_connected);
-    monitor_printf(mon, "%*s dev-prop-int: throttled: %d\n",
-                   indent, "", port->throttled);
+    monitor_printf(mon, "%*sport %d, guest %s, host %s, throttle %s\n",
+                   indent, "", port->id,
+                   port->guest_connected ? "on" : "off",
+                   port->host_connected ? "on" : "off",
+                   port->throttled ? "on" : "off");
 }
 
 /* This function is only used if a port id is not provided by the user */
-- 
1.7.2.3

[Qemu-devel] [PATCH 3/4] virtio-serial: Turn props any virtio-serial-bus device must have into bus props

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/virtio-console.c    |    4 ----
 hw/virtio-serial-bus.c |    5 +++++
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index de539c4..9185140 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -117,9 +117,7 @@ static VirtIOSerialPortInfo virtconsole_info = {
     .exit          = virtconsole_exitfn,
     .qdev.props = (Property[]) {
         DEFINE_PROP_UINT8("is_console", VirtConsole, port.is_console, 1),
-        DEFINE_PROP_UINT32("nr", VirtConsole, port.id, VIRTIO_CONSOLE_BAD_ID),
         DEFINE_PROP_CHR("chardev", VirtConsole, chr),
-        DEFINE_PROP_STRING("name", VirtConsole, port.name),
         DEFINE_PROP_END_OF_LIST(),
     },
 };
@@ -152,9 +150,7 @@ static VirtIOSerialPortInfo virtserialport_info = {
     .init          = virtserialport_initfn,
     .exit          = virtconsole_exitfn,
     .qdev.props = (Property[]) {
-        DEFINE_PROP_UINT32("nr", VirtConsole, port.id, VIRTIO_CONSOLE_BAD_ID),
         DEFINE_PROP_CHR("chardev", VirtConsole, chr),
-        DEFINE_PROP_STRING("name", VirtConsole, port.name),
         DEFINE_PROP_END_OF_LIST(),
     },
 };
diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index bd3121e..a7d6b2b 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -642,6 +642,11 @@ static struct BusInfo virtser_bus_info = {
     .name      = "virtio-serial-bus",
     .size      = sizeof(VirtIOSerialBus),
     .print_dev = virtser_bus_dev_print,
+    .props      = (Property[]) {
+        DEFINE_PROP_UINT32("nr", VirtIOSerialPort, id, VIRTIO_CONSOLE_BAD_ID),
+        DEFINE_PROP_STRING("name", VirtIOSerialPort, name),
+        DEFINE_PROP_END_OF_LIST()
+    }
 };
 
 static VirtIOSerialBus *virtser_bus_new(DeviceState *dev)
-- 
1.7.2.3

[Qemu-devel] [PATCH 4/4] ide: Turn properties any IDE device must have into bus properties

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ide/qdev.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 2bb5c27..dcc4a3a 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -31,6 +31,10 @@ static struct BusInfo ide_bus_info = {
     .name  = "IDE",
     .size  = sizeof(IDEBus),
     .get_fw_dev_path = idebus_get_fw_dev_path,
+    .props = (Property[]) {
+        DEFINE_PROP_UINT32("unit", IDEDevice, unit, -1),
+        DEFINE_PROP_END_OF_LIST(),
+    },
 };
 
 void ide_bus_new(IDEBus *idebus, DeviceState *dev, int bus_id)
@@ -157,7 +161,6 @@ static IDEDeviceInfo ide_drive_info = {
     .qdev.size  = sizeof(IDEDrive),
     .init       = ide_drive_initfn,
     .qdev.props = (Property[]) {
-        DEFINE_PROP_UINT32("unit", IDEDrive, dev.unit, -1),
         DEFINE_BLOCK_PROPERTIES(IDEDrive, dev.conf),
         DEFINE_PROP_STRING("ver",  IDEDrive, dev.version),
         DEFINE_PROP_STRING("serial",  IDEDrive, dev.serial),
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH 1/4] usb-ccid: Drop unused CCIDCardInfo callback print()

[Alon Levy]

On Thu, May 19, 2011 at 01:37:14PM +0200, Markus Armbruster wrote:
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

ACK.

> ---
>  hw/ccid.h     |    1 -
>  hw/usb-ccid.c |   11 -----------
>  2 files changed, 0 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/ccid.h b/hw/ccid.h
> index dbfc13c..d3e0371 100644
> --- a/hw/ccid.h
> +++ b/hw/ccid.h
> @@ -29,7 +29,6 @@ struct CCIDCardState {
>   */
>  struct CCIDCardInfo {
>      DeviceInfo qdev;
> -    void (*print)(Monitor *mon, CCIDCardState *card, int indent);
>      const uint8_t *(*get_atr)(CCIDCardState *card, uint32_t *len);
>      void (*apdu_from_guest)(CCIDCardState *card,
>                              const uint8_t *apdu,
> diff --git a/hw/usb-ccid.c b/hw/usb-ccid.c
> index 079b4a2..7772fe7 100644
> --- a/hw/usb-ccid.c
> +++ b/hw/usb-ccid.c
> @@ -1103,16 +1103,6 @@ static Answer *ccid_peek_next_answer(USBCCIDState *s)
>          : &s->pending_answers[s->pending_answers_start % PENDING_ANSWERS_NUM];
>  }
>  
> -static void ccid_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent)
> -{
> -    CCIDCardState *card = DO_UPCAST(CCIDCardState, qdev, qdev);
> -    CCIDCardInfo *info = DO_UPCAST(CCIDCardInfo, qdev, qdev->info);
> -
> -    if (info->print) {
> -        info->print(mon, card, indent);
> -    }
> -}
> -
>  struct CCIDBus {
>      BusState qbus;
>  };
> @@ -1120,7 +1110,6 @@ struct CCIDBus {
>  static struct BusInfo ccid_bus_info = {
>      .name = "ccid-bus",
>      .size = sizeof(CCIDBus),
> -    .print_dev = ccid_bus_dev_print,
>      .props = (Property[]) {
>          DEFINE_PROP_UINT32("slot", struct CCIDCardState, slot, 0),
>          DEFINE_PROP_END_OF_LIST(),
> -- 
> 1.7.2.3
> 

Re: [Qemu-devel] [PATCH 0/4] A few cleanups of qdev users

[Gerd Hoffmann]

On 05/19/11 13:37, Markus Armbruster wrote:
> Markus Armbruster (4):
>    usb-ccid: Drop unused CCIDCardInfo callback print()
>    virtio-serial: Clean up virtser_bus_dev_print() output
>    virtio-serial: Turn props any virtio-serial-bus device must have into
>      bus props
>    ide: Turn properties any IDE device must have into bus properties
>

All good.


Acked-by: Gerd Hoffmann <kraxel@redhat.com>

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 2/4] virtio-serial: Clean up virtser_bus_dev_print() output

[Amit Shah]

On (Thu) 19 May 2011 [13:37:15], Markus Armbruster wrote:
> Old version looks like this in info qtree (last four lines):
> 
>           dev: virtconsole, id ""
>             dev-prop: is_console = 1
>             dev-prop: nr = 0
>             dev-prop: chardev = <null>
>             dev-prop: name = <null>
>              dev-prop-int: id: 0
>              dev-prop-int: guest_connected: 1
>              dev-prop-int: host_connected: 0
>              dev-prop-int: throttled: 0
> 
> Indentation is off, and "dev-prop-int" suggests these are properties
> you can configure with -device, which isn't the case.  The other
> buses' print_dev() callbacks don't do that.  For instance, PCI's
> output looks like this:
> 
>         class Ethernet controller, addr 00:03.0, pci id 1af4:1000 (sub 1af4:0001)
>         bar 0: i/o at 0xffffffffffffffff [0x1e]
>         bar 1: mem at 0xffffffffffffffff [0xffe]
>         bar 6: mem at 0xffffffffffffffff [0xfffe]
> 
> Change virtser_bus_dev_print() to that style.  Result:
> 
>           dev: virtconsole, id ""
>             dev-prop: is_console = 1
>             dev-prop: nr = 0
>             dev-prop: chardev = <null>
>             dev-prop: name = <null>
>             port 0, guest on, host off, throttle off

Here the original guest_connected and host_connected meant whether the
endpoints were open.  guest on/off, host on/off don't convey that
meaning.  Can't think of a short version, can you?

		Amit

Re: [Qemu-devel] [PATCH 3/4] virtio-serial: Turn props any virtio-serial-bus device must have into bus props

[Amit Shah]

On (Thu) 19 May 2011 [13:37:16], Markus Armbruster wrote:
> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> index bd3121e..a7d6b2b 100644
> --- a/hw/virtio-serial-bus.c
> +++ b/hw/virtio-serial-bus.c
> @@ -642,6 +642,11 @@ static struct BusInfo virtser_bus_info = {
>      .name      = "virtio-serial-bus",
>      .size      = sizeof(VirtIOSerialBus),
>      .print_dev = virtser_bus_dev_print,
> +    .props      = (Property[]) {
> +        DEFINE_PROP_UINT32("nr", VirtIOSerialPort, id, VIRTIO_CONSOLE_BAD_ID),
> +        DEFINE_PROP_STRING("name", VirtIOSerialPort, name),
> +        DEFINE_PROP_END_OF_LIST()
> +    }

'name' isn't a must-have for all devices, but this is change OK.

		Amit

Re: [Qemu-devel] [PATCH 3/4] virtio-serial: Turn props any virtio-serial-bus device must have into bus props

[Markus Armbruster]

Amit Shah <amit.shah@redhat.com> writes:

> On (Thu) 19 May 2011 [13:37:16], Markus Armbruster wrote:
>> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
>> index bd3121e..a7d6b2b 100644
>> --- a/hw/virtio-serial-bus.c
>> +++ b/hw/virtio-serial-bus.c
>> @@ -642,6 +642,11 @@ static struct BusInfo virtser_bus_info = {
>>      .name      = "virtio-serial-bus",
>>      .size      = sizeof(VirtIOSerialBus),
>>      .print_dev = virtser_bus_dev_print,
>> +    .props      = (Property[]) {
>> +        DEFINE_PROP_UINT32("nr", VirtIOSerialPort, id, VIRTIO_CONSOLE_BAD_ID),
>> +        DEFINE_PROP_STRING("name", VirtIOSerialPort, name),
>> +        DEFINE_PROP_END_OF_LIST()
>> +    }
>
> 'name' isn't a must-have for all devices, but this is change OK.

The property is a must have, even though its value may remain null.
Correct?

Re: [Qemu-devel] [PATCH 3/4] virtio-serial: Turn props any virtio-serial-bus device must have into bus props

[Amit Shah]

On (Thu) 19 May 2011 [16:05:39], Markus Armbruster wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > On (Thu) 19 May 2011 [13:37:16], Markus Armbruster wrote:
> >> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> >> index bd3121e..a7d6b2b 100644
> >> --- a/hw/virtio-serial-bus.c
> >> +++ b/hw/virtio-serial-bus.c
> >> @@ -642,6 +642,11 @@ static struct BusInfo virtser_bus_info = {
> >>      .name      = "virtio-serial-bus",
> >>      .size      = sizeof(VirtIOSerialBus),
> >>      .print_dev = virtser_bus_dev_print,
> >> +    .props      = (Property[]) {
> >> +        DEFINE_PROP_UINT32("nr", VirtIOSerialPort, id, VIRTIO_CONSOLE_BAD_ID),
> >> +        DEFINE_PROP_STRING("name", VirtIOSerialPort, name),
> >> +        DEFINE_PROP_END_OF_LIST()
> >> +    }
> >
> > 'name' isn't a must-have for all devices, but this is change OK.
> 
> The property is a must have, even though its value may remain null.
> Correct?

Ah; must have for the device (ie qdev purposes)?  Yes.  It's not a
must-have for guest-host communication, that's what I meant.

		Amit

Re: [Qemu-devel] [PATCH 2/4] virtio-serial: Clean up virtser_bus_dev_print() output

[Markus Armbruster]

Amit Shah <amit.shah@redhat.com> writes:

> On (Thu) 19 May 2011 [13:37:15], Markus Armbruster wrote:
>> Old version looks like this in info qtree (last four lines):
>> 
>>           dev: virtconsole, id ""
>>             dev-prop: is_console = 1
>>             dev-prop: nr = 0
>>             dev-prop: chardev = <null>
>>             dev-prop: name = <null>
>>              dev-prop-int: id: 0
>>              dev-prop-int: guest_connected: 1
>>              dev-prop-int: host_connected: 0
>>              dev-prop-int: throttled: 0
>> 
>> Indentation is off, and "dev-prop-int" suggests these are properties
>> you can configure with -device, which isn't the case.  The other
>> buses' print_dev() callbacks don't do that.  For instance, PCI's
>> output looks like this:
>> 
>>         class Ethernet controller, addr 00:03.0, pci id 1af4:1000 (sub 1af4:0001)
>>         bar 0: i/o at 0xffffffffffffffff [0x1e]
>>         bar 1: mem at 0xffffffffffffffff [0xffe]
>>         bar 6: mem at 0xffffffffffffffff [0xfffe]
>> 
>> Change virtser_bus_dev_print() to that style.  Result:
>> 
>>           dev: virtconsole, id ""
>>             dev-prop: is_console = 1
>>             dev-prop: nr = 0
>>             dev-prop: chardev = <null>
>>             dev-prop: name = <null>
>>             port 0, guest on, host off, throttle off
>
> Here the original guest_connected and host_connected meant whether the
> endpoints were open.  guest on/off, host on/off don't convey that
> meaning.  Can't think of a short version, can you?

I chose on/off to stay consistent with how qdev shows bool properties
(print_bit() in qdev-properties.c).  May be misguided.  Like you, I'm
having difficulties coming up with a better version that is still
consise.

But: should "info qtree" show such device state?  It's about
configuration of the device tree, isn't it?  Connection status is useful
to know, but it's not device configuration.  Other print_dev() methods
may cross that line, too.  For instance, usb_bus_dev_print() prints
attached, which looks suspicious (commit 66a6593a).

Re: [Qemu-devel] [PATCH 2/4] virtio-serial: Clean up virtser_bus_dev_print() output

[Amit Shah]

On (Thu) 19 May 2011 [16:18:29], Markus Armbruster wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > On (Thu) 19 May 2011 [13:37:15], Markus Armbruster wrote:
> >> Old version looks like this in info qtree (last four lines):
> >> 
> >>           dev: virtconsole, id ""
> >>             dev-prop: is_console = 1
> >>             dev-prop: nr = 0
> >>             dev-prop: chardev = <null>
> >>             dev-prop: name = <null>
> >>              dev-prop-int: id: 0
> >>              dev-prop-int: guest_connected: 1
> >>              dev-prop-int: host_connected: 0
> >>              dev-prop-int: throttled: 0
> >> 
> >> Indentation is off, and "dev-prop-int" suggests these are properties
> >> you can configure with -device, which isn't the case.  The other
> >> buses' print_dev() callbacks don't do that.  For instance, PCI's
> >> output looks like this:
> >> 
> >>         class Ethernet controller, addr 00:03.0, pci id 1af4:1000 (sub 1af4:0001)
> >>         bar 0: i/o at 0xffffffffffffffff [0x1e]
> >>         bar 1: mem at 0xffffffffffffffff [0xffe]
> >>         bar 6: mem at 0xffffffffffffffff [0xfffe]
> >> 
> >> Change virtser_bus_dev_print() to that style.  Result:
> >> 
> >>           dev: virtconsole, id ""
> >>             dev-prop: is_console = 1
> >>             dev-prop: nr = 0
> >>             dev-prop: chardev = <null>
> >>             dev-prop: name = <null>
> >>             port 0, guest on, host off, throttle off
> >
> > Here the original guest_connected and host_connected meant whether the
> > endpoints were open.  guest on/off, host on/off don't convey that
> > meaning.  Can't think of a short version, can you?
> 
> I chose on/off to stay consistent with how qdev shows bool properties
> (print_bit() in qdev-properties.c).  May be misguided.  Like you, I'm
> having difficulties coming up with a better version that is still
> consise.
> 
> But: should "info qtree" show such device state?  It's about
> configuration of the device tree, isn't it?

Ah; right.  guest on/off, throttle are for debug (like debugfs
counterpart in the kernel).  I guess we'd need a new monitor command
to print these out?

		Amit

Re: [Qemu-devel] [PATCH 0/4] A few cleanups of qdev users

[Markus Armbruster]

Ping?

Markus Armbruster <armbru@redhat.com> writes:

> Markus Armbruster (4):
>   usb-ccid: Drop unused CCIDCardInfo callback print()
>   virtio-serial: Clean up virtser_bus_dev_print() output
>   virtio-serial: Turn props any virtio-serial-bus device must have into
>     bus props
>   ide: Turn properties any IDE device must have into bus properties
>
>  hw/ccid.h              |    1 -
>  hw/ide/qdev.c          |    5 ++++-
>  hw/usb-ccid.c          |   11 -----------
>  hw/virtio-console.c    |    4 ----
>  hw/virtio-serial-bus.c |   18 ++++++++++--------
>  5 files changed, 14 insertions(+), 25 deletions(-)

Re: [Qemu-devel] [PATCH 0/4] A few cleanups of qdev users

[Amit Shah]

On (Fri) 24 Jun 2011 [13:57:28], Markus Armbruster wrote:
> Ping?

There were a couple of things:

>             port 0, guest on, host off, throttle off

guest on/off, host on/off doesn't convey much -- what's on/off?

Also, 'throttle' could be 'thottled'?

		Amit

Re: [Qemu-devel] [PATCH 0/4] A few cleanups of qdev users

[Markus Armbruster]

Amit Shah <amit.shah@redhat.com> writes:

> On (Fri) 24 Jun 2011 [13:57:28], Markus Armbruster wrote:
>> Ping?
>
> There were a couple of things:
>
>>             port 0, guest on, host off, throttle off
>
> guest on/off, host on/off doesn't convey much -- what's on/off?
>
> Also, 'throttle' could be 'thottled'?

Discussion petered out with my message[*]:

    I chose on/off to stay consistent with how qdev shows bool
    properties (print_bit() in qdev-properties.c).  May be misguided.
    Like you, I'm having difficulties coming up with a better version
    that is still consise.
    
    But: should "info qtree" show such device state?  It's about
    configuration of the device tree, isn't it?  Connection status is
    useful to know, but it's not device configuration.  Other
    print_dev() methods may cross that line, too.  For instance,
    usb_bus_dev_print() prints attached, which looks suspicious (commit
    66a6593a).

Should info qtree continue to show this information?  If yes, care to
suggest a better format?


[*] http://lists.gnu.org/archive/html/qemu-devel/2011-05/msg01919.html

Re: [Qemu-devel] [PATCH 2/4] virtio-serial: Clean up virtser_bus_dev_print() output

[Andreas Färber]

Am 19.05.2011 um 16:18 schrieb Markus Armbruster:

> Amit Shah <amit.shah@redhat.com> writes:
>
>> On (Thu) 19 May 2011 [13:37:15], Markus Armbruster wrote:
>>> Old version looks like this in info qtree (last four lines):
>>>
>>>          dev: virtconsole, id ""
>>>            dev-prop: is_console = 1
>>>            dev-prop: nr = 0
>>>            dev-prop: chardev = <null>
>>>            dev-prop: name = <null>
>>>             dev-prop-int: id: 0
>>>             dev-prop-int: guest_connected: 1
>>>             dev-prop-int: host_connected: 0
>>>             dev-prop-int: throttled: 0
>>>
>>> Indentation is off, and "dev-prop-int" suggests these are properties
>>> you can configure with -device, which isn't the case.  The other
>>> buses' print_dev() callbacks don't do that.  For instance, PCI's
>>> output looks like this:
>>>
>>>        class Ethernet controller, addr 00:03.0, pci id 1af4:1000  
>>> (sub 1af4:0001)
>>>        bar 0: i/o at 0xffffffffffffffff [0x1e]
>>>        bar 1: mem at 0xffffffffffffffff [0xffe]
>>>        bar 6: mem at 0xffffffffffffffff [0xfffe]
>>>
>>> Change virtser_bus_dev_print() to that style.  Result:
>>>
>>>          dev: virtconsole, id ""
>>>            dev-prop: is_console = 1
>>>            dev-prop: nr = 0
>>>            dev-prop: chardev = <null>
>>>            dev-prop: name = <null>
>>>            port 0, guest on, host off, throttle off
>>
>> Here the original guest_connected and host_connected meant whether  
>> the
>> endpoints were open.  guest on/off, host on/off don't convey that
>> meaning.  Can't think of a short version, can you?
>
> I chose on/off to stay consistent with how qdev shows bool properties
> (print_bit() in qdev-properties.c).  May be misguided.  Like you, I'm
> having difficulties coming up with a better version that is still
> consise.

Erm, I'm not aware that my qdev bool patch got committed yet, so the  
question of how to parse/print bool properties (on/off vs. yes/no) is  
still undecided, no comments so far. It would be entirely possible to  
let the author decide that on a case-by-case basis by using different  
property type enums for the same 'bool' type.

Andreas

Re: [Qemu-devel] [PATCH 0/4] A few cleanups of qdev users

[Amit Shah]

On (Mon) 27 Jun 2011 [14:36:11], Markus Armbruster wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > On (Fri) 24 Jun 2011 [13:57:28], Markus Armbruster wrote:
> >> Ping?
> >
> > There were a couple of things:
> >
> >>             port 0, guest on, host off, throttle off
> >
> > guest on/off, host on/off doesn't convey much -- what's on/off?
> >
> > Also, 'throttle' could be 'thottled'?
> 
> Discussion petered out with my message[*]:
> 
>     I chose on/off to stay consistent with how qdev shows bool
>     properties (print_bit() in qdev-properties.c).  May be misguided.
>     Like you, I'm having difficulties coming up with a better version
>     that is still consise.
>     
>     But: should "info qtree" show such device state?  It's about
>     configuration of the device tree, isn't it?  Connection status is
>     useful to know, but it's not device configuration.  Other
>     print_dev() methods may cross that line, too.  For instance,
>     usb_bus_dev_print() prints attached, which looks suspicious (commit
>     66a6593a).
> 
> Should info qtree continue to show this information?  If yes, care to
> suggest a better format?

Don't know.  I'm fine with anything the qdev guys decide.  I agree
this isn't device state.


		Amit

Re: [Qemu-devel] [PATCH 0/4] A few cleanups of qdev users

[Markus Armbruster]

Amit Shah <amit.shah@redhat.com> writes:

> On (Mon) 27 Jun 2011 [14:36:11], Markus Armbruster wrote:
>> Amit Shah <amit.shah@redhat.com> writes:
>> 
>> > On (Fri) 24 Jun 2011 [13:57:28], Markus Armbruster wrote:
>> >> Ping?
>> >
>> > There were a couple of things:
>> >
>> >>             port 0, guest on, host off, throttle off
>> >
>> > guest on/off, host on/off doesn't convey much -- what's on/off?
>> >
>> > Also, 'throttle' could be 'thottled'?
>> 
>> Discussion petered out with my message[*]:
>> 
>>     I chose on/off to stay consistent with how qdev shows bool
>>     properties (print_bit() in qdev-properties.c).  May be misguided.
>>     Like you, I'm having difficulties coming up with a better version
>>     that is still consise.
>>     
>>     But: should "info qtree" show such device state?  It's about
>>     configuration of the device tree, isn't it?  Connection status is
>>     useful to know, but it's not device configuration.  Other
>>     print_dev() methods may cross that line, too.  For instance,
>>     usb_bus_dev_print() prints attached, which looks suspicious (commit
>>     66a6593a).
>> 
>> Should info qtree continue to show this information?  If yes, care to
>> suggest a better format?
>
> Don't know.  I'm fine with anything the qdev guys decide.  I agree
> this isn't device state.

Unfortunately, there's no qdev maintainer making descisions.

What shall we do now?

1. Commit as is.  Need an ACK then.

2. Respin with virtser_bus_dev_print() printing the same stuff prettier.
Need ideas on a prettier format.

3. Respin with virtser_bus_dev_print() printing less stuff, but
prettier.  Need ideas on what exactly to print, and how.

Re: [Qemu-devel] [PATCH 0/4] A few cleanups of qdev users

[Amit Shah]

On (Tue) 28 Jun 2011 [14:24:32], Markus Armbruster wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > On (Mon) 27 Jun 2011 [14:36:11], Markus Armbruster wrote:
> >> Amit Shah <amit.shah@redhat.com> writes:
> >> 
> >> > On (Fri) 24 Jun 2011 [13:57:28], Markus Armbruster wrote:
> >> >> Ping?
> >> >
> >> > There were a couple of things:
> >> >
> >> >>             port 0, guest on, host off, throttle off
> >> >
> >> > guest on/off, host on/off doesn't convey much -- what's on/off?
> >> >
> >> > Also, 'throttle' could be 'thottled'?
> >> 
> >> Discussion petered out with my message[*]:
> >> 
> >>     I chose on/off to stay consistent with how qdev shows bool
> >>     properties (print_bit() in qdev-properties.c).  May be misguided.
> >>     Like you, I'm having difficulties coming up with a better version
> >>     that is still consise.
> >>     
> >>     But: should "info qtree" show such device state?  It's about
> >>     configuration of the device tree, isn't it?  Connection status is
> >>     useful to know, but it's not device configuration.  Other
> >>     print_dev() methods may cross that line, too.  For instance,
> >>     usb_bus_dev_print() prints attached, which looks suspicious (commit
> >>     66a6593a).
> >> 
> >> Should info qtree continue to show this information?  If yes, care to
> >> suggest a better format?
> >
> > Don't know.  I'm fine with anything the qdev guys decide.  I agree
> > this isn't device state.
> 
> Unfortunately, there's no qdev maintainer making descisions.

I think Gerd and you can make those decisions? :-)

> What shall we do now?
> 
> 1. Commit as is.  Need an ACK then.
> 
> 2. Respin with virtser_bus_dev_print() printing the same stuff prettier.
> Need ideas on a prettier format.
> 
> 3. Respin with virtser_bus_dev_print() printing less stuff, but
> prettier.  Need ideas on what exactly to print, and how.

Frankly, I've no clue.  I can only suggest some better names, but
since these values are debug values, and there's not much churn
happening in the code, I could go with anything you people suggest.
I'm open to slightly renamed strings going in, open to reworking
things, etc..

How about:

    port 0, guest_con on, host_con off, throttled off

?


		Amit

Re: [Qemu-devel] [PATCH 2/4] virtio-serial: Clean up virtser_bus_dev_print() output

[Gerd Hoffmann]

   Hi,

> Erm, I'm not aware that my qdev bool patch got committed yet, so the
> question of how to parse/print bool properties (on/off vs. yes/no) is
> still undecided, no comments so far. It would be entirely possible to
> let the author decide that on a case-by-case basis by using different
> property type enums for the same 'bool' type.

Just accept both "on" and "yes" for true and both "off" and "no" for false?

For printing we'll have to pick one though.  I'd tend to pick on/off as 
I guess most properties will enable or disable some feature.  Or have 
the two enums ...

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 2/4] virtio-serial: Clean up virtser_bus_dev_print() output

[Markus Armbruster]

Andreas Färber <andreas.faerber@web.de> writes:

> Am 19.05.2011 um 16:18 schrieb Markus Armbruster:
>
>> Amit Shah <amit.shah@redhat.com> writes:
>>
>>> On (Thu) 19 May 2011 [13:37:15], Markus Armbruster wrote:
>>>> Old version looks like this in info qtree (last four lines):
>>>>
>>>>          dev: virtconsole, id ""
>>>>            dev-prop: is_console = 1
>>>>            dev-prop: nr = 0
>>>>            dev-prop: chardev = <null>
>>>>            dev-prop: name = <null>
>>>>             dev-prop-int: id: 0
>>>>             dev-prop-int: guest_connected: 1
>>>>             dev-prop-int: host_connected: 0
>>>>             dev-prop-int: throttled: 0
>>>>
>>>> Indentation is off, and "dev-prop-int" suggests these are properties
>>>> you can configure with -device, which isn't the case.  The other
>>>> buses' print_dev() callbacks don't do that.  For instance, PCI's
>>>> output looks like this:
>>>>
>>>>        class Ethernet controller, addr 00:03.0, pci id 1af4:1000
>>>> (sub 1af4:0001)
>>>>        bar 0: i/o at 0xffffffffffffffff [0x1e]
>>>>        bar 1: mem at 0xffffffffffffffff [0xffe]
>>>>        bar 6: mem at 0xffffffffffffffff [0xfffe]
>>>>
>>>> Change virtser_bus_dev_print() to that style.  Result:
>>>>
>>>>          dev: virtconsole, id ""
>>>>            dev-prop: is_console = 1
>>>>            dev-prop: nr = 0
>>>>            dev-prop: chardev = <null>
>>>>            dev-prop: name = <null>
>>>>            port 0, guest on, host off, throttle off
>>>
>>> Here the original guest_connected and host_connected meant whether
>>> the
>>> endpoints were open.  guest on/off, host on/off don't convey that
>>> meaning.  Can't think of a short version, can you?
>>
>> I chose on/off to stay consistent with how qdev shows bool properties
>> (print_bit() in qdev-properties.c).  May be misguided.  Like you, I'm
>> having difficulties coming up with a better version that is still
>> consise.
>
> Erm, I'm not aware that my qdev bool patch got committed yet, so the
> question of how to parse/print bool properties (on/off vs. yes/no) is
> still undecided, no comments so far.

No, there is precedence: PROP_TYPE_BIT's parse_bit(), print_bit().  The
fact that it's a bit within a uint32_t rather than bool is
implementation detail that shouldn't matter at the -device / info qtree
level.

>                                      It would be entirely possible to
> let the author decide that on a case-by-case basis by using different
> property type enums for the same 'bool' type.

Possible, but is it wise?

Re: [Qemu-devel] [PATCH 2/4] virtio-serial: Clean up virtser_bus_dev_print() output

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

>   Hi,
>
>> Erm, I'm not aware that my qdev bool patch got committed yet, so the
>> question of how to parse/print bool properties (on/off vs. yes/no) is
>> still undecided, no comments so far. It would be entirely possible to
>> let the author decide that on a case-by-case basis by using different
>> property type enums for the same 'bool' type.
>
> Just accept both "on" and "yes" for true and both "off" and "no" for false?
>
> For printing we'll have to pick one though.  I'd tend to pick on/off
> as I guess most properties will enable or disable some feature.  Or
> have the two enums ...

KISS.  Let's stick to one way to print bools.

If on/off carries so much meaning that it offends aesthetic
sensibilities too much, sidestep meaning and use 1/0.

Re: [Qemu-devel] [PATCH 2/4] virtio-serial: Clean up virtser_bus_dev_print() output

[Andreas Färber]

Hi,

Am 29.06.2011 um 10:33 schrieb Gerd Hoffmann:

>> Erm, I'm not aware that my qdev bool patch got committed yet, so the
>> question of how to parse/print bool properties (on/off vs. yes/no) is
>> still undecided, no comments so far. It would be entirely possible to
>> let the author decide that on a case-by-case basis by using different
>> property type enums for the same 'bool' type.
>
> Just accept both "on" and "yes" for true and both "off" and "no" for  
> false?

That's exactly what the patch does, in Markus' parse_bit() among others:

http://patchwork.ozlabs.org/patch/100271/

> For printing we'll have to pick one though.  I'd tend to pick on/off  
> as I guess most properties will enable or disable some feature.  Or  
> have the two enums ...

Like I wrote in another thread, on/off only applies to nouns, not  
adjectives like "enabled". I'll revisit this topic the weekend.

Andreas

Re: [Qemu-devel] [PATCH 0/4] A few cleanups of qdev users

[Anthony Liguori]

On 05/19/2011 06:37 AM, Markus Armbruster wrote:
> Markus Armbruster (4):
>    usb-ccid: Drop unused CCIDCardInfo callback print()
>    virtio-serial: Clean up virtser_bus_dev_print() output
>    virtio-serial: Turn props any virtio-serial-bus device must have into
>      bus props
>    ide: Turn properties any IDE device must have into bus properties

Applied.  Thanks.

Regards,

Anthony Liguori

>
>   hw/ccid.h              |    1 -
>   hw/ide/qdev.c          |    5 ++++-
>   hw/usb-ccid.c          |   11 -----------
>   hw/virtio-console.c    |    4 ----
>   hw/virtio-serial-bus.c |   18 ++++++++++--------
>   5 files changed, 14 insertions(+), 25 deletions(-)
>