Re: [Qemu-devel] [RFC PATCH v3 13/25] xen: Let buffer_append() return a size_t

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Philippe Mathieu-Daudé [mailto:philmd@redhat.com]
> Sent: 20 February 2019 01:02
> To: qemu-devel@nongnu.org; Prasad J Pandit <pjp@fedoraproject.org>; Marc-
> André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> <pbonzini@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>; Anthony Perard
> <anthony.perard@citrix.com>; qemu-ppc@nongnu.org; Stefan Berger
> <stefanb@linux.ibm.com>; David Gibson <david@gibson.dropbear.id.au>; Gerd
> Hoffmann <kraxel@redhat.com>; Zhang Chen <zhangckid@gmail.com>; xen-
> devel@lists.xenproject.org; Cornelia Huck <cohuck@redhat.com>; Samuel
> Thibault <samuel.thibault@ens-lyon.org>; Christian Borntraeger
> <borntraeger@de.ibm.com>; Amit Shah <amit@kernel.org>; Li Zhijian
> <lizhijian@cn.fujitsu.com>; Corey Minyard <minyard@acm.org>; Michael S.
> Tsirkin <mst@redhat.com>; Paul Durrant <Paul.Durrant@citrix.com>; Halil
> Pasic <pasic@linux.ibm.com>; Stefano Stabellini <sstabellini@kernel.org>;
> qemu-s390x@nongnu.org; Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>;
> Philippe Mathieu-Daudé <philmd@redhat.com>
> Subject: [RFC PATCH v3 13/25] xen: Let buffer_append() return a size_t
> 
> To the Xen team: this is not trivial to me to demonstrate
> this assertion can never happen, but then the whole series
> is justified and I can convert qemu_chr_fe_write() to use
> size_t argument.
> Can you help me here?

I'm not particularly familiar with this bit of code but I can try...

> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/char/xen_console.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
> index 1a30014a11..5b672a5a24 100644
> --- a/hw/char/xen_console.c
> +++ b/hw/char/xen_console.c
> @@ -92,6 +92,7 @@ static ssize_t buffer_append(struct XenConsole *con)
>      }
> 
>   out:
> +    assert(buffer->size >= buffer->consumed);
>      return buffer->size - buffer->consumed;

I think this assertion is reasonable as:

- buffer_advance() appears to hit a termination condition when buffer->consumed == buffer->size. (Nothing checks for overflow which is bad, but that fact also lends weight to the assertion that consumed > size is a bug).
- if buffer->size ever exceeds buffer->max_capacity then both size and consumed are re-calculated such that consumed <= size.

  Paul

>  }
> 
> --
> 2.20.1