| Message ID | 1456771254-17511-14-git-send-email-armbru@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On Mon, Feb 29, 2016 at 7:40 PM, Markus Armbruster <armbru@redhat.com> wrote: > Yes, the chardev is commonly useless after we read a bad version from > it, but destroying it is inappropriate anyway: the user created it, so > the user should be able to hold on to it as long as he likes. We > don't destroy it on other errors. Screwed up in commit 5105b1d. > > Stop reading instead. > > Also note QEMU's behavior in ivshmem-spec.txt. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> Interestingly enough, it seems both smartcard "passthru" and usb redirect do chr_delete(). It would a nice follow up to standardize this there too. Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > docs/specs/ivshmem-spec.txt | 3 +++ > hw/misc/ivshmem.c | 3 +-- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/docs/specs/ivshmem-spec.txt b/docs/specs/ivshmem-spec.txt > index 0835ba1..4fc6f37 100644 > --- a/docs/specs/ivshmem-spec.txt > +++ b/docs/specs/ivshmem-spec.txt > @@ -188,6 +188,9 @@ Each message consists of a single 8 byte little-endian signed number, > and may be accompanied by a file descriptor via SCM_RIGHTS. Both > client and server close the connection on error. > > +Note: QEMU currently doesn't close the connection right on error, but > +only when the character device is destroyed. > + > On connect, the server sends the following messages in order: > > 1. The protocol version number, currently zero. The client should > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c > index 7119a07..2850e8a 100644 > --- a/hw/misc/ivshmem.c > +++ b/hw/misc/ivshmem.c > @@ -707,8 +707,7 @@ static void ivshmem_check_version(void *opaque, const uint8_t * buf, int size) > if (tmp != -1 || version != IVSHMEM_PROTOCOL_VERSION) { > fprintf(stderr, "incompatible version, you are connecting to a ivshmem-" > "server using a different protocol please check your setup\n"); > - qemu_chr_delete(s->server_chr); > - s->server_chr = NULL; > + qemu_chr_add_handlers(s->server_chr, NULL, NULL, NULL, s); > return; > } > > -- > 2.4.3 > >
Marc-André Lureau <marcandre.lureau@gmail.com> writes: > Hi > > On Mon, Feb 29, 2016 at 7:40 PM, Markus Armbruster <armbru@redhat.com> wrote: >> Yes, the chardev is commonly useless after we read a bad version from >> it, but destroying it is inappropriate anyway: the user created it, so >> the user should be able to hold on to it as long as he likes. We >> don't destroy it on other errors. Screwed up in commit 5105b1d. >> >> Stop reading instead. >> >> Also note QEMU's behavior in ivshmem-spec.txt. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > > Interestingly enough, it seems both smartcard "passthru" and usb > redirect do chr_delete(). It would a nice follow up to standardize > this there too. Wasn't aware of them. I'll see what I can do. > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Thanks!
diff --git a/docs/specs/ivshmem-spec.txt b/docs/specs/ivshmem-spec.txt index 0835ba1..4fc6f37 100644 --- a/docs/specs/ivshmem-spec.txt +++ b/docs/specs/ivshmem-spec.txt @@ -188,6 +188,9 @@ Each message consists of a single 8 byte little-endian signed number, and may be accompanied by a file descriptor via SCM_RIGHTS. Both client and server close the connection on error. +Note: QEMU currently doesn't close the connection right on error, but +only when the character device is destroyed. + On connect, the server sends the following messages in order: 1. The protocol version number, currently zero. The client should diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c index 7119a07..2850e8a 100644 --- a/hw/misc/ivshmem.c +++ b/hw/misc/ivshmem.c @@ -707,8 +707,7 @@ static void ivshmem_check_version(void *opaque, const uint8_t * buf, int size) if (tmp != -1 || version != IVSHMEM_PROTOCOL_VERSION) { fprintf(stderr, "incompatible version, you are connecting to a ivshmem-" "server using a different protocol please check your setup\n"); - qemu_chr_delete(s->server_chr); - s->server_chr = NULL; + qemu_chr_add_handlers(s->server_chr, NULL, NULL, NULL, s); return; }
Yes, the chardev is commonly useless after we read a bad version from it, but destroying it is inappropriate anyway: the user created it, so the user should be able to hold on to it as long as he likes. We don't destroy it on other errors. Screwed up in commit 5105b1d. Stop reading instead. Also note QEMU's behavior in ivshmem-spec.txt. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- docs/specs/ivshmem-spec.txt | 3 +++ hw/misc/ivshmem.c | 3 +-- 2 files changed, 4 insertions(+), 2 deletions(-)