Re: [Xen-devel] [PATCH v3 02/25] chardev: Assert IOCanReadHandler can not be negative

[Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>]

Hi

On Wed, Feb 20, 2019 at 2:03 AM Philippe Mathieu-Daudé
<philmd@xxxxxxxxxx> wrote:
>
> The backend should not return a negative length to read.
> We will later change the prototype of IOCanReadHandler to return an
> unsigned length. Meanwhile make sure the return length is positive.
>
> Suggested-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>

In such patch, you should do extensive review of existing callbacks,
or find a convincing argument that this can't break.

The problem is there are a lot of can_read callbacks, and it's not
trivial. The *first* of git-grep is rng_egd_chr_can_read()

 57     QSIMPLEQ_FOREACH(req, &s->parent.requests, next) {
 58         size += req->size - req->offset;
 59     }
 60
 61     return size;

Clearly not obvious if it returns >= 0.

Another approach is to look at the caller and the return value
handling. If none handle negative values (or would have wrong
behaviour with negative values), the assert() is perhaps justified, as
it could prevent from doing more harm.

> ---
>  chardev/char.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index f6d61fa5f8..71ecd32b25 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -159,12 +159,15 @@ int qemu_chr_write(Chardev *s, const uint8_t *buf, int 
> len, bool write_all)
>  int qemu_chr_be_can_write(Chardev *s)
>  {
>      CharBackend *be = s->be;
> +    int receivable_bytes;
>
>      if (!be || !be->chr_can_read) {
>          return 0;
>      }
>
> -    return be->chr_can_read(be->opaque);
> +    receivable_bytes = be->chr_can_read(be->opaque);
> +    assert(receivable_bytes >= 0);
> +    return receivable_bytes;
>  }
>
>  void qemu_chr_be_write_impl(Chardev *s, uint8_t *buf, int len)
> --
> 2.20.1
>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel