[Qemu-devel] [PATCH] monitor: add usb_detach

[Qemu-devel] [PATCH] monitor: add usb_detach

[Alon Levy]

Signed-off-by: Alon Levy <alevy@redhat.com>

---
 qemu-monitor.hx |   17 +++++++++++++++++
 sysemu.h        |    1 +
 vl.c            |   41 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+), 0 deletions(-)

diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 49bcd8d..9c792a9 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -711,6 +711,23 @@ command @code{info usb} to see the devices you can remove.
 ETEXI
 
     {
+        .name       = "usb_detach",
+        .args_type  = "devname:s",
+        .params     = "device",
+        .help       = "detach USB device 'bus.addr'",
+        .mhandler.cmd = do_usb_detach,
+    },
+
+STEXI
+@item usb_detach @var{devname}
+@findex usb_detach
+
+Detach the USB device @var{devname} from the QEMU virtual USB
+hub. @var{devname} has the syntax @code{bus.addr}. Use the monitor
+command @code{info usb} to see the devices you can detach.
+ETEXI
+
+    {
         .name       = "device_add",
         .args_type  = "device:O",
         .params     = "driver[,prop=value][,...]",
diff --git a/sysemu.h b/sysemu.h
index a1f6466..ac68863 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -183,6 +183,7 @@ extern struct soundhw soundhw[];
 
 void do_usb_add(Monitor *mon, const QDict *qdict);
 void do_usb_del(Monitor *mon, const QDict *qdict);
+void do_usb_detach(Monitor *mon, const QDict *qdict);
 void usb_info(Monitor *mon);
 
 void rtc_change_mon_event(struct tm *tm);
diff --git a/vl.c b/vl.c
index d352d18..6cfa009 100644
--- a/vl.c
+++ b/vl.c
@@ -891,6 +891,47 @@ void do_usb_del(Monitor *mon, const QDict *qdict)
     }
 }
 
+static USBDevice *usb_device_from_bus_dot_addr(const char *devname)
+{
+    int bus_num, addr;
+    const char *p;
+    USBBus *bus;
+    USBPort *port;
+
+    if (!usb_enabled) {
+        return NULL;
+    }
+    p = strchr(devname, '.');
+    if (!p) {
+        return NULL;
+    }
+    bus_num = strtoul(devname, NULL, 0);
+    addr = strtoul(p + 1, NULL, 0);
+    bus = usb_bus_find(bus_num);
+    if (!bus) {
+        return NULL;
+    }
+    QTAILQ_FOREACH(port, &bus->used, next) {
+        if (port->dev->addr == addr) {
+            break;
+        }
+    }
+    if (!port) {
+        return NULL;
+    }
+    return port->dev;
+}
+
+void do_usb_detach(Monitor *mon, const QDict *qdict)
+{
+    const char *devname = qdict_get_str(qdict, "devname");
+    USBDevice *dev = usb_device_from_bus_dot_addr(devname);
+
+    if (dev == NULL || usb_device_detach(dev) < 0) {
+        error_report("could not detach USB device '%s'", devname);
+    }
+}
+
 /***********************************************************/
 /* PCMCIA/Cardbus */
 
-- 
1.7.3.1



-- 
Alon Levy <alevy@redhat.com>

Re: [Qemu-devel] [PATCH] monitor: add usb_detach

[Luiz Capitulino]

On Tue, 5 Oct 2010 16:40:29 +0200
Alon Levy <alevy@redhat.com> wrote:

> Signed-off-by: Alon Levy <alevy@redhat.com>

This needs to be rebased as it now conflicts with my last series merged
on master.

I have some minor comments (below), but in general seems ok to me. However,
I'd like to get an ACK from someone familiar with QEMU's USB before
applying to the monitor's queue.

Anthony? Gerd?

> 
> ---
>  qemu-monitor.hx |   17 +++++++++++++++++
>  sysemu.h        |    1 +
>  vl.c            |   41 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 59 insertions(+), 0 deletions(-)
> 
> diff --git a/qemu-monitor.hx b/qemu-monitor.hx
> index 49bcd8d..9c792a9 100644
> --- a/qemu-monitor.hx
> +++ b/qemu-monitor.hx
> @@ -711,6 +711,23 @@ command @code{info usb} to see the devices you can remove.
>  ETEXI
>  
>      {
> +        .name       = "usb_detach",
> +        .args_type  = "devname:s",
> +        .params     = "device",
> +        .help       = "detach USB device 'bus.addr'",
> +        .mhandler.cmd = do_usb_detach,
> +    },
> +
> +STEXI
> +@item usb_detach @var{devname}
> +@findex usb_detach
> +
> +Detach the USB device @var{devname} from the QEMU virtual USB
> +hub. @var{devname} has the syntax @code{bus.addr}. Use the monitor
> +command @code{info usb} to see the devices you can detach.
> +ETEXI
> +
> +    {
>          .name       = "device_add",
>          .args_type  = "device:O",
>          .params     = "driver[,prop=value][,...]",
> diff --git a/sysemu.h b/sysemu.h
> index a1f6466..ac68863 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -183,6 +183,7 @@ extern struct soundhw soundhw[];
>  
>  void do_usb_add(Monitor *mon, const QDict *qdict);
>  void do_usb_del(Monitor *mon, const QDict *qdict);
> +void do_usb_detach(Monitor *mon, const QDict *qdict);
>  void usb_info(Monitor *mon);
>  
>  void rtc_change_mon_event(struct tm *tm);
> diff --git a/vl.c b/vl.c
> index d352d18..6cfa009 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -891,6 +891,47 @@ void do_usb_del(Monitor *mon, const QDict *qdict)
>      }
>  }
>  
> +static USBDevice *usb_device_from_bus_dot_addr(const char *devname)
> +{
> +    int bus_num, addr;
> +    const char *p;
> +    USBBus *bus;
> +    USBPort *port;
> +
> +    if (!usb_enabled) {
> +        return NULL;
> +    }
> +    p = strchr(devname, '.');
> +    if (!p) {
> +        return NULL;
> +    }
> +    bus_num = strtoul(devname, NULL, 0);
> +    addr = strtoul(p + 1, NULL, 0);

Checking stroul()'s return would avoid nasty surprises.

> +    bus = usb_bus_find(bus_num);
> +    if (!bus) {
> +        return NULL;
> +    }
> +    QTAILQ_FOREACH(port, &bus->used, next) {
> +        if (port->dev->addr == addr) {
> +            break;
> +        }
> +    }
> +    if (!port) {
> +        return NULL;
> +    }
> +    return port->dev;
> +}
> +
> +void do_usb_detach(Monitor *mon, const QDict *qdict)
> +{
> +    const char *devname = qdict_get_str(qdict, "devname");
> +    USBDevice *dev = usb_device_from_bus_dot_addr(devname);
> +
> +    if (dev == NULL || usb_device_detach(dev) < 0) {
> +        error_report("could not detach USB device '%s'", devname);
> +    }

You can improve the error message if you check for dev and detach
separately. Minor.

> +}
> +
>  /***********************************************************/
>  /* PCMCIA/Cardbus */
>  

Re: [Qemu-devel] [PATCH] monitor: add usb_detach

[Anthony Liguori]

How is this different than usb_del?  Is it that it detaches it but does 
not delete the device?

Regards,

Anthony Liguori

On 10/05/2010 09:40 AM, Alon Levy wrote:
> Signed-off-by: Alon Levy<alevy@redhat.com>
>
> ---
>   qemu-monitor.hx |   17 +++++++++++++++++
>   sysemu.h        |    1 +
>   vl.c            |   41 +++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 59 insertions(+), 0 deletions(-)
>
> diff --git a/qemu-monitor.hx b/qemu-monitor.hx
> index 49bcd8d..9c792a9 100644
> --- a/qemu-monitor.hx
> +++ b/qemu-monitor.hx
> @@ -711,6 +711,23 @@ command @code{info usb} to see the devices you can remove.
>   ETEXI
>
>       {
> +        .name       = "usb_detach",
> +        .args_type  = "devname:s",
> +        .params     = "device",
> +        .help       = "detach USB device 'bus.addr'",
> +        .mhandler.cmd = do_usb_detach,
> +    },
> +
> +STEXI
> +@item usb_detach @var{devname}
> +@findex usb_detach
> +
> +Detach the USB device @var{devname} from the QEMU virtual USB
> +hub. @var{devname} has the syntax @code{bus.addr}. Use the monitor
> +command @code{info usb} to see the devices you can detach.
> +ETEXI
> +
> +    {
>           .name       = "device_add",
>           .args_type  = "device:O",
>           .params     = "driver[,prop=value][,...]",
> diff --git a/sysemu.h b/sysemu.h
> index a1f6466..ac68863 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -183,6 +183,7 @@ extern struct soundhw soundhw[];
>
>   void do_usb_add(Monitor *mon, const QDict *qdict);
>   void do_usb_del(Monitor *mon, const QDict *qdict);
> +void do_usb_detach(Monitor *mon, const QDict *qdict);
>   void usb_info(Monitor *mon);
>
>   void rtc_change_mon_event(struct tm *tm);
> diff --git a/vl.c b/vl.c
> index d352d18..6cfa009 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -891,6 +891,47 @@ void do_usb_del(Monitor *mon, const QDict *qdict)
>       }
>   }
>
> +static USBDevice *usb_device_from_bus_dot_addr(const char *devname)
> +{
> +    int bus_num, addr;
> +    const char *p;
> +    USBBus *bus;
> +    USBPort *port;
> +
> +    if (!usb_enabled) {
> +        return NULL;
> +    }
> +    p = strchr(devname, '.');
> +    if (!p) {
> +        return NULL;
> +    }
> +    bus_num = strtoul(devname, NULL, 0);
> +    addr = strtoul(p + 1, NULL, 0);
> +    bus = usb_bus_find(bus_num);
> +    if (!bus) {
> +        return NULL;
> +    }
> +    QTAILQ_FOREACH(port,&bus->used, next) {
> +        if (port->dev->addr == addr) {
> +            break;
> +        }
> +    }
> +    if (!port) {
> +        return NULL;
> +    }
> +    return port->dev;
> +}
> +
> +void do_usb_detach(Monitor *mon, const QDict *qdict)
> +{
> +    const char *devname = qdict_get_str(qdict, "devname");
> +    USBDevice *dev = usb_device_from_bus_dot_addr(devname);
> +
> +    if (dev == NULL || usb_device_detach(dev)<  0) {
> +        error_report("could not detach USB device '%s'", devname);
> +    }
> +}
> +
>   /***********************************************************/
>   /* PCMCIA/Cardbus */
>
>    

Re: [Qemu-devel] [PATCH] monitor: add usb_detach

[Alon Levy]

----- "Anthony Liguori" <anthony@codemonkey.ws> wrote:

> How is this different than usb_del?  Is it that it detaches it but
> does 
> not delete the device?
> 

yes. There is no usb_attach command because it was harder to write (can't
use the bus.addr since a detached device doesn't have them) and I didn't
need it right now, my device attaches itself based on a external event.

> Regards,
> 
> Anthony Liguori
> 
> On 10/05/2010 09:40 AM, Alon Levy wrote:
> > Signed-off-by: Alon Levy<alevy@redhat.com>
> >
> > ---
> >   qemu-monitor.hx |   17 +++++++++++++++++
> >   sysemu.h        |    1 +
> >   vl.c            |   41 +++++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 59 insertions(+), 0 deletions(-)
> >
> > diff --git a/qemu-monitor.hx b/qemu-monitor.hx
> > index 49bcd8d..9c792a9 100644
> > --- a/qemu-monitor.hx
> > +++ b/qemu-monitor.hx
> > @@ -711,6 +711,23 @@ command @code{info usb} to see the devices you
> can remove.
> >   ETEXI
> >
> >       {
> > +        .name       = "usb_detach",
> > +        .args_type  = "devname:s",
> > +        .params     = "device",
> > +        .help       = "detach USB device 'bus.addr'",
> > +        .mhandler.cmd = do_usb_detach,
> > +    },
> > +
> > +STEXI
> > +@item usb_detach @var{devname}
> > +@findex usb_detach
> > +
> > +Detach the USB device @var{devname} from the QEMU virtual USB
> > +hub. @var{devname} has the syntax @code{bus.addr}. Use the monitor
> > +command @code{info usb} to see the devices you can detach.
> > +ETEXI
> > +
> > +    {
> >           .name       = "device_add",
> >           .args_type  = "device:O",
> >           .params     = "driver[,prop=value][,...]",
> > diff --git a/sysemu.h b/sysemu.h
> > index a1f6466..ac68863 100644
> > --- a/sysemu.h
> > +++ b/sysemu.h
> > @@ -183,6 +183,7 @@ extern struct soundhw soundhw[];
> >
> >   void do_usb_add(Monitor *mon, const QDict *qdict);
> >   void do_usb_del(Monitor *mon, const QDict *qdict);
> > +void do_usb_detach(Monitor *mon, const QDict *qdict);
> >   void usb_info(Monitor *mon);
> >
> >   void rtc_change_mon_event(struct tm *tm);
> > diff --git a/vl.c b/vl.c
> > index d352d18..6cfa009 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -891,6 +891,47 @@ void do_usb_del(Monitor *mon, const QDict
> *qdict)
> >       }
> >   }
> >
> > +static USBDevice *usb_device_from_bus_dot_addr(const char
> *devname)
> > +{
> > +    int bus_num, addr;
> > +    const char *p;
> > +    USBBus *bus;
> > +    USBPort *port;
> > +
> > +    if (!usb_enabled) {
> > +        return NULL;
> > +    }
> > +    p = strchr(devname, '.');
> > +    if (!p) {
> > +        return NULL;
> > +    }
> > +    bus_num = strtoul(devname, NULL, 0);
> > +    addr = strtoul(p + 1, NULL, 0);
> > +    bus = usb_bus_find(bus_num);
> > +    if (!bus) {
> > +        return NULL;
> > +    }
> > +    QTAILQ_FOREACH(port,&bus->used, next) {
> > +        if (port->dev->addr == addr) {
> > +            break;
> > +        }
> > +    }
> > +    if (!port) {
> > +        return NULL;
> > +    }
> > +    return port->dev;
> > +}
> > +
> > +void do_usb_detach(Monitor *mon, const QDict *qdict)
> > +{
> > +    const char *devname = qdict_get_str(qdict, "devname");
> > +    USBDevice *dev = usb_device_from_bus_dot_addr(devname);
> > +
> > +    if (dev == NULL || usb_device_detach(dev)<  0) {
> > +        error_report("could not detach USB device '%s'", devname);
> > +    }
> > +}
> > +
> >   /***********************************************************/
> >   /* PCMCIA/Cardbus */
> >
> >

Re: [Qemu-devel] [PATCH] monitor: add usb_detach

[Gerd Hoffmann]

On 10/10/10 13:12, Alon Levy wrote:
>
> ----- "Anthony Liguori"<anthony@codemonkey.ws>  wrote:
>
>> How is this different than usb_del?  Is it that it detaches it but
>> does
>> not delete the device?
>
> yes. There is no usb_attach command because it was harder to write (can't
> use the bus.addr since a detached device doesn't have them) and I didn't
> need it right now, my device attaches itself based on a external event.

Which points out a problem with this patch:  It should better not use 
bus.addr.  addr isn't fixed and even can be uninitialized.  Yes, usb_del 
uses it (for historical reasons).  But we better should not use it in 
new code.  Better use the device id (like device_del).  Which will work 
for usb_attach too.

Next question:  What is the use case?  attach/detach is used by devices 
internally.  usb-host does attach/detach when devices get plugged-in and 
-out on the host.  The ccid device does simliar things on vsclient 
connect/disconnect.  So toggeling the attach state via monitor easily 
could have unwanted side effects ...

cheers,
   Gerd

Re: [Qemu-devel] [PATCH] monitor: add usb_detach

[Alon Levy]

----- "Gerd Hoffmann" <kraxel@redhat.com> wrote:

> On 10/10/10 13:12, Alon Levy wrote:
> >
> > ----- "Anthony Liguori"<anthony@codemonkey.ws>  wrote:
> >
> >> How is this different than usb_del?  Is it that it detaches it but
> >> does
> >> not delete the device?
> >
> > yes. There is no usb_attach command because it was harder to write
> (can't
> > use the bus.addr since a detached device doesn't have them) and I
> didn't
> > need it right now, my device attaches itself based on a external
> event.
> 
> Which points out a problem with this patch:  It should better not use
> 
> bus.addr.  addr isn't fixed and even can be uninitialized.  Yes,
> usb_del 
> uses it (for historical reasons).  But we better should not use it in
> 
> new code.  Better use the device id (like device_del).  Which will
> work 
> for usb_attach too.
> 
> Next question:  What is the use case?  attach/detach is used by
> devices 
> internally.  usb-host does attach/detach when devices get plugged-in
> and 
> -out on the host.  The ccid device does simliar things on vsclient 
> connect/disconnect.  So toggeling the attach state via monitor easily
> 

debugging. naturally when developing the ccid I had cases where I'd
rather detach the device then bring down qemu. since there is no way
currently to add/remove chardev's from monitor, removing/adding
a device is not enough to reset a device state to the state right
after start.

> could have unwanted side effects ...
> 
> cheers,
>    Gerd