Re: [Qemu-devel] [PATCH v2 1/2] usb-mtp: fix sending files larger than 4gb

[Bandan Das <bsd@redhat.com>]

Hi Isaac,

Some minor comments below.

Isaac Lozano <109lozanoi@gmail.com> writes:

> MTP requires that if a file is larger than 4gb or if sending data larger
> than 4gb, that the length field be set to 0xFFFFFFFF.
>
> Also widened a couple variables to prevent overflow errors.
>
> Signed-off-by: Isaac Lozano <109lozanoi@gmail.com>
> ---
>  hw/usb/dev-mtp.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
> index 1be85ae..6f6a270 100644
> --- a/hw/usb/dev-mtp.c
> +++ b/hw/usb/dev-mtp.c
> @@ -115,8 +115,8 @@ struct MTPControl {
>  struct MTPData {
>      uint16_t     code;
>      uint32_t     trans;
> -    uint32_t     offset;
> -    uint32_t     length;
> +    uint64_t     offset;
> +    uint64_t     length;
>      uint32_t     alloc;
>      uint8_t      *data;
>      bool         first;
> @@ -883,7 +883,13 @@ static MTPData *usb_mtp_get_object_info(MTPState *s, MTPControl *c,
>      usb_mtp_add_u32(d, QEMU_STORAGE_ID);
>      usb_mtp_add_u16(d, o->format);
>      usb_mtp_add_u16(d, 0);
> -    usb_mtp_add_u32(d, o->stat.st_size);
> +
> +    if (o->stat.st_size > 0xFFFFFFFF) {
> +        usb_mtp_add_u32(d, 0xFFFFFFFF);
> +    }
> +    else {
> +        usb_mtp_add_u32(d, o->stat.st_size);
> +    }

Or rewrite as:
	  uint32_t size =
	  	   o->stat.st_size > 0xFFFFFFFF ? 0xFFFFFFFF : o->stat.st_size;
	  usb_mtp_add_u32(d, size);

Either way is fine.


>  
>      usb_mtp_add_u16(d, 0);
>      usb_mtp_add_u32(d, 0);
> @@ -1193,10 +1199,15 @@ static void usb_mtp_handle_data(USBDevice *dev, USBPacket *p)
>          }
>          if (s->data_in !=  NULL) {
>              MTPData *d = s->data_in;
> -            int dlen = d->length - d->offset;
> +            uint64_t dlen = d->length - d->offset;
>              if (d->first) {
>                  trace_usb_mtp_data_in(s->dev.addr, d->trans, d->length);
> -                container.length = cpu_to_le32(d->length + sizeof(container));
> +                if (d->length + sizeof(container) > 0xFFFFFFFF) {
> +                    container.length = cpu_to_le32(0xFFFFFFFF);
> +                }
> +                else {
> +                    container.length = cpu_to_le32(d->length + sizeof(container));
> +                }

This will throw checkpatch errors and I see you have fixed them in the next patch.
Please just use the preferred style here to keep context.

>                  container.type   = cpu_to_le16(TYPE_DATA);
>                  container.code   = cpu_to_le16(d->code);
>                  container.trans  = cpu_to_le32(d->trans);

Please feel free to add
Reviewed-by: Bandan Das <bsd@redhat.com>