On 12/10/2015 01:56 PM, Victor Kaplansky wrote:
On Wed, Dec 09, 2015 at 06:06:06PM +0100, Didier Pallard wrote:On 12/09/2015 04:59 PM, Victor Kaplansky wrote:On Mon, Dec 07, 2015 at 02:31:36PM +0100, Marc-André Lureau wrote:HiOn Thu, Dec 3, 2015 at 10:53 AM, Didier Pallard <[email protected]> wrote:unix_send_msgfds is used by vhost-user control socket. qemu_chr_fe_write_all is used to send a message and retries as long as EAGAIN errno is set, but write_msgfds buffer is freed after first EAGAIN failure, causing message to be sent without proper fds attachment. In case unix_send_msgfds is called through qemu_chr_fe_write, it will be user responsability to resend message as is or to free write_msgfds using set_msgfds(0) Signed-off-by: Didier Pallard <[email protected]> Reviewed-by: Thibaut Collet <[email protected]> --- qemu-char.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/qemu-char.c b/qemu-char.c index 5448b0f..26d5f2e 100644 --- a/qemu-char.c +++ b/qemu-char.c @@ -2614,6 +2614,16 @@ static int unix_send_msgfds(CharDriverState *chr, const uint8_t *buf, int len) r = sendmsg(s->fd, &msgh, 0); } while (r < 0 && errno == EINTR); + /* Ancillary data are not sent if no byte is written + * so don't free msgfds buffer if return value is EAGAIN + * If called from qemu_chr_fe_write_all retry will come soon + * If called from qemu_chr_fe_write, it is the user responsibility + * to resend message or free fds using set_msgfds(0) + */ + if (r < 0 && errno == EAGAIN) { + return r; + } +This looks reasonable to me. However, I don't know what happens with partial write of ancillary data. Hopefully it's all or nothing. Apparently, reading unix_stream_sendmsg() in kernel shows that as long as a few bytes have been sent, the ancillary data is sent. So it looks like it still does the right thing in case of a partial write.If I may put my two cents in, it looks to me very similar to an fd leakage on back-end side. When a new set_call_fd request arrives, it is very easy to forget closing the previous file descriptor. As result, if interrupts are actively maksed/unmasked by the guest, the back-end can easily reach maximum fds, which will cause receiving side silently drop new fds in aux data. --VictorHi victor, This is not a problem of fd exausted. This was my first axe of investigation, but fd management is correct in our vhost-user backend, there is no fd leakage.That's good.And i guess you are refering to the problem fixed by patches 2 and 3, since the problem corrected by this patch is a message arriving from qemu without ancillary data, whatever the state of the fds in the vhost-user backend.I'm talking about the problem that supposed to be fixed by the first patch. It is not clear to me how the patch fixes the partial send. sendmsg() is called in qemu-char.c:unix_send_msgfds with zero flags, which means a blocking operation, so I'm surprised that sendmsg can return with errno == EAGAIN.
Well, vhost-user socket is started with following chardev:
-chardev socket,id=vhostuserchr0,path=/tmp/vhost_sock0,server
and according to code in tcp_chr_add_client:
static int tcp_chr_add_client(CharDriverState *chr, int fd)
{
...
qemu_set_nonblock(fd);
So fd is set in non blocking mode. This is enough to have an
EAGAIN returned value on socket buffer full, whatever flags used in
sendmsg, i think.
Perhaps changing the blocking mode here may also correct the first
problem, but I am not able to measure the impact that may have such a
modification...
thanks didierReviewed-by: Marc-André Lureau <[email protected]>/* free the written msgfds, no matter what */ if (s->write_msgfds_num) { g_free(s->write_msgfds); -- 2.1.4-- Marc-André Lureau
