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

[David Marchand <david.marchand@6wind.com>]

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