[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.