Re: [Qemu-devel] [RFC] contrib: add vhost-user-sim

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4216 bytes --]

On Tue, Sep 17, 2019 at 02:26:44PM +0200, Johannes Berg wrote:
> +static int unix_sock_new(const char *unix_fn)
> +{
> +    int sock;
> +    struct sockaddr_un un;
> +    size_t len;
> +
> +    g_assert(unix_fn);
> +
> +    sock = socket(AF_UNIX, SOCK_STREAM, 0);
> +    if (sock <= 0) {
> +        perror("socket");
> +        g_assert(0);
> +        return -1;
> +    }
> +
> +    un.sun_family = AF_UNIX;
> +    (void)snprintf(un.sun_path, sizeof(un.sun_path), "%s", unix_fn);
> +    len = sizeof(un.sun_family) + strlen(un.sun_path);

According to unix(7):

  The addrlen argument that describes the enclosing sockaddr_un
  structure should have a value of at least:

    offsetof(struct sockaddr_un, sun_path)+strlen(addr.sun_path)+1

  or, more simply, addrlen can be specified as sizeof(struct sockaddr_un).

Please either increase len by 1 or use sizeof(struct sockaddr_un).

> +gboolean vu_net_client_connected(GIOChannel *src,
> +                                 GIOCondition cond,
> +                                 gpointer data)
> +{
> +    int lsock = g_io_channel_unix_get_fd(src);
> +    int csock = accept(lsock, NULL, NULL);
> +    VuNetDev *ndev;
> +
> +    if (csock < 0) {
> +        fprintf(stderr, "Accept error %s\n", strerror(errno));
> +        return TRUE;
> +    }
> +
> +    ndev = g_new0(VuNetDev, 1);
> +    if (!ndev) {

g_new0() cannot return NULL, please remove this if statement.

> +static int full_read(int fd, void *_buf, size_t len)
> +{
> +    unsigned char *buf = _buf;
> +    ssize_t ret;
> +
> +    do {
> +        ret = read(fd, buf, len);
> +        if (ret > 0) {
> +            buf += ret;
> +            len -= ret;
> +        } else if (ret == 0) {
> +            return 0;
> +        } else {
> +            return -errno;
> +        }

Want to loop on EINTR?

> +    } while (len > 0);
> +
> +    return buf - (unsigned char *)_buf;
> +}
> +
> +static int full_write(int fd, const void *_buf, size_t len)
> +{
> +    const unsigned char *buf = _buf;
> +    ssize_t ret;
> +
> +    do {
> +        ret = write(fd, buf, len);
> +        if (ret > 0) {
> +            buf += ret;
> +            len -= ret;
> +        } else if (ret == 0) {
> +            return 0;
> +        } else {
> +            return -errno;

EINTR?

> +        }
> +    } while (len > 0);
> +
> +    return buf - (const unsigned char *)_buf;
> +}
> +
> +static void simtime_handle_message(SimTimeConnection *conn,
> +                                   struct um_timetravel_msg *msg)
> +{
> +    struct um_timetravel_msg resp = {
> +        .op = UM_TIMETRAVEL_ACK,
> +    };
> +
> +    DPRINT(" %d | message %s (%lld, time=%lld)\n",
> +           conn->idx, simtime_op_str(msg->op), msg->op, msg->time);
> +
> +    switch (msg->op) {
> +    case UM_TIMETRAVEL_REQUEST:
> +        if (calendar_entry_remove(&conn->entry)) {
> +            conn->entry.time = conn->offset + msg->time;
> +            calendar_entry_add(&conn->entry);
> +            DPRINT(" %d | calendar entry added for %lld\n", conn->idx, msg->time);
> +        } else {
> +            conn->entry.time = conn->offset + msg->time;
> +            DPRINT(" %d | calendar entry time updated for %lld\n", conn->idx, msg->time);
> +        }

Just checking the expected semantics:

If the entry was already added, then we update the time and it stays
scheduled.  If the entry was not already added then we just stash away
the time but don't schedule it?

Also, are the DPRINT() messages swapped in the if ... else ... bodies?
They seem to be talking about the other case.

> +    conn = g_new0(SimTimeConnection, 1);
> +    if (!conn) {
> +        return TRUE;
> +    }

g_new0() does not fail.  If it did, then csock would be leaked here.
Please drop the if statement.

> diff --git a/include/standard-headers/linux/um_timetravel.h b/include/standard-headers/linux/um_timetravel.h
> new file mode 100644
> index 000000000000..3aaced426a92
> --- /dev/null
> +++ b/include/standard-headers/linux/um_timetravel.h
> @@ -0,0 +1,107 @@

Please use scripts/update-linux-headers.sh to import this header file
with the necessary conversions (e.g. #include <linux/types.h> ->
#include "standard-headers/linux/types.h", __u64 -> uint64_t).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]