| Message ID | 20180625124238.25339-36-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | Use the IEC binary prefix definitions | expand |
On 06/25/2018 05:42 AM, Philippe Mathieu-Daudé wrote: > -#define VSCARD_IN_SIZE 65536 > +#define VSCARD_IN_SIZE (64 * KiB) > > /* maximum size of ATR - from 7816-3 */ > #define MAX_ATR_SIZE 40 > @@ -276,7 +277,7 @@ static void ccid_card_vscard_read(void *opaque, const uint8_t *buf, int size) > > if (card->vscard_in_pos + size > VSCARD_IN_SIZE) { > error_report( > - "no room for data: pos %d + size %d > %d. dropping connection.", > + "no room for data: pos %u + size %d > %ld. dropping connection.", > card->vscard_in_pos, size, VSCARD_IN_SIZE); Did you test this with i686 host? %ld doesn't look right. r~
On 06/27/2018 04:04 AM, Richard Henderson wrote: > On 06/25/2018 05:42 AM, Philippe Mathieu-Daudé wrote: >> -#define VSCARD_IN_SIZE 65536 >> +#define VSCARD_IN_SIZE (64 * KiB) >> >> /* maximum size of ATR - from 7816-3 */ >> #define MAX_ATR_SIZE 40 >> @@ -276,7 +277,7 @@ static void ccid_card_vscard_read(void *opaque, const uint8_t *buf, int size) >> >> if (card->vscard_in_pos + size > VSCARD_IN_SIZE) { >> error_report( >> - "no room for data: pos %d + size %d > %d. dropping connection.", >> + "no room for data: pos %u + size %d > %ld. dropping connection.", >> card->vscard_in_pos, size, VSCARD_IN_SIZE); > > Did you test this with i686 host? %ld doesn't look right. Yes... $ uname -m x86_64 $ make hw/usb/ccid-card-passthru.o CC hw/usb/ccid-card-passthru.o $ If there are no other changes asked on this series, the maintainer taking this can update to use PRId64 with: -- >8 -- diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c --- a/hw/usb/ccid-card-passthru.c +++ b/hw/usb/ccid-card-passthru.c @@ -276,9 +276,9 @@ static void ccid_card_vscard_read(void *opaque, const uint8_t *buf, int size) VSCMsgHeader *hdr; if (card->vscard_in_pos + size > VSCARD_IN_SIZE) { - error_report( - "no room for data: pos %u + size %d > %ld. dropping connection.", - card->vscard_in_pos, size, VSCARD_IN_SIZE); + error_report("no room for data: pos %u + size %d > %" PRId64 "." + " dropping connection.", + card->vscard_in_pos, size, VSCARD_IN_SIZE); ccid_card_vscard_drop_connection(card); return; } -- Or I can send as a cleanup patch once the series get merged. Thanks for your review! Phil.
On 06/27/2018 06:03 AM, Philippe Mathieu-Daudé wrote: > On 06/27/2018 04:04 AM, Richard Henderson wrote: >> On 06/25/2018 05:42 AM, Philippe Mathieu-Daudé wrote: >>> -#define VSCARD_IN_SIZE 65536 >>> +#define VSCARD_IN_SIZE (64 * KiB) >>> >>> /* maximum size of ATR - from 7816-3 */ >>> #define MAX_ATR_SIZE 40 >>> @@ -276,7 +277,7 @@ static void ccid_card_vscard_read(void *opaque, const uint8_t *buf, int size) >>> >>> if (card->vscard_in_pos + size > VSCARD_IN_SIZE) { >>> error_report( >>> - "no room for data: pos %d + size %d > %d. dropping connection.", >>> + "no room for data: pos %u + size %d > %ld. dropping connection.", >>> card->vscard_in_pos, size, VSCARD_IN_SIZE); >> >> Did you test this with i686 host? %ld doesn't look right. > > Yes... > > $ uname -m > x86_64 > $ make hw/usb/ccid-card-passthru.o > CC hw/usb/ccid-card-passthru.o > $ Ah, no, I mean 32-bit i686, not x86_64. r~
On 06/27/2018 10:47 AM, Richard Henderson wrote: > On 06/27/2018 06:03 AM, Philippe Mathieu-Daudé wrote: >> On 06/27/2018 04:04 AM, Richard Henderson wrote: >>> On 06/25/2018 05:42 AM, Philippe Mathieu-Daudé wrote: >>>> -#define VSCARD_IN_SIZE 65536 >>>> +#define VSCARD_IN_SIZE (64 * KiB) >>>> >>>> /* maximum size of ATR - from 7816-3 */ >>>> #define MAX_ATR_SIZE 40 >>>> @@ -276,7 +277,7 @@ static void ccid_card_vscard_read(void *opaque, const uint8_t *buf, int size) >>>> >>>> if (card->vscard_in_pos + size > VSCARD_IN_SIZE) { >>>> error_report( >>>> - "no room for data: pos %d + size %d > %d. dropping connection.", >>>> + "no room for data: pos %u + size %d > %ld. dropping connection.", >>>> card->vscard_in_pos, size, VSCARD_IN_SIZE); >>> >>> Did you test this with i686 host? %ld doesn't look right. >> >> Yes... >> >> $ uname -m >> x86_64 >> $ make hw/usb/ccid-card-passthru.o >> CC hw/usb/ccid-card-passthru.o >> $ > > Ah, no, I mean 32-bit i686, not x86_64. I built using the MXE MinGW32 toolchain, but since the libcacard is not packaged for this target I didn't notice... win32# make hw/usb/ccid-card-passthru.o CC hw/usb/ccid-card-passthru.o hw/usb/ccid-card-passthru.c:13:23: fatal error: libcacard.h: No such file or directory compilation terminated. win32# apt install libcacard-dev E: Unable to locate package libcacard-dev I forgot to run 'make vm-build-ubuntu.i386' on this series.
diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c index 25fb19b0d7..e75d433e56 100644 --- a/hw/usb/ccid-card-passthru.c +++ b/hw/usb/ccid-card-passthru.c @@ -9,6 +9,7 @@ */ #include "qemu/osdep.h" +#include "qemu/units.h" #include <libcacard.h> #include "chardev/char-fe.h" #include "qemu/error-report.h" @@ -40,7 +41,7 @@ static const uint8_t DEFAULT_ATR[] = { 0x13, 0x08 }; -#define VSCARD_IN_SIZE 65536 +#define VSCARD_IN_SIZE (64 * KiB) /* maximum size of ATR - from 7816-3 */ #define MAX_ATR_SIZE 40 @@ -276,7 +277,7 @@ static void ccid_card_vscard_read(void *opaque, const uint8_t *buf, int size) if (card->vscard_in_pos + size > VSCARD_IN_SIZE) { error_report( - "no room for data: pos %d + size %d > %d. dropping connection.", + "no room for data: pos %u + size %d > %ld. dropping connection.", card->vscard_in_pos, size, VSCARD_IN_SIZE); ccid_card_vscard_drop_connection(card); return; diff --git a/hw/usb/combined-packet.c b/hw/usb/combined-packet.c index 48cac87f6a..01a7ed0848 100644 --- a/hw/usb/combined-packet.c +++ b/hw/usb/combined-packet.c @@ -20,6 +20,7 @@ * along with this program; if not, see <http://www.gnu.org/licenses/>. */ #include "qemu/osdep.h" +#include "qemu/units.h" #include "qemu-common.h" #include "hw/usb.h" #include "qemu/iov.h" @@ -171,7 +172,7 @@ void usb_ep_combine_input_packets(USBEndpoint *ep) if ((p->iov.size % ep->max_packet_size) != 0 || !p->short_not_ok || next == NULL || /* Work around for Linux usbfs bulk splitting + migration */ - (totalsize == 16348 && p->int_req)) { + (totalsize == (16 * KiB - 36) && p->int_req)) { usb_device_handle_data(ep->dev, first); assert(first->status == USB_RET_ASYNC); if (first->combined) { diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c index 13d0befd9c..8f716fc165 100644 --- a/hw/usb/dev-smartcard-reader.c +++ b/hw/usb/dev-smartcard-reader.c @@ -35,6 +35,7 @@ */ #include "qemu/osdep.h" +#include "qemu/units.h" #include "qapi/error.h" #include "qemu-common.h" #include "qemu/error-report.h" @@ -63,7 +64,7 @@ do { \ * or handle the migration complexity - VMState doesn't handle this case. * sizes are expected never to be exceeded, unless guest misbehaves. */ -#define BULK_OUT_DATA_SIZE 65536 +#define BULK_OUT_DATA_SIZE (64 * KiB) #define PENDING_ANSWERS_NUM 128 #define BULK_IN_BUF_SIZE 384 diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c index 58e8f7f5bd..99094a721e 100644 --- a/hw/usb/redirect.c +++ b/hw/usb/redirect.c @@ -26,6 +26,7 @@ */ #include "qemu/osdep.h" +#include "qemu/units.h" #include "qapi/error.h" #include "qemu-common.h" #include "qemu/timer.h" @@ -1298,7 +1299,7 @@ static int usbredir_chardev_can_read(void *opaque) } /* usbredir_parser_do_read will consume *all* data we give it */ - return 1024 * 1024; + return 1 * MiB; } static void usbredir_chardev_read(void *opaque, const uint8_t *buf, int size)
It eases code review, unit is explicit. Patch generated using: $ git grep -E '(1024|2048|4096|8192|(<<|>>).?(10|20|30))' hw/ include/hw/ and modified manually. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- hw/usb/ccid-card-passthru.c | 5 +++-- hw/usb/combined-packet.c | 3 ++- hw/usb/dev-smartcard-reader.c | 3 ++- hw/usb/redirect.c | 3 ++- 4 files changed, 9 insertions(+), 5 deletions(-)