Re: [Qemu-devel] [PATCH 27/38] ivshmem: Simplify how we cope with short reads from server

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> Hi
>
> On Mon, Feb 29, 2016 at 7:40 PM, Markus Armbruster <address@hidden> wrote:
>> Short reads from a UNIX domain sockets are exceedingly unlikely when
>> the other side always sends eight bytes and we always read eight
>> bytes.  We cope with them anyway.  However, the code doing that is
>> rather convoluted.  Dumb it down radically.
>>
>> Replace the convoluted code
>
> agreed
>
>>
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  hw/misc/ivshmem.c | 76 
>> ++++++++++++-------------------------------------------
>>  1 file changed, 16 insertions(+), 60 deletions(-)
>>
>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>> index e578b8a..fb8a4f7 100644
>> --- a/hw/misc/ivshmem.c
>> +++ b/hw/misc/ivshmem.c
>> @@ -26,7 +26,6 @@
>>  #include "migration/migration.h"
>>  #include "qemu/error-report.h"
>>  #include "qemu/event_notifier.h"
>> -#include "qemu/fifo8.h"
>>  #include "sysemu/char.h"
>>  #include "sysemu/hostmem.h"
>>  #include "qapi/visitor.h"
>> @@ -80,7 +79,6 @@ typedef struct IVShmemState {
>>      uint32_t intrstatus;
>>
>>      CharDriverState *server_chr;
>> -    Fifo8 incoming_fifo;
>>      MemoryRegion ivshmem_mmio;
>>
>>      /* We might need to register the BAR before we actually have the memory.
>> @@ -99,6 +97,8 @@ typedef struct IVShmemState {
>>      uint32_t vectors;
>>      uint32_t features;
>>      MSIVector *msi_vectors;
>> +    uint64_t msg_buf;           /* buffer for receiving server messages */
>> +    int msg_buffered_bytes;     /* #bytes in @msg_buf */
>>
>>      Error *migration_blocker;
>>
>> @@ -255,11 +255,6 @@ static const MemoryRegionOps ivshmem_mmio_ops = {
>>      },
>>  };
>>
>> -static int ivshmem_can_receive(void * opaque)
>> -{
>> -    return sizeof(int64_t);
>> -}
>> -
>>  static void ivshmem_vector_notify(void *opaque)
>>  {
>>      MSIVector *entry = opaque;
>> @@ -459,53 +454,6 @@ static void resize_peers(IVShmemState *s, int nb_peers)
>>      }
>>  }
>>
>> -static bool fifo_update_and_get(IVShmemState *s, const uint8_t *buf, int 
>> size,
>> -                                void *data, size_t len)
>> -{
>> -    const uint8_t *p;
>> -    uint32_t num;
>> -
>> -    assert(len <= sizeof(int64_t)); /* limitation of the fifo */
>> -    if (fifo8_is_empty(&s->incoming_fifo) && size == len) {
>> -        memcpy(data, buf, size);
>> -        return true;
>> -    }
>> -
>> -    IVSHMEM_DPRINTF("short read of %d bytes\n", size);
>> -
>> -    num = MIN(size, sizeof(int64_t) - fifo8_num_used(&s->incoming_fifo));
>> -    fifo8_push_all(&s->incoming_fifo, buf, num);
>> -
>> -    if (fifo8_num_used(&s->incoming_fifo) < len) {
>> -        assert(num == 0);
>> -        return false;
>> -    }
>> -
>> -    size -= num;
>> -    buf += num;
>> -    p = fifo8_pop_buf(&s->incoming_fifo, len, &num);
>> -    assert(num == len);
>> -
>> -    memcpy(data, p, len);
>> -
>> -    if (size > 0) {
>> -        fifo8_push_all(&s->incoming_fifo, buf, size);
>> -    }
>> -
>> -    return true;
>> -}
>> -
>> -static bool fifo_update_and_get_i64(IVShmemState *s,
>> -                                    const uint8_t *buf, int size, int64_t 
>> *i64)
>> -{
>> -    if (fifo_update_and_get(s, buf, size, i64, sizeof(*i64))) {
>> -        *i64 = GINT64_FROM_LE(*i64);
>> -        return true;
>> -    }
>> -
>> -    return false;
>> -}
>> -
>>  static void ivshmem_add_kvm_msi_virq(IVShmemState *s, int vector,
>>                                       Error **errp)
>>  {
>> @@ -658,6 +606,14 @@ static void process_msg(IVShmemState *s, int64_t msg, 
>> int fd, Error **errp)
>>      }
>>  }
>>
>> +static int ivshmem_can_receive(void *opaque)
>> +{
>> +    IVShmemState *s = opaque;
>> +
>> +    assert(s->msg_buffered_bytes < sizeof(s->msg_buf));
>> +    return sizeof(s->msg_buf) - s->msg_buffered_bytes;
>> +}
>> +
>>  static void ivshmem_read(void *opaque, const uint8_t *buf, int size)
>>  {
>>      IVShmemState *s = opaque;
>> @@ -665,8 +621,12 @@ static void ivshmem_read(void *opaque, const uint8_t 
>> *buf, int size)
>>      int incoming_fd;
>>      int64_t incoming_posn;
>>
>> -    if (!fifo_update_and_get_i64(s, buf, size, &incoming_posn)) {
>> -        return;
>> +    assert(size >= 0 && s->msg_buffered_bytes + size <= sizeof(s->msg_buf));
>> +    memcpy((unsigned char *)&s->msg_buf + s->msg_buffered_bytes, buf, size);
>> +    s->msg_buffered_bytes += size;
>> +    if (s->msg_buffered_bytes == sizeof(s->msg_buf)) {
>> +        incoming_posn = le64_to_cpu(s->msg_buf);
>> +        s->msg_buffered_bytes = 0;
>>      }
>>
>
> missing "else return" though.

Indeed.  Glad you caught my screwup.

>>      incoming_fd = qemu_chr_fe_get_msgfd(s->server_chr);
>> @@ -1019,8 +979,6 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error 
>> **errp)
>>          }
>>      }
>>
>> -    fifo8_create(&s->incoming_fifo, sizeof(int64_t));
>> -
>>      if (s->role_val == IVSHMEM_PEER) {
>>          error_setg(&s->migration_blocker,
>>                     "Migration is disabled when using feature 'peer mode' in 
>> device 'ivshmem'");
>> @@ -1033,8 +991,6 @@ static void pci_ivshmem_exit(PCIDevice *dev)
>>      IVShmemState *s = IVSHMEM(dev);
>>      int i;
>>
>> -    fifo8_destroy(&s->incoming_fifo);
>> -
>>      if (s->migration_blocker) {
>>          migrate_del_blocker(s->migration_blocker);
>>          error_free(s->migration_blocker);
>> --
>> 2.4.3
>>
>>