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

[David Marchand]

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