Re: [PATCH] hw/us/bus.c PCAP adding 0xA in Windows version

[Thomas Huth]

On 25/02/2024 20.49, benlunt@fysnet.net wrote:
> Since Windows text files use CRLFs for all \n, the Windows version of QEMU
> inserts a CR in the PCAP stream when a LF is encountered when using USB PCAP
> files. This is due to the fact that the PCAP file is opened as TEXT instead
> of BINARY.
>
> To show an example, when using a very common protocol to USB disks, the BBB
> protocol uses a 10-byte command packet. For example, the READ_CAPACITY(10)
> command will have a command block length of 10 (0xA). When this 10-byte
> command (part of the 31-byte CBW) is placed into the PCAP file, the Windows
> file manager inserts a 0xD before the 0xA, turning the 31-byte CBW into a
> 32-byte CBW.
>
> Actual CBW:
>    0040 55 53 42 43 01 00 00 00 08 00 00 00 80 00 0a 25 USBC...........%
>    0050 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00       ...............
>
> PCAP CBW
>    0040 55 53 42 43 01 00 00 00 08 00 00 00 80 00 0d 0a USBC............
>    0050 25 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 %..............
>
> I believe simply opening the PCAP file as BINARY instead of TEXT will fix
> this issue.
>
> Resolves: https://bugs.launchpad.net/qemu/+bug/2054889
> Signed-off-by: Benjamin David Lunt <benlunt@fysnet.net>
> ---
>   hw/usb/bus.c | 2 +-
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff -u a/hw/usb/bus.c b/hw/usb/bus.c
> --- a/hw/usb/bus.c
> +++ b/hw/usb/bus.c
> @@ -273,13 +273,13 @@
>       }
>   
>       if (dev->pcap_filename) {
> -        int fd = qemu_open_old(dev->pcap_filename, O_CREAT | O_WRONLY |
> O_TRUNC, 0666);
> +        int fd = qemu_open_old(dev->pcap_filename, O_CREAT | O_WRONLY |
> O_TRUNC | O_BINARY, 0666);

 Hi Benjamin!

Thanks for the patch! Since it's rather a trivial patch and our USB 
maintainer Gerd is pretty much busy with other stuff right now, I went ahead 
and put it in my current pull request.

Two things to notice: First, something along the way (likely your mail 
program) added a line break after the "O_WRONLY |" in the above two lines, 
so I had to undo that change manually before I was able to apply the patch 
... please try to use "git send-email" for sending patches, then such things 
don't happen.
And second, please use the scripts/checkpatch.pl script to check your 
patches - it was complaining about a line getting too long here, so I went 
ahead and fixed that, too (i.e. no need to resend, just a FYI for future 
patches).

 Thanks,
  Thomas

>           if (fd < 0) {
>               error_setg(errp, "open %s failed", dev->pcap_filename);
>               usb_qdev_unrealize(qdev);
>               return;
>           }
> -        dev->pcap = fdopen(fd, "w");
> +        dev->pcap = fdopen(fd, "wb");
>           usb_pcap_init(dev->pcap);
>       }
>   }