From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48194) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z4j6d-0002kf-Kb for qemu-devel@nongnu.org; Tue, 16 Jun 2015 01:08:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z4j6Z-00087X-4I for qemu-devel@nongnu.org; Tue, 16 Jun 2015 01:08:15 -0400 Received: from mail-pa0-f50.google.com ([209.85.220.50]:35692) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z4j6Y-00087Q-TK for qemu-devel@nongnu.org; Tue, 16 Jun 2015 01:08:11 -0400 Received: by pacyx8 with SMTP id yx8so5204767pac.2 for ; Mon, 15 Jun 2015 22:08:10 -0700 (PDT) Message-ID: <557FAF37.9020502@igel.co.jp> Date: Tue, 16 Jun 2015 14:08:07 +0900 From: Tetsuya Mukawa MIME-Version: 1.0 References: <1432538908-26298-5-git-send-email-mukawa@igel.co.jp> <1432874550-10921-1-git-send-email-mukawa@igel.co.jp> <1432874550-10921-3-git-send-email-mukawa@igel.co.jp> <20150615134022.GB9410@stefanha-thinkpad.redhat.com> In-Reply-To: <20150615134022.GB9410@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v1 2/4] vhost-user: Shutdown vhost-user connection when wrong messages are passed List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: jasowang@redhat.com, qemu-devel@nongnu.org, n.nikolaev@virtualopensystems.com, mst@redhat.com On 2015/06/15 22:40, Stefan Hajnoczi wrote: > On Fri, May 29, 2015 at 01:42:28PM +0900, Tetsuya Mukawa wrote: >> When wrong vhost-user message are passed, the connection should be >> shutdown. >> >> Signed-off-by: Tetsuya Mukawa >> --- >> hw/virtio/vhost-user.c | 17 ++++++++++------- >> include/sysemu/char.h | 7 +++++++ >> qemu-char.c | 15 +++++++++++++++ >> 3 files changed, 32 insertions(+), 7 deletions(-) >> >> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c >> index e7ab829..4d7e3ba 100644 >> --- a/hw/virtio/vhost-user.c >> +++ b/hw/virtio/vhost-user.c >> @@ -183,6 +183,7 @@ static int vhost_user_write(struct vhost_dev *dev, VhostUserMsg *msg, >> static int vhost_user_call(struct vhost_dev *dev, unsigned long int request, >> void *arg) >> { >> + CharDriverState *chr = dev->opaque; >> VhostUserMsg msg; >> VhostUserRequest msg_request; >> struct vhost_vring_file *file = 0; >> @@ -237,7 +238,7 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request, >> if (!fd_num) { >> error_report("Failed initializing vhost-user memory map, " >> "consider using -object memory-backend-file share=on"); >> - return -1; >> + goto close; >> } >> >> msg.size = sizeof(m.memory.nregions); >> @@ -281,7 +282,7 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request, >> break; >> default: >> error_report("vhost-user trying to send unhandled ioctl"); >> - return -1; >> + goto close; >> break; >> } >> >> @@ -297,32 +298,34 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request, >> if (msg_request != msg.request) { >> error_report("Received unexpected msg type." >> " Expected %d received %d", msg_request, msg.request); >> - return -1; >> + goto close; >> } >> >> switch (msg_request) { >> case VHOST_USER_GET_FEATURES: >> if (msg.size != sizeof(m.u64)) { >> error_report("Received bad msg size."); >> - return -1; >> + goto close; >> } >> *((__u64 *) arg) = msg.u64; >> break; >> case VHOST_USER_GET_VRING_BASE: >> if (msg.size != sizeof(m.state)) { >> error_report("Received bad msg size."); >> - return -1; >> + goto close; >> } >> memcpy(arg, &msg.state, sizeof(struct vhost_vring_state)); >> break; >> default: >> error_report("Received unexpected msg type."); >> - return -1; >> - break; >> } >> } >> >> return 0; >> + >> +close: >> + qemu_chr_shutdown(chr, SHUT_RDWR); >> + return -1; >> } > Why is shutdown(2) necessary? We're aborting the connection and don't > expect to process any more data, so we could just close it. Yes, you are right. It's my fault and here is current wrong behavior of this patch. 1. socket is shutdown by QEMU. 2. Vhost-user backend detects it, because socket is shutdown. 3. Vhost-user backend close socket. (This behavior is came from my test program) 4. QEMU detects it, then start closing event handling. Apparently, I needed to close socket by QEMU. So I've checked the qemu-char code more. And I've found 'tcp_chr_disconnect' is the function I need to call, also I've found I may not be able to use 'tcp_chr_close' for my purpose, because 'tcp_chr_close' closes not only accepted fd, but also listen fd. In my case, I just want to close accepted fd. Because of above, 'qemu_chr_delete' that calls 'tcp_chr_close' is not for my purpose. Unfortunately, there is no function like 'qemu_chr_disconnect' in qemu-char. So I may need to add it instead of 'qemu_chr_shutdown' in next patches. Is this correct direction? > >> diff --git a/include/sysemu/char.h b/include/sysemu/char.h >> index 832b7fe..d944ded 100644 >> --- a/include/sysemu/char.h >> +++ b/include/sysemu/char.h >> @@ -70,6 +70,7 @@ struct CharDriverState { >> IOReadHandler *chr_read; >> void *handler_opaque; >> void (*chr_close)(struct CharDriverState *chr); >> + void (*chr_shutdown)(struct CharDriverState *chr, int how); >> void (*chr_accept_input)(struct CharDriverState *chr); >> void (*chr_set_echo)(struct CharDriverState *chr, bool echo); >> void (*chr_set_fe_open)(struct CharDriverState *chr, int fe_open); >> @@ -124,6 +125,12 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts, >> */ >> CharDriverState *qemu_chr_new(const char *label, const char *filename, >> void (*init)(struct CharDriverState *s)); >> +/** >> + * @qemu_chr_shutdown: >> + * >> + * Shutdown a fd of character backend. >> + */ >> +void qemu_chr_shutdown(CharDriverState *chr, int how); >> >> /** >> * @qemu_chr_delete: >> diff --git a/qemu-char.c b/qemu-char.c >> index d0c1564..2b28808 100644 >> --- a/qemu-char.c >> +++ b/qemu-char.c >> @@ -2839,6 +2839,13 @@ static void tcp_chr_disconnect(CharDriverState *chr) >> } >> } >> >> +static void tcp_chr_shutdown(CharDriverState *chr, int how) >> +{ >> + TCPCharDriver *s = chr->opaque; >> + >> + shutdown(s->fd, how); >> +} >> + >> static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque) >> { >> CharDriverState *chr = opaque; >> @@ -3836,6 +3843,13 @@ void qemu_chr_fe_release(CharDriverState *s) >> s->avail_connections++; >> } >> >> +void qemu_chr_shutdown(CharDriverState *chr, int how) >> +{ >> + if (chr->chr_shutdown) { >> + chr->chr_shutdown(chr, how); >> + } >> +} >> + >> void qemu_chr_delete(CharDriverState *chr) >> { >> QTAILQ_REMOVE(&chardevs, chr, next); >> @@ -4154,6 +4168,7 @@ static CharDriverState *qmp_chardev_open_socket(ChardevSocket *sock, >> chr->chr_write = tcp_chr_write; >> chr->chr_sync_read = tcp_chr_sync_read; >> chr->chr_close = tcp_chr_close; >> + chr->chr_shutdown = tcp_chr_shutdown; >> chr->get_msgfds = tcp_get_msgfds; >> chr->set_msgfds = tcp_set_msgfds; >> chr->chr_add_client = tcp_chr_add_client; > Please split this into a separate qemu-char.c patch. I hesitate to add > a shutdown(2) interface since it adds a new state that the rest of the > qemu-char.c code doesn't know about. As described above, I may need to add 'qemu_chr_disconnect'. It will be the interface of closing accepted fd. Regards, Tetsuya