From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57684) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bSVTD-00027k-EQ for qemu-devel@nongnu.org; Wed, 27 Jul 2016 16:30:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bSVT8-0002Bs-FE for qemu-devel@nongnu.org; Wed, 27 Jul 2016 16:30:23 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58782) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bSVT8-0002BW-6i for qemu-devel@nongnu.org; Wed, 27 Jul 2016 16:30:18 -0400 From: Bandan Das References: <13b6f9d93dc97dc36cc89867348c00f13d49526d.1468876280.git.109lozanoi@gmail.com> Date: Wed, 27 Jul 2016 16:30:15 -0400 In-Reply-To: <13b6f9d93dc97dc36cc89867348c00f13d49526d.1468876280.git.109lozanoi@gmail.com> (Isaac Lozano's message of "Thu, 21 Jul 2016 02:56:33 -0700") Message-ID: MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v2 1/2] usb-mtp: fix sending files larger than 4gb List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Isaac Lozano <109lozanoi@gmail.com> Cc: qemu-devel@nongnu.org, kraxel@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