[Qemu-devel] [PATCH 0/2] usb-linux: physical port handling.

[Qemu-devel] [PATCH 0/2] usb-linux: physical port handling.

[Gerd Hoffmann]

  Hi,

These patches fix and improve the physical port handling in the usb host
driver.  Passthrough of devices connected via usb hub should work better
now.  Also you can specify usb devices for passthrough by physical port
(on the host) now.

please review,
  Gerd

Gerd Hoffmann (2):
  usb-linux: fix device path aka physical port handling
  usb-linux: add hostport property

 usb-linux.c |   47 +++++++++++++++++++++++++++--------------------
 1 files changed, 27 insertions(+), 20 deletions(-)

[Qemu-devel] [PATCH 1/2] usb-linux: fix device path aka physical port handling

[Gerd Hoffmann]

The device path isn't just a number.  It specifies the physical port
the device is connected to and in case the device is connected via
usb hub you'll have two numbers there, like this: "5.1".  The first
specifies the root port where the hub is plugged into, the second
specifies the port number of the hub where the device is plugged in.
With multiple hubs chained the string can become longer.

This patch renames devpath to port and makes it a string.   It also
adapts the sysfs parsing code accordingly.  The "info usbhost" monitor
command now prints bus number, (os-assigned) device address and physical
port for each device.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 usb-linux.c |   38 ++++++++++++++++++++------------------
 1 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index 72fe6f5..9543a69 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -62,7 +62,7 @@ struct usb_ctrlrequest {
     uint16_t wLength;
 };
 
-typedef int USBScanFunc(void *opaque, int bus_num, int addr, int devpath,
+typedef int USBScanFunc(void *opaque, int bus_num, int addr, char *port,
                         int class_id, int vendor_id, int product_id,
                         const char *product_name, int speed);
 
@@ -79,6 +79,7 @@ typedef int USBScanFunc(void *opaque, int bus_num, int addr, int devpath,
 #define USBPROCBUS_PATH "/proc/bus/usb"
 #define PRODUCT_NAME_SZ 32
 #define MAX_ENDPOINTS 15
+#define MAX_PORTLEN 8
 #define USBDEVBUS_PATH "/dev/bus/usb"
 #define USBSYSBUS_PATH "/sys/bus/usb"
 
@@ -155,7 +156,7 @@ typedef struct USBHostDevice {
     /* Host side address */
     int bus_num;
     int addr;
-    int devpath;
+    char port[MAX_PORTLEN];
     struct USBAutoFilter match;
 
     QTAILQ_ENTRY(USBHostDevice) next;
@@ -1092,7 +1093,7 @@ static int usb_linux_get_configuration(USBHostDevice *s)
         char device_name[32], line[1024];
         int configuration;
 
-        sprintf(device_name, "%d-%d", s->bus_num, s->devpath);
+        sprintf(device_name, "%d-%s", s->bus_num, s->port);
 
         if (!usb_host_read_file(line, sizeof(line), "bConfigurationValue",
                                 device_name)) {
@@ -1138,7 +1139,7 @@ static uint8_t usb_linux_get_alt_setting(USBHostDevice *s,
         char device_name[64], line[1024];
         int alt_setting;
 
-        sprintf(device_name, "%d-%d:%d.%d", s->bus_num, s->devpath,
+        sprintf(device_name, "%d-%s:%d.%d", s->bus_num, s->port,
                 (int)configuration, (int)interface);
 
         if (!usb_host_read_file(line, sizeof(line), "bAlternateSetting",
@@ -1257,7 +1258,7 @@ static int usb_linux_update_endp_table(USBHostDevice *s)
 }
 
 static int usb_host_open(USBHostDevice *dev, int bus_num,
-                         int addr, int devpath, const char *prod_name)
+                         int addr, char *port, const char *prod_name)
 {
     int fd = -1, ret;
     struct usbdevfs_connectinfo ci;
@@ -1283,7 +1284,7 @@ static int usb_host_open(USBHostDevice *dev, int bus_num,
 
     dev->bus_num = bus_num;
     dev->addr = addr;
-    dev->devpath = devpath;
+    strcpy(dev->port, port);
     dev->fd = fd;
 
     /* read the device description */
@@ -1655,8 +1656,9 @@ static int usb_host_scan_sys(void *opaque, USBScanFunc *func)
 {
     DIR *dir = NULL;
     char line[1024];
-    int bus_num, addr, devpath, speed, class_id, product_id, vendor_id;
+    int bus_num, addr, speed, class_id, product_id, vendor_id;
     int ret = 0;
+    char port[MAX_PORTLEN];
     char product_name[512];
     struct dirent *de;
 
@@ -1672,8 +1674,8 @@ static int usb_host_scan_sys(void *opaque, USBScanFunc *func)
             if (!strncmp(de->d_name, "usb", 3)) {
                 tmpstr += 3;
             }
-            if (sscanf(tmpstr, "%d-%d", &bus_num, &devpath) < 1) {
-                goto the_end;
+            if (sscanf(tmpstr, "%d-%7[0-9.]", &bus_num, port) < 2) {
+                continue;
             }
 
             if (!usb_host_read_file(line, sizeof(line), "devnum", de->d_name)) {
@@ -1725,7 +1727,7 @@ static int usb_host_scan_sys(void *opaque, USBScanFunc *func)
                 speed = USB_SPEED_FULL;
             }
 
-            ret = func(opaque, bus_num, addr, devpath, class_id, vendor_id,
+            ret = func(opaque, bus_num, addr, port, class_id, vendor_id,
                        product_id, product_name, speed);
             if (ret) {
                 goto the_end;
@@ -1816,7 +1818,7 @@ static int usb_host_scan(void *opaque, USBScanFunc *func)
 
 static QEMUTimer *usb_auto_timer;
 
-static int usb_host_auto_scan(void *opaque, int bus_num, int addr, int devpath,
+static int usb_host_auto_scan(void *opaque, int bus_num, int addr, char *port,
                               int class_id, int vendor_id, int product_id,
                               const char *product_name, int speed)
 {
@@ -1852,7 +1854,7 @@ static int usb_host_auto_scan(void *opaque, int bus_num, int addr, int devpath,
         }
         DPRINTF("husb: auto open: bus_num %d addr %d\n", bus_num, addr);
 
-        usb_host_open(s, bus_num, addr, devpath, product_name);
+        usb_host_open(s, bus_num, addr, port, product_name);
     }
 
     return 0;
@@ -1974,8 +1976,8 @@ static const char *usb_class_str(uint8_t class)
     return p->class_name;
 }
 
-static void usb_info_device(Monitor *mon, int bus_num, int addr, int class_id,
-                            int vendor_id, int product_id,
+static void usb_info_device(Monitor *mon, int bus_num, int addr, char *port,
+                            int class_id, int vendor_id, int product_id,
                             const char *product_name,
                             int speed)
 {
@@ -1996,8 +1998,8 @@ static void usb_info_device(Monitor *mon, int bus_num, int addr, int class_id,
         break;
     }
 
-    monitor_printf(mon, "  Device %d.%d, speed %s Mb/s\n",
-                bus_num, addr, speed_str);
+    monitor_printf(mon, "  Bus %d, Addr %d, Port %s, Speed %s Mb/s\n",
+                   bus_num, addr, port, speed_str);
     class_str = usb_class_str(class_id);
     if (class_str) {
         monitor_printf(mon, "    %s:", class_str);
@@ -2012,14 +2014,14 @@ static void usb_info_device(Monitor *mon, int bus_num, int addr, int class_id,
 }
 
 static int usb_host_info_device(void *opaque, int bus_num, int addr,
-                                int devpath, int class_id,
+                                char *path, int class_id,
                                 int vendor_id, int product_id,
                                 const char *product_name,
                                 int speed)
 {
     Monitor *mon = opaque;
 
-    usb_info_device(mon, bus_num, addr, class_id, vendor_id, product_id,
+    usb_info_device(mon, bus_num, addr, path, class_id, vendor_id, product_id,
                     product_name, speed);
     return 0;
 }
-- 
1.7.1

[Qemu-devel] [PATCH 2/2] usb-linux: add hostport property

[Gerd Hoffmann]

This patch adds a hostport property which allows to specify the host usb
devices to pass through by bus number and physical port.  This means you
can basically hand over one (or more) of the usb plugs on your host to
the guest and whatever device is plugged in there will show up in the
guest.

Usage:

  -device usb-host,hostbus=1,hostport=1

You can figure the port numbers by plugging in some usb device, then
find it in "info usbhost" and pick bus and port specified there.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 usb-linux.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index 9543a69..5f50767 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -135,6 +135,7 @@ struct ctrl_struct {
 struct USBAutoFilter {
     uint32_t bus_num;
     uint32_t addr;
+    char     *port;
     uint32_t vendor_id;
     uint32_t product_id;
 };
@@ -1416,6 +1417,7 @@ static struct USBDeviceInfo usb_host_dev_info = {
     .qdev.props     = (Property[]) {
         DEFINE_PROP_UINT32("hostbus",  USBHostDevice, match.bus_num,    0),
         DEFINE_PROP_UINT32("hostaddr", USBHostDevice, match.addr,       0),
+        DEFINE_PROP_STRING("hostport", USBHostDevice, match.port),
         DEFINE_PROP_HEX32("vendorid",  USBHostDevice, match.vendor_id,  0),
         DEFINE_PROP_HEX32("productid", USBHostDevice, match.product_id, 0),
         DEFINE_PROP_END_OF_LIST(),
@@ -1838,6 +1840,9 @@ static int usb_host_auto_scan(void *opaque, int bus_num, int addr, char *port,
         if (f->addr > 0 && f->addr != addr) {
             continue;
         }
+        if (f->port != NULL && (port == NULL || strcmp(f->port, port) != 0)) {
+            continue;
+        }
 
         if (f->vendor_id > 0 && f->vendor_id != vendor_id) {
             continue;
@@ -2063,7 +2068,7 @@ void usb_host_info(Monitor *mon)
         dec2str(f->addr, addr, sizeof(addr));
         hex2str(f->vendor_id, vid, sizeof(vid));
         hex2str(f->product_id, pid, sizeof(pid));
-        monitor_printf(mon, "    Device %s.%s ID %s:%s\n",
-                       bus, addr, vid, pid);
+        monitor_printf(mon, "    Bus %s, Addr %s, Port %s, ID %s:%s\n",
+                       bus, addr, f->port ? f->port : "*", vid, pid);
     }
 }
-- 
1.7.1

Re: [Qemu-devel] [PATCH 0/2] usb-linux: physical port handling.

[Brad Hards]

On Tuesday 10 May 2011 12:30:41 Gerd Hoffmann wrote:
>   Hi,
> 
> These patches fix and improve the physical port handling in the usb host
> driver.  Passthrough of devices connected via usb hub should work better
> now.  Also you can specify usb devices for passthrough by physical port
> (on the host) now.
> 
> please review,
>   Gerd
> 
> Gerd Hoffmann (2):
>   usb-linux: fix device path aka physical port handling
>   usb-linux: add hostport property
> 
>  usb-linux.c |   47 +++++++++++++++++++++++++++--------------------
>  1 files changed, 27 insertions(+), 20 deletions(-)
Some additional docs (based on 2/2 hints) might be a useful addition.
Possible candidates:
- docs/qdev-device-use.txt
- qemu-doc.texi

Brad

Re: [Qemu-devel] [PATCH 1/2] usb-linux: fix device path aka physical port handling

[Markus Armbruster]

Good stuff, just a few questions.

Gerd Hoffmann <kraxel@redhat.com> writes:

> The device path isn't just a number.  It specifies the physical port
> the device is connected to and in case the device is connected via
> usb hub you'll have two numbers there, like this: "5.1".  The first
> specifies the root port where the hub is plugged into, the second
> specifies the port number of the hub where the device is plugged in.
> With multiple hubs chained the string can become longer.

"5.1"?  Do you mean "5-1"?

I think you're talking about the file names in /sys/bus/usb/devices.
Kernel names them in drivers/usb/core/usb.c's usb_alloc_dev().

Roots:
		dev->devpath[0] = '0';
		dev_set_name(&dev->dev, "usb%d", bus->busnum);

Hubs:
		if (parent->devpath[0] == '0') {
			snprintf(dev->devpath, sizeof dev->devpath,
				"%d", port1);
		} else {
			snprintf(dev->devpath, sizeof dev->devpath,
				"%s.%d", parent->devpath, port1);
		}
		dev_set_name(&dev->dev, "%d-%s", bus->busnum, dev->devpath);

On my system:

$ ls /sys/bus/usb/devices
1-0:1.0  2-0:1.0  3-0:1.0  4-0:1.0  5-0:1.0  usb1  usb2  usb3  usb4  usb5

> This patch renames devpath to port and makes it a string.   It also
> adapts the sysfs parsing code accordingly.  The "info usbhost" monitor
> command now prints bus number, (os-assigned) device address and physical
> port for each device.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  usb-linux.c |   38 ++++++++++++++++++++------------------
>  1 files changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/usb-linux.c b/usb-linux.c
> index 72fe6f5..9543a69 100644
> --- a/usb-linux.c
> +++ b/usb-linux.c
> @@ -62,7 +62,7 @@ struct usb_ctrlrequest {
>      uint16_t wLength;
>  };
>  
> -typedef int USBScanFunc(void *opaque, int bus_num, int addr, int devpath,
> +typedef int USBScanFunc(void *opaque, int bus_num, int addr, char *port,
>                          int class_id, int vendor_id, int product_id,
>                          const char *product_name, int speed);
>  
> @@ -79,6 +79,7 @@ typedef int USBScanFunc(void *opaque, int bus_num, int addr, int devpath,
>  #define USBPROCBUS_PATH "/proc/bus/usb"
>  #define PRODUCT_NAME_SZ 32
>  #define MAX_ENDPOINTS 15
> +#define MAX_PORTLEN 8

Where does 8 come from?  For what it's worth, kernel's struct usb_device
has char devpath[16].

>  #define USBDEVBUS_PATH "/dev/bus/usb"
>  #define USBSYSBUS_PATH "/sys/bus/usb"
>  
> @@ -155,7 +156,7 @@ typedef struct USBHostDevice {
>      /* Host side address */
>      int bus_num;
>      int addr;
> -    int devpath;
> +    char port[MAX_PORTLEN];
>      struct USBAutoFilter match;
>  
>      QTAILQ_ENTRY(USBHostDevice) next;
> @@ -1092,7 +1093,7 @@ static int usb_linux_get_configuration(USBHostDevice *s)
>          char device_name[32], line[1024];
>          int configuration;
>  
> -        sprintf(device_name, "%d-%d", s->bus_num, s->devpath);
> +        sprintf(device_name, "%d-%s", s->bus_num, s->port);
>  
>          if (!usb_host_read_file(line, sizeof(line), "bConfigurationValue",
>                                  device_name)) {
> @@ -1138,7 +1139,7 @@ static uint8_t usb_linux_get_alt_setting(USBHostDevice *s,
>          char device_name[64], line[1024];
>          int alt_setting;
>  
> -        sprintf(device_name, "%d-%d:%d.%d", s->bus_num, s->devpath,
> +        sprintf(device_name, "%d-%s:%d.%d", s->bus_num, s->port,
>                  (int)configuration, (int)interface);
>  
>          if (!usb_host_read_file(line, sizeof(line), "bAlternateSetting",
> @@ -1257,7 +1258,7 @@ static int usb_linux_update_endp_table(USBHostDevice *s)
>  }
>  
>  static int usb_host_open(USBHostDevice *dev, int bus_num,
> -                         int addr, int devpath, const char *prod_name)
> +                         int addr, char *port, const char *prod_name)
>  {
>      int fd = -1, ret;
>      struct usbdevfs_connectinfo ci;
> @@ -1283,7 +1284,7 @@ static int usb_host_open(USBHostDevice *dev, int bus_num,
>  
>      dev->bus_num = bus_num;
>      dev->addr = addr;
> -    dev->devpath = devpath;
> +    strcpy(dev->port, port);
>      dev->fd = fd;
>  
>      /* read the device description */
> @@ -1655,8 +1656,9 @@ static int usb_host_scan_sys(void *opaque, USBScanFunc *func)
>  {
>      DIR *dir = NULL;
>      char line[1024];
> -    int bus_num, addr, devpath, speed, class_id, product_id, vendor_id;
> +    int bus_num, addr, speed, class_id, product_id, vendor_id;
>      int ret = 0;
> +    char port[MAX_PORTLEN];
>      char product_name[512];
>      struct dirent *de;
>  
> @@ -1672,8 +1674,8 @@ static int usb_host_scan_sys(void *opaque, USBScanFunc *func)
>              if (!strncmp(de->d_name, "usb", 3)) {
>                  tmpstr += 3;
>              }

Sloppy parsing, but that's not your fault.

> -            if (sscanf(tmpstr, "%d-%d", &bus_num, &devpath) < 1) {
> -                goto the_end;
> +            if (sscanf(tmpstr, "%d-%7[0-9.]", &bus_num, port) < 2) {
> +                continue;
>              }
>  
>              if (!usb_host_read_file(line, sizeof(line), "devnum", de->d_name)) {

The old sscanf() succeeds if at least one item is assigned, i.e. tmpstr
starts with an integer.  I suspect this is broken for roots.  Consider
d_name "usb1": tmpstr is "1", sscan() returns 1, and devpath remains
uninitialized.  It's passed to the func() callback.  Bug?  If yes, the
commit message should mention it.

The new sscan() succeeds only if both items are assigned, i.e. tmpstr
starts with an integer, '-', and up to 7 of [0-9.].  Unlike the old one,
it fails for roots.  Intentional?

[...]

Re: [Qemu-devel] [PATCH 1/2] usb-linux: fix device path aka physical port handling

[Gerd Hoffmann]

On 05/11/11 10:52, Markus Armbruster wrote:
> Good stuff, just a few questions.
>
> Gerd Hoffmann<kraxel@redhat.com>  writes:
>
>> The device path isn't just a number.  It specifies the physical port
>> the device is connected to and in case the device is connected via
>> usb hub you'll have two numbers there, like this: "5.1".  The first
>> specifies the root port where the hub is plugged into, the second
>> specifies the port number of the hub where the device is plugged in.
>> With multiple hubs chained the string can become longer.
>
> "5.1"?  Do you mean "5-1"?

No.

> 			snprintf(dev->devpath, sizeof dev->devpath,
> 				"%s.%d", parent->devpath, port1);
                                  ^^^^^

> $ ls /sys/bus/usb/devices
> 1-0:1.0  2-0:1.0  3-0:1.0  4-0:1.0  5-0:1.0  usb1  usb2  usb3  usb4  usb5

Boring, nothing plugged in here ;)

Laptop docking stations often have a usb hub built in.  If you have one 
place your laptop there, then connect a usb device to one of the ports 
of the docking station.  You'll see a file named like this:

   1-5.3

where "1" is the bus number, "5" is the port number of the root hub and 
"3" is the port number of the docking station hub.

>> @@ -79,6 +79,7 @@ typedef int USBScanFunc(void *opaque, int bus_num, int addr, int devpath,
>>   #define USBPROCBUS_PATH "/proc/bus/usb"
>>   #define PRODUCT_NAME_SZ 32
>>   #define MAX_ENDPOINTS 15
>> +#define MAX_PORTLEN 8
>
> Where does 8 come from?

Pulled out of thin air.  Should be enougth, you can build chains with 
three usb hubs (anyone ever did in practice? did the devices still 
work?), getting port addresses like "1.2.3.4", and it still fits in.

> For what it's worth, kernel's struct usb_device
> has char devpath[16].

We can make that 16 too to be on the really safe side.

>> @@ -1672,8 +1674,8 @@ static int usb_host_scan_sys(void *opaque, USBScanFunc *func)
>>               if (!strncmp(de->d_name, "usb", 3)) {
>>                   tmpstr += 3;
>>               }
>
> Sloppy parsing, but that's not your fault.

I think this can be zapped now, the new sscanf will fail on them and 
skip the entries anyway.

>> -            if (sscanf(tmpstr, "%d-%d",&bus_num,&devpath)<  1) {
>> -                goto the_end;
>> +            if (sscanf(tmpstr, "%d-%7[0-9.]",&bus_num, port)<  2) {
>> +                continue;
>>               }
>>
>>               if (!usb_host_read_file(line, sizeof(line), "devnum", de->d_name)) {
>
> The old sscanf() succeeds if at least one item is assigned, i.e. tmpstr
> starts with an integer.  I suspect this is broken for roots.  Consider
> d_name "usb1": tmpstr is "1", sscan() returns 1, and devpath remains
> uninitialized.  It's passed to the func() callback.  Bug?  If yes, the
> commit message should mention it.

Indeed.

> The new sscan() succeeds only if both items are assigned, i.e. tmpstr
> starts with an integer, '-', and up to 7 of [0-9.].  Unlike the old one,
> it fails for roots.  Intentional?

Yes, now the roots are skipped.  You can't assign them anyway, so it is 
pretty pointless to loom at them and to list them in "info usbhost".

cheers,
   Gerd

[Qemu-devel] qdev device documentation (Re: [PATCH 0/2] usb-linux: physical port handling.)

[Gerd Hoffmann]

   Hi,

> Some additional docs (based on 2/2 hints) might be a useful addition.

Indeed.

> Possible candidates:
> - docs/qdev-device-use.txt
> - qemu-doc.texi

Hmm.

qemu-doc.texi only documents the pre-qdev way to create devices.
qdev-device-use.txt is a conversion guide.

devices which can only be created via -device (or provide additional 
features via qdev attributes when created that way) are not documented 
anywhere now.

Suggestions?  Should we add a qdev section to qemu-doc.texi?  Or better 
a separate qemu-devices.txt (or .texi)?

What is the status of the qdev documentation patches btw.?  Maybe we 
should just merge them, then start filling stuff and maybe also 
autogenerate documentation from that?

cheers,
   Gerd

Re: [Qemu-devel] qdev device documentation (Re: [PATCH 0/2] usb-linux: physical port handling.)

[Markus Armbruster]

[note cc: Amit]

Gerd Hoffmann <kraxel@redhat.com> writes:

>   Hi,
>
>> Some additional docs (based on 2/2 hints) might be a useful addition.
>
> Indeed.
>
>> Possible candidates:
>> - docs/qdev-device-use.txt
>> - qemu-doc.texi
>
> Hmm.
>
> qemu-doc.texi only documents the pre-qdev way to create devices.
> qdev-device-use.txt is a conversion guide.
>
> devices which can only be created via -device (or provide additional
> features via qdev attributes when created that way) are not documented
> anywhere now.
>
> Suggestions?  Should we add a qdev section to qemu-doc.texi?  Or
> better a separate qemu-devices.txt (or .texi)?
>
> What is the status of the qdev documentation patches btw.?

http://lists.gnu.org/archive/html/qemu-devel/2011-02/msg02169.html

>                                                             Maybe we
> should just merge them, then start filling stuff and maybe also
> autogenerate documentation from that?

Yes.

Amit, once more into the breach?

Re: [Qemu-devel] qdev device documentation (Re: [PATCH 0/2] usb-linux: physical port handling.)

[Gerd Hoffmann]

   Hi,

>> What is the status of the qdev documentation patches btw.?
>
> http://lists.gnu.org/archive/html/qemu-devel/2011-02/msg02169.html

What is the problem with the empty strings btw?

The only way around I can see is having _DOC and _NODOC versions for all 
the property macros, but I'd prefer to not have _NODOC macros in the 
tree ...

cheers,
   Gerd

Re: [Qemu-devel] qdev device documentation (Re: [PATCH 0/2] usb-linux: physical port handling.)

[Anthony Liguori]

On 05/12/2011 10:25 AM, Gerd Hoffmann wrote:
> Hi,
>
>>> What is the status of the qdev documentation patches btw.?
>>
>> http://lists.gnu.org/archive/html/qemu-devel/2011-02/msg02169.html
>
> What is the problem with the empty strings btw?
>
> The only way around I can see is having _DOC and _NODOC versions for all
> the property macros, but I'd prefer to not have _NODOC macros in the
> tree ...

Inline documentation is bad.  Our documentation should be centralized. 
  That's the only way to keep it consistent and thorough.

There's no way to easily extract the inline docs in a complete way since 
some devices are built conditionally.

Regards,

Anthony Liguori

>
> cheers,
> Gerd
>
>

Re: [Qemu-devel] qdev device documentation (Re: [PATCH 0/2] usb-linux: physical port handling.)

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

>   Hi,
>
>>> What is the status of the qdev documentation patches btw.?
>>
>> http://lists.gnu.org/archive/html/qemu-devel/2011-02/msg02169.html
>
> What is the problem with the empty strings btw?

Anthony ;)

[...]

Re: [Qemu-devel] qdev device documentation (Re: [PATCH 0/2] usb-linux: physical port handling.)

[Anthony Liguori]

[-- Attachment #1: Type: text/plain, Size: 651 bytes --]

On 05/12/2011 10:25 AM, Gerd Hoffmann wrote:
> Hi,
>
>>> What is the status of the qdev documentation patches btw.?
>>
>> http://lists.gnu.org/archive/html/qemu-devel/2011-02/msg02169.html
>
> What is the problem with the empty strings btw?
>
> The only way around I can see is having _DOC and _NODOC versions for all
> the property macros, but I'd prefer to not have _NODOC macros in the
> tree ...

Here's an example of what I'm suggesting.  I think we should just go 
with this and add better output as we go.

But we need all of the qdev information..  not just a doc string for 
each property.

Regards,

Anthony Liguori

>
> cheers,
> Gerd
>
>


[-- Attachment #2: 0001-qdev-add-centralized-documentation-for-qdev.patch --]
[-- Type: text/x-patch, Size: 2528 bytes --]

>From 130c817790880c61b79dbccf66f5863c406eb7d4 Mon Sep 17 00:00:00 2001
From: Anthony Liguori <aliguori@us.ibm.com>
Date: Thu, 12 May 2011 10:56:29 -0500
Subject: [PATCH] qdev: add centralized documentation for qdev

This is mostly a proof-of-concept.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/Makefile b/Makefile
index 2b0438c..fddb261 100644
--- a/Makefile
+++ b/Makefile
@@ -341,5 +341,7 @@ tarbin:
 	$(mandir)/man1/qemu-img.1 \
 	$(mandir)/man8/qemu-nbd.8
 
+include $(SRC_PATH)/Makefile.doc
+
 # Include automatically generated dependency files
 -include $(wildcard *.d audio/*.d slirp/*.d block/*.d net/*.d ui/*.d)
diff --git a/Makefile.doc b/Makefile.doc
new file mode 100644
index 0000000..f769b23
--- /dev/null
+++ b/Makefile.doc
@@ -0,0 +1,2 @@
+qdev-doc.html: $(SRC_PATH)/qdev-doc.json
+	python $(SRC_PATH)/scripts/qdev-doc-to-html.py < $< > $@
diff --git a/qdev-doc.json b/qdev-doc.json
new file mode 100644
index 0000000..c24630b
--- /dev/null
+++ b/qdev-doc.json
@@ -0,0 +1,14 @@
+# -*- Mode: Python -*-
+
+[ { "device": "isa-serial",
+    "parent": "ISADevice",
+    "properties": {
+            "index": { "type": "uint32",
+                       "doc": "A value from 0-3 that describes which IO regions to expose the device on.  This sets appropriate values for iobase and irq." },
+            "iobase": { "type": "hex32",
+                        "doc": "The base IO address to expose the device on." },
+            "irq": { "type": "uint32",
+                     "doc": "The IRQ to use for the device." },
+            "chardev": { "type": "chr",
+                         "doc": "The name of a character device to specify." } } }
+  ]
diff --git a/scripts/qdev-doc-to-html.py b/scripts/qdev-doc-to-html.py
new file mode 100644
index 0000000..a25fe35
--- /dev/null
+++ b/scripts/qdev-doc-to-html.py
@@ -0,0 +1,40 @@
+#!/usr/bin/python
+
+import sys
+
+data = sys.stdin.read()
+
+docs = eval(data)
+
+sys.stdout.write('''
+<html>
+<head>
+<title>QEMU device documentation</title>
+</head>
+<body>
+''')
+
+for item in docs:
+    sys.stdout.write('''
+<h2>%(device)s :: %(parent)s</h2>
+
+<table border="1">
+<tr>
+<th>Name</th><th>Type</th><th>Comments</th>
+</tr>
+''' % item)
+    for prop in item["properties"]:
+        sys.stdout.write('''
+<tr>
+<td>%s</td><td>%s</td><td>%s</td>
+</tr>
+''' % (prop, item["properties"][prop]['type'], item["properties"][prop]['doc']))
+
+    sys.stdout.write('''
+</table>
+''')
+
+sys.stdout.write('''
+</body>
+</html>
+''')
-- 
1.7.4.1

Re: [Qemu-devel] qdev device documentation (Re: [PATCH 0/2] usb-linux: physical port handling.)

[Anthony Liguori]

On 05/12/2011 10:56 AM, Markus Armbruster wrote:
> Gerd Hoffmann<kraxel@redhat.com>  writes:
>
>>    Hi,
>>
>>>> What is the status of the qdev documentation patches btw.?
>>>
>>> http://lists.gnu.org/archive/html/qemu-devel/2011-02/msg02169.html
>>
>> What is the problem with the empty strings btw?
>
> Anthony ;)

1) It doesn't help us get good written documentation without 
non-existent magic to extract a qdev schema.

2) It makes the documentation hidden in random places in the tree making 
it hard to audit what is undocumented.

3) It guarantees that documentation will vary drastically in language 
and quality.

4) I greatly fear that we're embarking down yet another "introduce a new 
API that never gets fully converted" path.

I've sent a quick patch out with what I think is a significantly better 
approach.

Regards,

Anthony Liguori

>
> [...]
>

Re: [Qemu-devel] qdev device documentation (Re: [PATCH 0/2] usb-linux: physical port handling.)

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> On 05/12/2011 10:25 AM, Gerd Hoffmann wrote:
>> Hi,
>>
>>>> What is the status of the qdev documentation patches btw.?
>>>
>>> http://lists.gnu.org/archive/html/qemu-devel/2011-02/msg02169.html
>>
>> What is the problem with the empty strings btw?
>>
>> The only way around I can see is having _DOC and _NODOC versions for all
>> the property macros, but I'd prefer to not have _NODOC macros in the
>> tree ...
>
> Inline documentation is bad.  Our documentation should be
> centralized. That's the only way to keep it consistent and thorough.

External documentation of code details is bad.  Our documentation should
be next to the code.  That's the only way to keep it up-to-date and
consistent with the code.

> There's no way to easily extract the inline docs in a complete way
> since some devices are built conditionally.

For each configured target: extract docs of the devices it builds
Concatenate and discard the duplicates

Yes, that means you don't get docs for devices none of your targets has.
That's a feature.  If you really want docs for all devices, build all
targets.

Re: [Qemu-devel] qdev device documentation (Re: [PATCH 0/2] usb-linux: physical port handling.)

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> On 05/12/2011 10:25 AM, Gerd Hoffmann wrote:
>> Hi,
>>
>>>> What is the status of the qdev documentation patches btw.?
>>>
>>> http://lists.gnu.org/archive/html/qemu-devel/2011-02/msg02169.html
>>
>> What is the problem with the empty strings btw?
>>
>> The only way around I can see is having _DOC and _NODOC versions for all
>> the property macros, but I'd prefer to not have _NODOC macros in the
>> tree ...
>
> Here's an example of what I'm suggesting.  I think we should just go
> with this and add better output as we go.
>
> But we need all of the qdev information..  not just a doc string for
> each property.

Missing: make "device_add ?" show your device doc strings, and
"device_add NAME,?" show your property doc strings.

Missing: automated check qdev-doc.json matches the code.  Keeping the
docs far from the code is a bad idea even with such a check.

[...]

Re: [Qemu-devel] qdev device documentation (Re: [PATCH 0/2] usb-linux: physical port handling.)

[Anthony Liguori]

On 05/12/2011 11:08 AM, Markus Armbruster wrote:
> Anthony Liguori<anthony@codemonkey.ws>  writes:
>
>> On 05/12/2011 10:25 AM, Gerd Hoffmann wrote:
>>> Hi,
>>>
>>>>> What is the status of the qdev documentation patches btw.?
>>>>
>>>> http://lists.gnu.org/archive/html/qemu-devel/2011-02/msg02169.html
>>>
>>> What is the problem with the empty strings btw?
>>>
>>> The only way around I can see is having _DOC and _NODOC versions for all
>>> the property macros, but I'd prefer to not have _NODOC macros in the
>>> tree ...
>>
>> Inline documentation is bad.  Our documentation should be
>> centralized. That's the only way to keep it consistent and thorough.
>
> External documentation of code details is bad.  Our documentation should
> be next to the code.  That's the only way to keep it up-to-date and
> consistent with the code.

qdev properties are *not* code details.  It's a public user interface 
that we have to support for every.

It should be disconnected from the internal implementation.  And yes, 
the incestuous relationship that exists today is a problem, but it's one 
we're going to have to live with.

>
>> There's no way to easily extract the inline docs in a complete way
>> since some devices are built conditionally.
>
> For each configured target: extract docs of the devices it builds
> Concatenate and discard the duplicates
>
> Yes, that means you don't get docs for devices none of your targets has.
> That's a feature.  If you really want docs for all devices, build all
> targets.

But for things like Spice where the lack of libspice influences whether 
the device is available, how do I extract formal documentation to 
publish on qemu.org reliably?

Regards,

Anthony Liguori

>

Re: [Qemu-devel] qdev device documentation (Re: [PATCH 0/2] usb-linux: physical port handling.)

[Anthony Liguori]

On 05/12/2011 11:18 AM, Markus Armbruster wrote:
> Anthony Liguori<anthony@codemonkey.ws>  writes:
>
>> On 05/12/2011 10:25 AM, Gerd Hoffmann wrote:
>>> Hi,
>>>
>>>>> What is the status of the qdev documentation patches btw.?
>>>>
>>>> http://lists.gnu.org/archive/html/qemu-devel/2011-02/msg02169.html
>>>
>>> What is the problem with the empty strings btw?
>>>
>>> The only way around I can see is having _DOC and _NODOC versions for all
>>> the property macros, but I'd prefer to not have _NODOC macros in the
>>> tree ...
>>
>> Here's an example of what I'm suggesting.  I think we should just go
>> with this and add better output as we go.
>>
>> But we need all of the qdev information..  not just a doc string for
>> each property.
>
> Missing: make "device_add ?" show your device doc strings, and
> "device_add NAME,?" show your property doc strings.

I really dislike overloading things for inline help.  Introducing a 
device_help is fine and hopefully you realize that generating this is 
trivial.

>
> Missing: automated check qdev-doc.json matches the code.  Keeping the
> docs far from the code is a bad idea even with such a check.

I view this as a feature.  What's documented is what's supported. 
Anything that isn't documented isn't supported.

But yes, hopefully it's obvious that we can add automated checks to 
improve this.

But if we're going to start somewhere, this is where I think we should 
start.

Regards,

Anthony Liguori

> [...]
>

Re: [Qemu-devel] qdev device documentation (Re: [PATCH 0/2] usb-linux: physical port handling.)

[Anthony Liguori]

On 05/12/2011 11:18 AM, Markus Armbruster wrote:
> Anthony Liguori<anthony@codemonkey.ws>  writes:
>
>> On 05/12/2011 10:25 AM, Gerd Hoffmann wrote:
>>> Hi,
>>>
>>>>> What is the status of the qdev documentation patches btw.?
>>>>
>>>> http://lists.gnu.org/archive/html/qemu-devel/2011-02/msg02169.html
>>>
>>> What is the problem with the empty strings btw?
>>>
>>> The only way around I can see is having _DOC and _NODOC versions for all
>>> the property macros, but I'd prefer to not have _NODOC macros in the
>>> tree ...
>>
>> Here's an example of what I'm suggesting.  I think we should just go
>> with this and add better output as we go.
>>
>> But we need all of the qdev information..  not just a doc string for
>> each property.
>
> Missing: make "device_add ?" show your device doc strings, and
> "device_add NAME,?" show your property doc strings.

This is all it takes:

#!/usr/bin/python

import sys

data = sys.stdin.read()

docs = eval(data)

sys.stdout.write('DeviceStateDocumentation device_docs[] = {')

for item in docs:
     sys.stdout.write('''
     {
       .name = "%(device)s",
       .properties = (PropertyDocumentation[])({''' % item)
     for prop in item["properties"]:
         sys.stdout.write('''
         { "%s", "%s", "%s" },''' % (prop, 
item["properties"][prop]['type'], item["properties"][prop]['doc']))

     sys.stdout.write('''
         { },
     },''')

sys.stdout.write('''
};
''')

Plus a little plumbing magic to add the actual command.

> Missing: automated check qdev-doc.json matches the code.  Keeping the
> docs far from the code is a bad idea even with such a check.

If you walk the DeviceInfo list, you can validate that (1) each device 
has a corresponding entry in device_docs (2) any field in device_docs is 
present in device (3) the types match.

Regards,

Anthony Liguori

> [...]
>

Re: [Qemu-devel] qdev device documentation (Re: [PATCH 0/2] usb-linux: physical port handling.)

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> On 05/12/2011 11:08 AM, Markus Armbruster wrote:
>> Anthony Liguori<anthony@codemonkey.ws>  writes:
>>
>>> On 05/12/2011 10:25 AM, Gerd Hoffmann wrote:
>>>> Hi,
>>>>
>>>>>> What is the status of the qdev documentation patches btw.?
>>>>>
>>>>> http://lists.gnu.org/archive/html/qemu-devel/2011-02/msg02169.html
>>>>
>>>> What is the problem with the empty strings btw?
>>>>
>>>> The only way around I can see is having _DOC and _NODOC versions for all
>>>> the property macros, but I'd prefer to not have _NODOC macros in the
>>>> tree ...
>>>
>>> Inline documentation is bad.  Our documentation should be
>>> centralized. That's the only way to keep it consistent and thorough.
>>
>> External documentation of code details is bad.  Our documentation should
>> be next to the code.  That's the only way to keep it up-to-date and
>> consistent with the code.
>
> qdev properties are *not* code details.  It's a public user interface
> that we have to support for every.

The fact that they are public user interface doesn't change the fact
that they're code at all.  They are *both*.

> It should be disconnected from the internal implementation.  And yes,
> the incestuous relationship that exists today is a problem, but it's
> one we're going to have to live with.

I doubt we'll ever reach consensus on this one.

>>> There's no way to easily extract the inline docs in a complete way
>>> since some devices are built conditionally.
>>
>> For each configured target: extract docs of the devices it builds
>> Concatenate and discard the duplicates
>>
>> Yes, that means you don't get docs for devices none of your targets has.
>> That's a feature.  If you really want docs for all devices, build all
>> targets.
>
> But for things like Spice where the lack of libspice influences
> whether the device is available, how do I extract formal documentation
> to publish on qemu.org reliably?

If no maintainer of QEMU can build with Spice enabled, it got more
serious problems than extracting its documentation.

Re: [Qemu-devel] qdev device documentation (Re: [PATCH 0/2] usb-linux: physical port handling.)

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> On 05/12/2011 11:18 AM, Markus Armbruster wrote:
>> Anthony Liguori<anthony@codemonkey.ws>  writes:
>>
>>> On 05/12/2011 10:25 AM, Gerd Hoffmann wrote:
>>>> Hi,
>>>>
>>>>>> What is the status of the qdev documentation patches btw.?
>>>>>
>>>>> http://lists.gnu.org/archive/html/qemu-devel/2011-02/msg02169.html
>>>>
>>>> What is the problem with the empty strings btw?
>>>>
>>>> The only way around I can see is having _DOC and _NODOC versions for all
>>>> the property macros, but I'd prefer to not have _NODOC macros in the
>>>> tree ...
>>>
>>> Here's an example of what I'm suggesting.  I think we should just go
>>> with this and add better output as we go.
>>>
>>> But we need all of the qdev information..  not just a doc string for
>>> each property.
>>
>> Missing: make "device_add ?" show your device doc strings, and
>> "device_add NAME,?" show your property doc strings.
>
> I really dislike overloading things for inline help.  Introducing a
> device_help is fine and hopefully you realize that generating this is
> trivial.

I never liked "device_add ?" myself, but it's what we got.  I'd support
replacing it with something more decent.

[...]

Re: [Qemu-devel] qdev device documentation (Re: [PATCH 0/2] usb-linux: physical port handling.)

[Anthony Liguori]

On 05/12/2011 12:58 PM, Markus Armbruster wrote:
> Anthony Liguori<anthony@codemonkey.ws>  writes:
>
>> On 05/12/2011 11:08 AM, Markus Armbruster wrote:
>>> Anthony Liguori<anthony@codemonkey.ws>   writes:
>>>
>>>> On 05/12/2011 10:25 AM, Gerd Hoffmann wrote:
>>>>> Hi,
>>>>>
>>>>>>> What is the status of the qdev documentation patches btw.?
>>>>>>
>>>>>> http://lists.gnu.org/archive/html/qemu-devel/2011-02/msg02169.html
>>>>>
>>>>> What is the problem with the empty strings btw?
>>>>>
>>>>> The only way around I can see is having _DOC and _NODOC versions for all
>>>>> the property macros, but I'd prefer to not have _NODOC macros in the
>>>>> tree ...
>>>>
>>>> Inline documentation is bad.  Our documentation should be
>>>> centralized. That's the only way to keep it consistent and thorough.
>>>
>>> External documentation of code details is bad.  Our documentation should
>>> be next to the code.  That's the only way to keep it up-to-date and
>>> consistent with the code.
>>
>> qdev properties are *not* code details.  It's a public user interface
>> that we have to support for every.
>
> The fact that they are public user interface doesn't change the fact
> that they're code at all.  They are *both*.

That's fine.  But what better way to ensure a consistent and stable UI 
than having it centralized in one place.

>> It should be disconnected from the internal implementation.  And yes,
>> the incestuous relationship that exists today is a problem, but it's
>> one we're going to have to live with.
>
> I doubt we'll ever reach consensus on this one.

I don't think that matters though.  qdev properties are part of our UI 
and need to be stable.

Just like we express our command line options centrally (with 
documentation) via qemu-options.hx, it seems reasonable to me to do it 
for qdev properties.

I think the only downside is that we have to duplicate this the current 
DeviceInfo definitions but that's a harder problem that can be deferred 
for another day.

>>> Yes, that means you don't get docs for devices none of your targets has.
>>> That's a feature.  If you really want docs for all devices, build all
>>> targets.
>>
>> But for things like Spice where the lack of libspice influences
>> whether the device is available, how do I extract formal documentation
>> to publish on qemu.org reliably?
>
> If no maintainer of QEMU can build with Spice enabled, it got more
> serious problems than extracting its documentation.

It's not about Spice.  But having documentation that is influenced by 
build options seems like a bad thing to me.

But regardless, centralized documentation means more consistency in the 
documentation.  I think this is a critically important point.

Regards,

Anthony Liguori

Re: [Qemu-devel] qdev device documentation (Re: [PATCH 0/2] usb-linux: physical port handling.)

[Peter Maydell]

On 12 May 2011 19:58, Markus Armbruster <armbru@redhat.com> wrote:
> Anthony Liguori <anthony@codemonkey.ws> writes:
>> But for things like Spice where the lack of libspice influences
>> whether the device is available, how do I extract formal documentation
>> to publish on qemu.org reliably?
>
> If no maintainer of QEMU can build with Spice enabled, it got more
> serious problems than extracting its documentation.

The point isn't that you can arrange to build with option Foo
enabled but that if the build environment changes accidentally
there's not much warning that the official docs have suddenly
lost some devices. The spice probe in configure will barf if you
said "--enable-spice" and spice wasn't found, but I bet not all
configure checks that influence availability of a device do that.
And since there isn't currently an '--enable-all' option to
configure the docs builder would have to keep track of every
new --enable-foo switch and add it to the configure command...

Which looks pretty ugly and not very reliable to me.

-- PMM

Re: [Qemu-devel] qdev device documentation (Re: [PATCH 0/2] usb-linux: physical port handling.)

[Alon Levy]

On Thu, May 12, 2011 at 08:15:43PM +0200, Peter Maydell wrote:
> On 12 May 2011 19:58, Markus Armbruster <armbru@redhat.com> wrote:
> > Anthony Liguori <anthony@codemonkey.ws> writes:
> >> But for things like Spice where the lack of libspice influences
> >> whether the device is available, how do I extract formal documentation
> >> to publish on qemu.org reliably?
> >
> > If no maintainer of QEMU can build with Spice enabled, it got more
> > serious problems than extracting its documentation.
> 
> The point isn't that you can arrange to build with option Foo
> enabled but that if the build environment changes accidentally
> there's not much warning that the official docs have suddenly
> lost some devices. The spice probe in configure will barf if you
> said "--enable-spice" and spice wasn't found, but I bet not all
> configure checks that influence availability of a device do that.
> And since there isn't currently an '--enable-all' option to
> configure the docs builder would have to keep track of every
> new --enable-foo switch and add it to the configure command...
> 
> Which looks pretty ugly and not very reliable to me.
> 

We could have a (just picking up the Spice example) spice-docs file
that was checked in and updated periodically by generation by the maintainer.
It's error prone, you could still update the source but forget to checkin the spice-docs
and get an old version, but at least it would still be auto-generated, and it would
not prevent the docs builder from working.

> -- PMM
> 

Re: [Qemu-devel] qdev device documentation (Re: [PATCH 0/2] usb-linux: physical port handling.)

[Anthony Liguori]

On 05/12/2011 02:32 PM, Alon Levy wrote:
> We could have a (just picking up the Spice example) spice-docs file
> that was checked in and updated periodically by generation by the maintainer.
> It's error prone, you could still update the source but forget to checkin the spice-docs
> and get an old version, but at least it would still be auto-generated, and it would
> not prevent the docs builder from working.

It's not just Spice though.  There are still a number of devices that 
only get built with certain targets.

Doing it right is actually fairly complicated.  It's far easier to use a 
central doc file, and then add mechanisms to ensure things don't get out 
of sync.

Regards,

Anthony Liguori

>
>> -- PMM
>>
>

Re: [Qemu-devel] qdev device documentation (Re: [PATCH 0/2] usb-linux: physical port handling.)

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 12 May 2011 19:58, Markus Armbruster <armbru@redhat.com> wrote:
>> Anthony Liguori <anthony@codemonkey.ws> writes:
>>> But for things like Spice where the lack of libspice influences
>>> whether the device is available, how do I extract formal documentation
>>> to publish on qemu.org reliably?
>>
>> If no maintainer of QEMU can build with Spice enabled, it got more
>> serious problems than extracting its documentation.
>
> The point isn't that you can arrange to build with option Foo
> enabled but that if the build environment changes accidentally
> there's not much warning that the official docs have suddenly
> lost some devices.

Fair point.  Compare against list of known devices.  That way, all you
keep at bit-rotting distance from code is the list of known devices.

[...]

Re: [Qemu-devel] qdev device documentation (Re: [PATCH 0/2] usb-linux: physical port handling.)

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> On 05/12/2011 12:58 PM, Markus Armbruster wrote:
>> Anthony Liguori<anthony@codemonkey.ws>  writes:
>>
>>> On 05/12/2011 11:08 AM, Markus Armbruster wrote:
>>>> Anthony Liguori<anthony@codemonkey.ws>   writes:
>>>>
>>>>> On 05/12/2011 10:25 AM, Gerd Hoffmann wrote:
>>>>>> Hi,
>>>>>>
>>>>>>>> What is the status of the qdev documentation patches btw.?
>>>>>>>
>>>>>>> http://lists.gnu.org/archive/html/qemu-devel/2011-02/msg02169.html
>>>>>>
>>>>>> What is the problem with the empty strings btw?
>>>>>>
>>>>>> The only way around I can see is having _DOC and _NODOC versions for all
>>>>>> the property macros, but I'd prefer to not have _NODOC macros in the
>>>>>> tree ...
>>>>>
>>>>> Inline documentation is bad.  Our documentation should be
>>>>> centralized. That's the only way to keep it consistent and thorough.
>>>>
>>>> External documentation of code details is bad.  Our documentation should
>>>> be next to the code.  That's the only way to keep it up-to-date and
>>>> consistent with the code.
>>>
>>> qdev properties are *not* code details.  It's a public user interface
>>> that we have to support for every.
>>
>> The fact that they are public user interface doesn't change the fact
>> that they're code at all.  They are *both*.
>
> That's fine.  But what better way to ensure a consistent and stable UI
> than having it centralized in one place.

Consistent, stable, and bit-rotten.  Unless you come up with a way to
catch bit-rot immediately.  That means on "make", not on "make docs".

Extra points if the error message points right to the offending source
line.

>>> It should be disconnected from the internal implementation.  And yes,
>>> the incestuous relationship that exists today is a problem, but it's
>>> one we're going to have to live with.
>>
>> I doubt we'll ever reach consensus on this one.
>
> I don't think that matters though.

No need to tell me; I'm well aware that when you have sufficiently
strong opinions, consensus doesn't matter ;-P

>                                     qdev properties are part of our UI
> and need to be stable.

Presenting them all to the user in a single document makes sense.
Doesn't follow we have to *write* the documentation in a single place.

Keeping property documentation in one place makes it soemwhat easier to
keep the properties consistent with each other across devices.

Property documentation next the device code implementing them makes it
easier to keep the documentation consistent with reality.  It also
serves as code documentation.

It's a trade off.  I prefer property documentation next to the code.  To
really get consistency across devices, you have to read the resulting
user document, anyway.

> Just like we express our command line options centrally (with
> documentation) via qemu-options.hx, it seems reasonable to me to do it
> for qdev properties.
>
> I think the only downside is that we have to duplicate this the
> current DeviceInfo definitions but that's a harder problem that can be
> deferred for another day.

Having to code stuff twice, once in C and once in JSON, is a pretty ugly
downside.

JSON is a fine data interchange format for programs, but neither a
programming language, nor a document markup language.

[...]

Re: [Qemu-devel] qdev device documentation (Re: [PATCH 0/2] usb-linux: physical port handling.)

[Anthony Liguori]

On 05/13/2011 02:35 AM, Markus Armbruster wrote:

>> That's fine.  But what better way to ensure a consistent and stable UI
>> than having it centralized in one place.
>
> Consistent, stable, and bit-rotten.  Unless you come up with a way to
> catch bit-rot immediately.  That means on "make", not on "make docs".

How about make check?

See the latest patch.  Here is the output from a bad doc file:


Warning: device `virtio-9p-pci' is undocumented
Warning: device `virtio-balloon-pci' is undocumented
Warning: device `virtio-serial-pci' is undocumented
Warning: device `virtio-net-pci' is undocumented
Warning: device `virtio-blk-pci' is undocumented
[ .. snip .. ]
Warning: device `x3130-upstream' is undocumented
Warning: device `xio3130-downstream' is undocumented
Error: device `isa-serial' has documented property `bad-property' with 
no definition
Warning: device `isa-parallel' is undocumented
Warning: device `isa-pit' is undocumented
[ .. snip .. ]
Warning: device `i440FX-xen' is undocumented
Warning: device `i440FX' is undocumented
Warning: device `i440FX-pcihost' is undocumented
Warning: device `vmport' is undocumented
Warning: device `ib700' is undocumented
Warning: device `isa-debugcon' is undocumented
Error: documented device `BadDevice' has no definition
91 warnings, 2 errors.


> Extra points if the error message points right to the offending source
> line.

Would need some macro magic in the qdev registration functions.  It's 
not outside the realm of possibility.

>>                                      qdev properties are part of our UI
>> and need to be stable.
>
> Presenting them all to the user in a single document makes sense.
> Doesn't follow we have to *write* the documentation in a single place.
>
> Keeping property documentation in one place makes it soemwhat easier to
> keep the properties consistent with each other across devices.
>
> Property documentation next the device code implementing them makes it
> easier to keep the documentation consistent with reality.  It also
> serves as code documentation.
>
> It's a trade off.  I prefer property documentation next to the code.  To
> really get consistency across devices, you have to read the resulting
> user document, anyway.

I see this evolving.  To me, this is the start of a qdev schema from 
which we can generate factory interfaces to each qdev device.

>> Just like we express our command line options centrally (with
>> documentation) via qemu-options.hx, it seems reasonable to me to do it
>> for qdev properties.
>>
>> I think the only downside is that we have to duplicate this the
>> current DeviceInfo definitions but that's a harder problem that can be
>> deferred for another day.
>
> Having to code stuff twice, once in C and once in JSON, is a pretty ugly
> downside.

At least I didn't submit it using XML ;-)

Regards,

Anthony Liguori


> JSON is a fine data interchange format for programs, but neither a
> programming language, nor a document markup language.
>
> [...]
>

Re: [Qemu-devel] qdev device documentation (Re: [PATCH 0/2] usb-linux: physical port handling.)

[Anthony Liguori]

[-- Attachment #1: Type: text/plain, Size: 496 bytes --]

On 05/13/2011 02:35 AM, Markus Armbruster wrote:
> Anthony Liguori<anthony@codemonkey.ws>  writes:
>
>> That's fine.  But what better way to ensure a consistent and stable UI
>> than having it centralized in one place.
>
> Consistent, stable, and bit-rotten.  Unless you come up with a way to
> catch bit-rot immediately.  That means on "make", not on "make docs".
>
> Extra points if the error message points right to the offending source
> line.

Er, hit send too fast.  Here's the next patch.

[-- Attachment #2: 0001-qdev-add-centralized-documentation-for-qdev-v2.patch --]
[-- Type: text/x-patch, Size: 9450 bytes --]

>From f5c30c922ce1cf755ef78887f4015c131d8d6841 Mon Sep 17 00:00:00 2001
From: Anthony Liguori <aliguori@us.ibm.com>
Date: Thu, 12 May 2011 10:56:29 -0500
Subject: [PATCH 1/1] qdev: add centralized documentation for qdev (v2)

This adds a -qdev-verify option that will confirm that the documentation matches
the code.  It returns a non-zero exit status if any errors are detected.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 Makefile                    |    2 +
 Makefile.doc                |    5 ++
 Makefile.objs               |    2 +-
 qdev-doc.h                  |   23 +++++++++++
 qdev-doc.json               |   16 ++++++++
 qemu-options.hx             |   10 +++++
 scripts/qdev-doc-to-c.py    |   30 ++++++++++++++
 scripts/qdev-doc-to-html.py |   40 +++++++++++++++++++
 vl.c                        |   89 +++++++++++++++++++++++++++++++++++++++++++
 9 files changed, 216 insertions(+), 1 deletions(-)
 create mode 100644 Makefile.doc
 create mode 100644 qdev-doc.h
 create mode 100644 qdev-doc.json
 create mode 100644 scripts/qdev-doc-to-c.py
 create mode 100644 scripts/qdev-doc-to-html.py

diff --git a/Makefile b/Makefile
index 2b0438c..fddb261 100644
--- a/Makefile
+++ b/Makefile
@@ -341,5 +341,7 @@ tarbin:
 	$(mandir)/man1/qemu-img.1 \
 	$(mandir)/man8/qemu-nbd.8
 
+include $(SRC_PATH)/Makefile.doc
+
 # Include automatically generated dependency files
 -include $(wildcard *.d audio/*.d slirp/*.d block/*.d net/*.d ui/*.d)
diff --git a/Makefile.doc b/Makefile.doc
new file mode 100644
index 0000000..c1cf74a
--- /dev/null
+++ b/Makefile.doc
@@ -0,0 +1,5 @@
+qdev-doc.html: $(SRC_PATH)/qdev-doc.json $(SRC_PATH)/scripts/qdev-doc-to-html.py
+	python $(SRC_PATH)/scripts/qdev-doc-to-html.py < $< > $@
+
+qdev-doc.c: $(SRC_PATH)/qdev-doc.json $(SRC_PATH)/scripts/qdev-doc-to-c.py
+	python $(SRC_PATH)/scripts/qdev-doc-to-c.py < $< > $@
diff --git a/Makefile.objs b/Makefile.objs
index 4478c61..9547ab1 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -97,7 +97,7 @@ common-obj-y += bt-hci-csr.o
 common-obj-y += buffered_file.o migration.o migration-tcp.o
 common-obj-y += qemu-char.o savevm.o #aio.o
 common-obj-y += msmouse.o ps2.o
-common-obj-y += qdev.o qdev-properties.o
+common-obj-y += qdev.o qdev-properties.o qdev-doc.o
 common-obj-y += block-migration.o iohandler.o
 common-obj-y += pflib.o
 common-obj-y += bitmap.o bitops.o
diff --git a/qdev-doc.h b/qdev-doc.h
new file mode 100644
index 0000000..401e03e
--- /dev/null
+++ b/qdev-doc.h
@@ -0,0 +1,23 @@
+#ifndef QDEV_DOC_H
+#define QDEV_DOC_H
+
+#include "qemu-common.h"
+
+typedef struct PropertyDocumentation
+{
+    const char *name;
+    const char *type;
+    const char *docs;
+} PropertyDocumentation;
+
+typedef struct DeviceStateDocumentation
+{
+    const char *name;
+    PropertyDocumentation *properties;
+} DeviceStateDocumentation;
+
+extern DeviceStateDocumentation device_docs[];
+
+bool qdev_verify_docs(void);
+
+#endif
diff --git a/qdev-doc.json b/qdev-doc.json
new file mode 100644
index 0000000..a798bff
--- /dev/null
+++ b/qdev-doc.json
@@ -0,0 +1,16 @@
+# -*- Mode: Python -*-
+
+[ { "device": "isa-serial",
+    "parent": "ISADevice",
+    "properties": {
+            "index": { "type": "uint32",
+                       "doc": "A value from 0-3 that describes which IO regions to expose the device on.  This sets appropriate values for iobase and irq." },
+            "iobase": { "type": "hex32",
+                        "doc": "The base IO address to expose the device on." },
+            "irq": { "type": "uint32",
+                     "doc": "The IRQ to use for the device." },
+            "chardev": { "type": "chr",
+                         "doc": "The name of a character device to specify." }
+            }
+  }
+  ]
diff --git a/qemu-options.hx b/qemu-options.hx
index 9f121ad..a6abab4 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2378,6 +2378,16 @@ Specify a trace file to log output traces to.
 ETEXI
 #endif
 
+DEF("qdev-verify", 0, QEMU_OPTION_qdev_verify,
+    "-qdev-verify\n"
+    "                Verify qdev properties match documentation\n",
+    QEMU_ARCH_ALL)
+STEXI
+@item -qdev-verify
+@findex -qdev-verify
+Verify qdev properties match documentation.
+ETEXI
+
 HXCOMM This is the last statement. Insert new options before this line!
 STEXI
 @end table
diff --git a/scripts/qdev-doc-to-c.py b/scripts/qdev-doc-to-c.py
new file mode 100644
index 0000000..eb7ee59
--- /dev/null
+++ b/scripts/qdev-doc-to-c.py
@@ -0,0 +1,30 @@
+#!/usr/bin/python
+
+import sys
+
+data = sys.stdin.read()
+
+docs = eval(data)
+
+sys.stdout.write('''
+#include "qdev-doc.h"
+
+DeviceStateDocumentation device_docs[] = {''')
+
+for item in docs:
+    sys.stdout.write('''
+    {
+      .name = "%(device)s",
+      .properties = (PropertyDocumentation[]){''' % item)
+    for prop in item["properties"]:
+        sys.stdout.write('''
+        { "%s", "%s", "%s" },''' % (prop, item["properties"][prop]['type'], item["properties"][prop]['doc']))
+
+    sys.stdout.write('''
+        { } },
+    },''')
+
+sys.stdout.write('''
+    { }
+};
+''')
diff --git a/scripts/qdev-doc-to-html.py b/scripts/qdev-doc-to-html.py
new file mode 100644
index 0000000..a25fe35
--- /dev/null
+++ b/scripts/qdev-doc-to-html.py
@@ -0,0 +1,40 @@
+#!/usr/bin/python
+
+import sys
+
+data = sys.stdin.read()
+
+docs = eval(data)
+
+sys.stdout.write('''
+<html>
+<head>
+<title>QEMU device documentation</title>
+</head>
+<body>
+''')
+
+for item in docs:
+    sys.stdout.write('''
+<h2>%(device)s :: %(parent)s</h2>
+
+<table border="1">
+<tr>
+<th>Name</th><th>Type</th><th>Comments</th>
+</tr>
+''' % item)
+    for prop in item["properties"]:
+        sys.stdout.write('''
+<tr>
+<td>%s</td><td>%s</td><td>%s</td>
+</tr>
+''' % (prop, item["properties"][prop]['type'], item["properties"][prop]['doc']))
+
+    sys.stdout.write('''
+</table>
+''')
+
+sys.stdout.write('''
+</body>
+</html>
+''')
diff --git a/vl.c b/vl.c
index bffba69..b9f6e63 100644
--- a/vl.c
+++ b/vl.c
@@ -163,6 +163,8 @@ int main(int argc, char **argv)
 
 #include "ui/qemu-spice.h"
 
+#include "qdev-doc.h"
+
 //#define DEBUG_NET
 //#define DEBUG_SLIRP
 
@@ -1298,6 +1300,82 @@ void qemu_system_vmstop_request(int reason)
     qemu_notify_event();
 }
 
+bool qdev_verify_docs(void)
+{
+    DeviceStateDocumentation *doc;
+    DeviceInfo *dev;
+    int errors = 0;
+    int warnings = 0;
+
+    for (dev = device_info_list; dev; dev = dev->next) {
+        PropertyDocumentation *prop_doc;
+        Property *prop;
+
+        for (doc = device_docs; doc->name; doc++) {
+            if (strcmp(doc->name, dev->name) == 0) {
+                break;
+            }
+        }
+
+        if (doc->name == NULL) {
+            fprintf(stderr, "Warning: device `%s' is undocumented\n",
+                    dev->name);
+            warnings++;
+            continue;
+        }
+
+        for (prop = dev->props; prop->name; prop++) {
+            for (prop_doc = doc->properties; prop_doc->name; prop_doc++) {
+                if (strcmp(prop->name, prop_doc->name) == 0) {
+                    break;
+                }
+            }
+
+            if (prop_doc->name == NULL) {
+                fprintf(stderr, "Warning: device `%s' has undocumented property `%s'\n",
+                        dev->name, prop->name);
+                warnings++;
+            }
+        }
+
+        for (prop_doc = doc->properties; prop_doc->name; prop_doc++) {
+            for (prop = dev->props; prop->name; prop++) {
+                if (strcmp(prop->name, prop_doc->name) == 0) {
+                    break;
+                }
+            }
+
+            if (prop->name == NULL) {
+                fprintf(stderr, "Error: device `%s' has documented property `%s' with no definition\n",
+                        dev->name, prop_doc->name);
+                errors++;
+            }
+        }
+    }
+
+    for (doc = device_docs; doc->name; doc++) {
+        for (dev = device_info_list; dev; dev = dev->next) {
+            if (strcmp(doc->name, dev->name) == 0) {
+                break;
+            }
+        }
+
+        if (dev == NULL) {
+            fprintf(stderr, "Error: documented device `%s' has no definition\n",
+                    doc->name);
+            errors++;
+        }
+    }
+
+    fprintf(stderr, "%d warnings, %d errors.\n", warnings, errors);
+
+    if (errors > 0) {
+        return true;
+    }
+
+    return false;
+}
+
 void main_loop_wait(int nonblocking)
 {
     fd_set rfds, wfds, xfds;
@@ -2057,6 +2135,7 @@ int main(int argc, char **argv, char **envp)
 #endif
     int defconfig = 1;
     const char *trace_file = NULL;
+    bool do_qdev_verify = false;
 
     atexit(qemu_run_exit_notifiers);
     error_set_progname(argv[0]);
@@ -2890,6 +2969,9 @@ int main(int argc, char **argv, char **envp)
                     fclose(fp);
                     break;
                 }
+            case QEMU_OPTION_qdev_verify:
+                do_qdev_verify = true;
+                break;
             default:
                 os_parse_cmd_args(popt->index, optarg);
             }
@@ -3136,6 +3218,13 @@ int main(int argc, char **argv, char **envp)
 
     module_call_init(MODULE_INIT_DEVICE);
 
+    if (do_qdev_verify) {
+        if (qdev_verify_docs()) {
+            exit(1);
+        }
+        exit(0);
+    }
+
     if (qemu_opts_foreach(qemu_find_opts("device"), device_help_func, NULL, 0) != 0)
         exit(0);
 
-- 
1.7.4.1