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

[Johannes Berg <johannes@sipsolutions.net>]

On Mon, 2019-09-23 at 10:25 +0100, Stefan Hajnoczi wrote:
> 
> 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).

Good catch, I actually realized this later as I was porting it elsewhere
but forgot to update this code.

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

:-)

Shows I don't work with glib much ...

> > +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?

Can we even get EINTR? We're not really expecting to deal with any
signals in this code, do we?

But I guess we can.

> > +    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?

Right.

The first case is the "normally running" case, where the request is
coming in after UM_TIMETRAVEL_WAIT but before we sent it
UM_TIMETRAVEL_RUN, i.e. when the sender of the message got an interrupt
from elsewhere and needs to change the next runtime.

The second case is when it's requesting before it went into
UM_TIMETRAVEL_WAIT, in which case we just want the entry to the calendar
to be added for when we actually get _WAIT.

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

Or better rephrased I guess :)

> > 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).

Hah, sure, wasn't aware of that.


Note, I'm not happy with this code at all, it deadlocks all the time.
Unfortunately I haven't really been able to figure out how to make glib
do what I wanted.

The first issue is that sometimes glib actually seems to reports an FD
as readable when it's not, so I even put them into non-blocking mode :(

The second is that I can't seem to understand how to do recursive
mainloops.

To really do this *well*, it should remain a single-threaded
application, but would need a hook like

run_mainloop_until_fd_readable(vdev->call_fd)

inside the libvhost-user.c code, and that's a bit ugly ... if I even
could figure out how to implement that in glib.

johannes