From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Marchand Subject: Re: [Qemu-devel] [PATCH v3 1/2] contrib: add ivshmem client and server Date: Mon, 18 Aug 2014 14:19:52 +0200 Message-ID: <53F1EF68.9090408@6wind.com> References: <1407488118-11245-1-git-send-email-david.marchand@6wind.com> <1407488118-11245-2-git-send-email-david.marchand@6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Cc: 'Olivier Matz' , kvm@vger.kernel.org, claudio.fontana@huawei.com, armbru@redhat.com, pbonzini@redhat.com, jani.kokkonen@huawei.com, cam@cs.ualberta.ca, arei.gonglei@huawei.com To: Gonglei , qemu-devel@nongnu.org Return-path: Received: from mail-wi0-f173.google.com ([209.85.212.173]:49384 "EHLO mail-wi0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750710AbaHRMT6 (ORCPT ); Mon, 18 Aug 2014 08:19:58 -0400 Received: by mail-wi0-f173.google.com with SMTP id f8so3608843wiw.12 for ; Mon, 18 Aug 2014 05:19:57 -0700 (PDT) In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On 08/10/2014 05:57 AM, Gonglei wrote: >> + /* can return a peer or the local client */ >> + peer = ivshmem_client_search_peer(client, peer_id); >> + >> + /* delete peer */ >> + if (fd == -1) { >> + > Maybe the above check should be moved before getting the peer. > And the next check peer is extra. We always need to know the peer, either for a deletion, creation or update. > >> + if (peer == NULL || peer == &client->local) { >> + debug_log(client, "receive delete for invalid peer %ld", > > Missing '\n' ? Ok. > >> peer_id); >> + return -1; >> + } >> + >> + debug_log(client, "delete peer id = %ld\n", peer_id); >> + free_peer(client, peer); >> + return 0; >> + } >> + >> + /* new peer */ >> + if (peer == NULL) { >> + peer = malloc(sizeof(*peer)); > > g_malloc0 ?. Ok, replaced malloc/free with g_malloc/g_free in client and server. >> + client->notif_cb = notif_cb; >> + client->notif_arg = notif_arg; >> + client->verbose = verbose; > > Missing client->sock_fd = -1; ? Ok. >> + >> + /* first, we expect our index + a fd == -1 */ >> + if (read_one_msg(client, &client->local.id, &fd) < 0 || >> + client->local.id < 0 || fd != -1) { > Why not fd < 0 ? Because the server will send us an id and a fd == -1 see ivshmem-server.c send_initial_info(). > >> + debug_log(client, "cannot read from server\n"); >> + close(client->sock_fd); >> + client->sock_fd = -1; >> + return -1; >> + } >> + debug_log(client, "our_id=%ld\n", client->local.id); >> + >> + /* now, we expect shared mem fd + a -1 index, note that shm fd >> + * is not used */ >> + if (read_one_msg(client, &tmp, &fd) < 0 || >> + tmp != -1 || fd < 0) { >> + debug_log(client, "cannot read from server (2)\n"); >> + close(client->sock_fd); >> + client->sock_fd = -1; >> + return -1; > I think the error logic handle can move the end of this function, reducing > duplicated code. Something like this: > > goto err; > } > err: > debug_log(client, "cannot read from server (2)\n"); > close(client->sock_fd); > client->sock_fd = -1; > return -1; Ok, I also updated the server. >> + int fd; >> + >> + if (vector > peer->vectors_count) { > > Maybe if (vector >= peer->vectors_count) , otherwise the > peer->vectors[] array bounds. Oh yes, good catch. It should not happen, at the moment, but it is wrong, indeed. >> +/* send a notification to all vectors of a peer */ >> +int >> +ivshmem_client_notify_all_vects(const struct ivshmem_client *client, >> + const struct ivshmem_client_peer >> *peer) >> +{ >> + unsigned vector; >> + int ret = 0; >> + >> + for (vector = 0; vector < peer->vectors_count; vector++) { >> + if (ivshmem_client_notify(client, peer, vector) < 0) { >> + ret = -1; > The ret's value will be covered when multi clients failed. Do we need > store the failed status for server?. It indicates that we could not notify *all* clients. Thanks Gonglei. -- David Marchand From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37712) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XJLuu-00083q-Ki for qemu-devel@nongnu.org; Mon, 18 Aug 2014 08:20:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XJLuo-0002rA-E3 for qemu-devel@nongnu.org; Mon, 18 Aug 2014 08:20:04 -0400 Received: from mail-wi0-f177.google.com ([209.85.212.177]:32859) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XJLuo-0002r0-7M for qemu-devel@nongnu.org; Mon, 18 Aug 2014 08:19:58 -0400 Received: by mail-wi0-f177.google.com with SMTP id ho1so3553415wib.4 for ; Mon, 18 Aug 2014 05:19:57 -0700 (PDT) Message-ID: <53F1EF68.9090408@6wind.com> Date: Mon, 18 Aug 2014 14:19:52 +0200 From: David Marchand MIME-Version: 1.0 References: <1407488118-11245-1-git-send-email-david.marchand@6wind.com> <1407488118-11245-2-git-send-email-david.marchand@6wind.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 1/2] contrib: add ivshmem client and server List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gonglei , qemu-devel@nongnu.org Cc: 'Olivier Matz' , kvm@vger.kernel.org, claudio.fontana@huawei.com, armbru@redhat.com, arei.gonglei@huawei.com, pbonzini@redhat.com, jani.kokkonen@huawei.com, cam@cs.ualberta.ca On 08/10/2014 05:57 AM, Gonglei wrote: >> + /* can return a peer or the local client */ >> + peer = ivshmem_client_search_peer(client, peer_id); >> + >> + /* delete peer */ >> + if (fd == -1) { >> + > Maybe the above check should be moved before getting the peer. > And the next check peer is extra. We always need to know the peer, either for a deletion, creation or update. > >> + if (peer == NULL || peer == &client->local) { >> + debug_log(client, "receive delete for invalid peer %ld", > > Missing '\n' ? Ok. > >> peer_id); >> + return -1; >> + } >> + >> + debug_log(client, "delete peer id = %ld\n", peer_id); >> + free_peer(client, peer); >> + return 0; >> + } >> + >> + /* new peer */ >> + if (peer == NULL) { >> + peer = malloc(sizeof(*peer)); > > g_malloc0 ?. Ok, replaced malloc/free with g_malloc/g_free in client and server. >> + client->notif_cb = notif_cb; >> + client->notif_arg = notif_arg; >> + client->verbose = verbose; > > Missing client->sock_fd = -1; ? Ok. >> + >> + /* first, we expect our index + a fd == -1 */ >> + if (read_one_msg(client, &client->local.id, &fd) < 0 || >> + client->local.id < 0 || fd != -1) { > Why not fd < 0 ? Because the server will send us an id and a fd == -1 see ivshmem-server.c send_initial_info(). > >> + debug_log(client, "cannot read from server\n"); >> + close(client->sock_fd); >> + client->sock_fd = -1; >> + return -1; >> + } >> + debug_log(client, "our_id=%ld\n", client->local.id); >> + >> + /* now, we expect shared mem fd + a -1 index, note that shm fd >> + * is not used */ >> + if (read_one_msg(client, &tmp, &fd) < 0 || >> + tmp != -1 || fd < 0) { >> + debug_log(client, "cannot read from server (2)\n"); >> + close(client->sock_fd); >> + client->sock_fd = -1; >> + return -1; > I think the error logic handle can move the end of this function, reducing > duplicated code. Something like this: > > goto err; > } > err: > debug_log(client, "cannot read from server (2)\n"); > close(client->sock_fd); > client->sock_fd = -1; > return -1; Ok, I also updated the server. >> + int fd; >> + >> + if (vector > peer->vectors_count) { > > Maybe if (vector >= peer->vectors_count) , otherwise the > peer->vectors[] array bounds. Oh yes, good catch. It should not happen, at the moment, but it is wrong, indeed. >> +/* send a notification to all vectors of a peer */ >> +int >> +ivshmem_client_notify_all_vects(const struct ivshmem_client *client, >> + const struct ivshmem_client_peer >> *peer) >> +{ >> + unsigned vector; >> + int ret = 0; >> + >> + for (vector = 0; vector < peer->vectors_count; vector++) { >> + if (ivshmem_client_notify(client, peer, vector) < 0) { >> + ret = -1; > The ret's value will be covered when multi clients failed. Do we need > store the failed status for server?. It indicates that we could not notify *all* clients. Thanks Gonglei. -- David Marchand