| Message ID | 1305805037-17752-3-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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).
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
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
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
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?
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.
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
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 */
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(-)