Re: [PATCH v2 07/10] usb/msd: Add some additional assertions

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 11/4/25 10:04, Nicholas Piggin wrote:
> Add more assertions to help verify internal logic.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   hw/usb/dev-storage.c | 23 +++++++++++++++++++----
>   1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
> index a9d8d4e8618..3b806872587 100644
> --- a/hw/usb/dev-storage.c
> +++ b/hw/usb/dev-storage.c
> @@ -264,13 +264,24 @@ void usb_msd_transfer_data(SCSIRequest *req, uint32_t len)
>       MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent);
>       USBPacket *p = s->data_packet;
>   
> -    if ((s->mode == USB_MSDM_DATAOUT) != (req->cmd.mode == SCSI_XFER_TO_DEV)) {
> -        usb_msd_fatal_error(s);
> -        return;
> +    if (s->mode == USB_MSDM_DATAIN) {

Or switch().

> +        if (req->cmd.mode == SCSI_XFER_TO_DEV) {
> +            usb_msd_fatal_error(s);
> +            return;
> +        }
> +    } else if (s->mode == USB_MSDM_DATAOUT) {
> +        if (req->cmd.mode != SCSI_XFER_TO_DEV) {
> +            usb_msd_fatal_error(s);
> +            return;
> +        }
> +    } else {
> +        g_assert_not_reached();
>       }
>   
> +    assert(s->scsi_len == 0);
>       s->scsi_len = len;
>       s->scsi_off = 0;
> +
>       if (p) {
>           usb_msd_copy_data(s, p);
>           p = s->data_packet;
> @@ -288,6 +299,10 @@ void usb_msd_command_complete(SCSIRequest *req, size_t resid)
>   
>       trace_usb_msd_cmd_complete(req->status, req->tag);
>   
> +    g_assert(s->req);
> +    /* The CBW is what starts the SCSI request */
> +    g_assert(s->mode != USB_MSDM_CBW);
> +
>       s->csw.sig = cpu_to_le32(0x53425355);
>       s->csw.tag = cpu_to_le32(req->tag);
>       s->csw.residue = cpu_to_le32(s->data_len);
> @@ -486,7 +501,7 @@ static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
>           trace_usb_msd_cmd_submit(cbw.lun, tag, cbw.flags,
>                                    cbw.cmd_len, s->data_len);
>           assert(le32_to_cpu(s->csw.residue) == 0);
> -        s->scsi_len = 0;
> +        assert(s->scsi_len == 0);

Preferably having scsi_len changes in a distinct patch,
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

>           s->req = scsi_req_new(scsi_dev, tag, cbw.lun,
>                                 cbw.cmd, cbw.cmd_len, NULL);
>           if (s->commandlog) {