Patchwork char: Allow devices to use a single multiplexed chardev.

login
register
mail settings
Submitter Kusanagi Kouichi
Date April 22, 2011, 12:59 p.m.
Message ID <20110422125943.2F24E6FC039@msa105.auone-net.jp>
Download mbox | patch
Permalink /patch/92529/
State New
Headers show

Comments

Kusanagi Kouichi - April 22, 2011, 12:59 p.m.
This fixes regression caused by commit
2d6c1ef40f3678ab47a4d14fb5dadaa486bfcda6
("char: Prevent multiple devices opening same chardev").

Signed-off-by: Kusanagi Kouichi <slash@ac.auone-net.jp>
---
 hw/qdev-properties.c |    4 ++--
 qemu-char.c          |    5 ++++-
 qemu-char.h          |    2 +-
 3 files changed, 7 insertions(+), 4 deletions(-)
Amit Shah - April 25, 2011, 9:57 a.m.
On (Fri) 22 Apr 2011 [21:59:42], Kusanagi Kouichi wrote:
> This fixes regression caused by commit
> 2d6c1ef40f3678ab47a4d14fb5dadaa486bfcda6
> ("char: Prevent multiple devices opening same chardev").

What's the regression?  How do I test it?

> Signed-off-by: Kusanagi Kouichi <slash@ac.auone-net.jp>
> ---
>  hw/qdev-properties.c |    4 ++--
>  qemu-char.c          |    5 ++++-
>  qemu-char.h          |    2 +-
>  3 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index 1088a26..0eed712 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -354,10 +354,10 @@ static int parse_chr(DeviceState *dev, Property *prop, const char *str)
>      if (*ptr == NULL) {
>          return -ENOENT;
>      }
> -    if ((*ptr)->assigned) {
> +    if ((*ptr)->avail < 1) {
>          return -EEXIST;
>      }
> -    (*ptr)->assigned = 1;
> +    --(*ptr)->avail;
>      return 0;
>  }
>  
> diff --git a/qemu-char.c b/qemu-char.c
> index 03858d4..f08f2b8 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -199,7 +199,7 @@ void qemu_chr_add_handlers(CharDriverState *s,
>  {
>      if (!opaque) {
>          /* chr driver being released. */
> -        s->assigned = 0;
> +        ++s->avail;
>      }

Will just checking for handlers (fd_can_read, fd_read, fd_write not
NULL) here help instead of this patch?

>      s->chr_can_read = fd_can_read;
>      s->chr_read = fd_read;
> @@ -2544,7 +2544,10 @@ CharDriverState *qemu_chr_open_opts(QemuOpts *opts,
>          snprintf(base->label, len, "%s-base", qemu_opts_id(opts));
>          chr = qemu_chr_open_mux(base);
>          chr->filename = base->filename;
> +        chr->avail = MAX_MUX;
>          QTAILQ_INSERT_TAIL(&chardevs, chr, next);
> +    } else {
> +        chr->avail = 1;
>      }
>      chr->label = qemu_strdup(qemu_opts_id(opts));
>      return chr;
> diff --git a/qemu-char.h b/qemu-char.h
> index fb96eef..ebf3641 100644
> --- a/qemu-char.h
> +++ b/qemu-char.h
> @@ -70,7 +70,7 @@ struct CharDriverState {
>      char *label;
>      char *filename;
>      int opened;
> -    int assigned; /* chardev assigned to a device */
> +    int avail;
>      QTAILQ_ENTRY(CharDriverState) next;
>  };
>  
> -- 
> 1.7.4.4
> 
> 

		Amit
Kusanagi Kouichi - April 25, 2011, 3:30 p.m.
On 2011-04-25 15:27:20 +0530, Amit Shah wrote:
> On (Fri) 22 Apr 2011 [21:59:42], Kusanagi Kouichi wrote:
> > This fixes regression caused by commit
> > 2d6c1ef40f3678ab47a4d14fb5dadaa486bfcda6
> > ("char: Prevent multiple devices opening same chardev").
> 
> What's the regression?  How do I test it?
> 

-nodefaults -nographic -chardev stdio,id=stdio,mux=on,signal=off -mon stdio -device virtio-serial-pci -device virtconsole,chardev=stdio -device isa-serial,chardev=stdio
fails with
qemu-system-x86_64: -device isa-serial,chardev=stdio: Property 'isa-serial.chardev' can't take value 'stdio', it's in use
Is this intended?

> > Signed-off-by: Kusanagi Kouichi <slash@ac.auone-net.jp>
> > ---
> >  hw/qdev-properties.c |    4 ++--
> >  qemu-char.c          |    5 ++++-
> >  qemu-char.h          |    2 +-
> >  3 files changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> > index 1088a26..0eed712 100644
> > --- a/hw/qdev-properties.c
> > +++ b/hw/qdev-properties.c
> > @@ -354,10 +354,10 @@ static int parse_chr(DeviceState *dev, Property *prop, const char *str)
> >      if (*ptr == NULL) {
> >          return -ENOENT;
> >      }
> > -    if ((*ptr)->assigned) {
> > +    if ((*ptr)->avail < 1) {
> >          return -EEXIST;
> >      }
> > -    (*ptr)->assigned = 1;
> > +    --(*ptr)->avail;
> >      return 0;
> >  }
> >  
> > diff --git a/qemu-char.c b/qemu-char.c
> > index 03858d4..f08f2b8 100644
> > --- a/qemu-char.c
> > +++ b/qemu-char.c
> > @@ -199,7 +199,7 @@ void qemu_chr_add_handlers(CharDriverState *s,
> >  {
> >      if (!opaque) {
> >          /* chr driver being released. */
> > -        s->assigned = 0;
> > +        ++s->avail;
> >      }
> 
> Will just checking for handlers (fd_can_read, fd_read, fd_write not
> NULL) here help instead of this patch?
> 

That doesn't help.

> >      s->chr_can_read = fd_can_read;
> >      s->chr_read = fd_read;
> > @@ -2544,7 +2544,10 @@ CharDriverState *qemu_chr_open_opts(QemuOpts *opts,
> >          snprintf(base->label, len, "%s-base", qemu_opts_id(opts));
> >          chr = qemu_chr_open_mux(base);
> >          chr->filename = base->filename;
> > +        chr->avail = MAX_MUX;
> >          QTAILQ_INSERT_TAIL(&chardevs, chr, next);
> > +    } else {
> > +        chr->avail = 1;
> >      }
> >      chr->label = qemu_strdup(qemu_opts_id(opts));
> >      return chr;
> > diff --git a/qemu-char.h b/qemu-char.h
> > index fb96eef..ebf3641 100644
> > --- a/qemu-char.h
> > +++ b/qemu-char.h
> > @@ -70,7 +70,7 @@ struct CharDriverState {
> >      char *label;
> >      char *filename;
> >      int opened;
> > -    int assigned; /* chardev assigned to a device */
> > +    int avail;
> >      QTAILQ_ENTRY(CharDriverState) next;
> >  };
> >  
> > -- 
> > 1.7.4.4
> > 
> > 
> 
> 		Amit
Amit Shah - April 26, 2011, 4:32 a.m.
On (Tue) 26 Apr 2011 [00:30:28], Kusanagi Kouichi wrote:
> On 2011-04-25 15:27:20 +0530, Amit Shah wrote:
> > On (Fri) 22 Apr 2011 [21:59:42], Kusanagi Kouichi wrote:
> > > This fixes regression caused by commit
> > > 2d6c1ef40f3678ab47a4d14fb5dadaa486bfcda6
> > > ("char: Prevent multiple devices opening same chardev").
> > 
> > What's the regression?  How do I test it?
> > 
> 
> -nodefaults -nographic -chardev stdio,id=stdio,mux=on,signal=off -mon stdio -device virtio-serial-pci -device virtconsole,chardev=stdio -device isa-serial,chardev=stdio
> fails with
> qemu-system-x86_64: -device isa-serial,chardev=stdio: Property 'isa-serial.chardev' can't take value 'stdio', it's in use

OK, so please mention it in the commit message :-)

> Is this intended?

No, it's not.

Just one more thing:

> > > diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> > > index 1088a26..0eed712 100644
> > > --- a/hw/qdev-properties.c
> > > +++ b/hw/qdev-properties.c
> > > @@ -354,10 +354,10 @@ static int parse_chr(DeviceState *dev, Property *prop, const char *str)
> > >      if (*ptr == NULL) {
> > >          return -ENOENT;
> > >      }
> > > -    if ((*ptr)->assigned) {
> > > +    if ((*ptr)->avail < 1) {
> > >          return -EEXIST;
> > >      }
> > > -    (*ptr)->assigned = 1;
> > > +    --(*ptr)->avail;
> > >      return 0;
> > >  }

'avail' isn't readily intuitive.  Can you use a better name, like
'avail_connections' or something like that?

Please CC me on the updated patch.

		Amit

Patch

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 1088a26..0eed712 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -354,10 +354,10 @@  static int parse_chr(DeviceState *dev, Property *prop, const char *str)
     if (*ptr == NULL) {
         return -ENOENT;
     }
-    if ((*ptr)->assigned) {
+    if ((*ptr)->avail < 1) {
         return -EEXIST;
     }
-    (*ptr)->assigned = 1;
+    --(*ptr)->avail;
     return 0;
 }
 
diff --git a/qemu-char.c b/qemu-char.c
index 03858d4..f08f2b8 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -199,7 +199,7 @@  void qemu_chr_add_handlers(CharDriverState *s,
 {
     if (!opaque) {
         /* chr driver being released. */
-        s->assigned = 0;
+        ++s->avail;
     }
     s->chr_can_read = fd_can_read;
     s->chr_read = fd_read;
@@ -2544,7 +2544,10 @@  CharDriverState *qemu_chr_open_opts(QemuOpts *opts,
         snprintf(base->label, len, "%s-base", qemu_opts_id(opts));
         chr = qemu_chr_open_mux(base);
         chr->filename = base->filename;
+        chr->avail = MAX_MUX;
         QTAILQ_INSERT_TAIL(&chardevs, chr, next);
+    } else {
+        chr->avail = 1;
     }
     chr->label = qemu_strdup(qemu_opts_id(opts));
     return chr;
diff --git a/qemu-char.h b/qemu-char.h
index fb96eef..ebf3641 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -70,7 +70,7 @@  struct CharDriverState {
     char *label;
     char *filename;
     int opened;
-    int assigned; /* chardev assigned to a device */
+    int avail;
     QTAILQ_ENTRY(CharDriverState) next;
 };