Re: [PATCH 5/5] winebus.sys: Use irp IoStatus.Status consistently.

[Zebediah Figura (she/her)]

On 8/12/21 3:00 PM, Rémi Bernon wrote:
> On 8/12/21 7:10 PM, Zebediah Figura (she/her) wrote:
> > On 8/12/21 2:47 AM, Rémi Bernon wrote:
> > > case IOCTL_HID_READ_REPORT:
> > >           {
> > >               TRACE_(hid_report)("IOCTL_HID_READ_REPORT\n");
> > > -            status = ext->vtbl->begin_report_processing(device);
> > > -            if (status != STATUS_SUCCESS)
> > > -            {
> > > -                irp->IoStatus.Status = status;
> > > -                break;
> > > -            }
> > > +            irp->IoStatus.Status = 
> > > ext->vtbl->begin_report_processing(device);
> > > +            if (irp->IoStatus.Status != STATUS_SUCCESS) break;
> > >               if (!ext->last_report_read)
> > >               {
> > > -                irp->IoStatus.Status = status = 
> > > deliver_last_report(ext,
> > > +                irp->IoStatus.Status = deliver_last_report(ext,
> > >                       buffer_len, irp->UserBuffer, 
> > > &irp->IoStatus.Information);
> > >                   ext->last_report_read = TRUE;
> > >               }
> > >               else
> > >               {
> > >                   InsertTailList(&ext->irp_queue, 
> > > &irp->Tail.Overlay.ListEntry);
> > > -                status = STATUS_PENDING;
> > > +                irp->IoStatus.Status = STATUS_PENDING;
> > >               }
> > >               break;
> > >           }
> >
> > If you return STATUS_PENDING and queue the IRP here...
> > > @@ -990,9 +982,8 @@ static NTSTATUS WINAPI 
> > > hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp)
> > >       LeaveCriticalSection(&ext->cs);
> >
> > ...and release the CS here, then the IRP can be completed before you 
> > access it...
> > > -    if (status != STATUS_PENDING)
> > > -        IoCompleteRequest(irp, IO_NO_INCREMENT);
> > > -
> > >
> > > +    status = irp->IoStatus.Status;
> >
> > ...here.
> >
> > This is kind of the reason that I've gone for the pattern
> >
> > switch (ioctl)
> >      {
> >          case X:
> >              status = STATUS_SUCCESS;
> >              break;
> >
> > case Y:
> >              status = STATUS_PENDING;
> >              IoMarkIrpPending(irp);
> >              queue_irp(irp);
> >              break;
> >      }
> >
> > if (status != STATUS_PENDING)
> >      {
> >          irp->IoStatus.Status = status;
> >          IoCompleteRequest(irp);
> >      }
> >      return status;
> >
> > in other ntoskrnl code; it's almost the same amount of code and less 
> > easy to get wrong.
> > > +    if (status != STATUS_PENDING) IoCompleteRequest(irp, 
> > > IO_NO_INCREMENT);
> > >       return status;
> > >   }
>
> Indeed.
>
> I like the idea of passing the IO_STATUS_BLOCK to the callback directly, 
> though, instead of having the length passed as parameters and then moved 
> again into the irp after the call and the very long lines which set the 
> status on every call.
>
> Same thing for the HID_XFER_PACKET, most of the time we get one already 
> from the caller and we instead expand all its members as parameters.

True, it's less obviously better if you have to fill the Information as 
well. On the other hand, it's only one field. My preferred pattern, when 
I need to fill Information, is to fill it in the same place as I fill 
the output buffer, but to leave Status to be filled by the request handler.

All this is just more evidence that we need a helper framework—whether 
WDF or homegrown—to deal with the ugliness of WDM...