Re: [Qemu-devel] [PATCH v3 1/2] contrib: add ivshmem client and server

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux