| Message ID | 20110422125943.2F24E6FC039@msa105.auone-net.jp |
|---|---|
| State | New |
| Headers | show |
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
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
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
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; };
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(-)