Re: [PATCH v3 2/4] OvmfPkg/VirtioScsiDxe: add helper to create a fake host adapter error

[Laszlo Ersek <lersek@redhat.com>]

On 08/31/17 17:01, Brijesh Singh wrote:
> When virtio request fails we return EFI_DEVICE_ERROR, as per the spec
> EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru() member function is required
> to implement elaborated error reporting.
> 
> The patch refactors out entire block of the code that creates the host
> adapter error into a separate helper function (ReportHostAdapterError).
> 
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 36 ++++++++++++++++----
>  1 file changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> index 5e72b1a24b59..5e977c636a0a 100644
> --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> @@ -97,7 +97,7 @@
>  // set some fields in the EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET in/out
>  // parameter on return. The following is a full list of those fields, for
>  // easier validation of PopulateRequest(), ParseResponse(), and
> -// VirtioScsiPassThru() below.
> +// ReportHostAdapterError() below.
>  //
>  // - InTransferLength
>  // - OutTransferLength
> @@ -387,6 +387,33 @@ ParseResponse (
>    return EFI_DEVICE_ERROR;
>  }
>  
> +/**
> + * The function can be used to create a fake host adapter error.
> + *
> + * When VirtioScsiPassThru is failed due to some reasons then this function
> + * can be called to contstruct a host adapter error.
> + *
> + * @param[out] Packet         The Extended SCSI Pass Thru Protocol packet that
> + *                            the host adapter error shall be placed in.
> + *
> + * @retval EFI_DEVICE_ERROR  The function returns this status code
> + *                           unconditionally, to be propagated by
> + *                           VirtioScsiPassThru().
> + */

(1) I hoped that you'd just copy & paste the comment block from my v2
review -- in v3, the bar of "*" characters to the left has not been
removed (it should have been, it is not edk2 coding style), plus the
"contstruct" typo has not been fixed, etc.

I'll update the comment block before pushing.

Thanks,
Laszlo


> +STATIC
> +EFI_STATUS
> +ReportHostAdapterError (
> +  OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET  *Packet
> +  )
> +{
> +  Packet->InTransferLength  = 0;
> +  Packet->OutTransferLength = 0;
> +  Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
> +  Packet->TargetStatus      = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
> +  Packet->SenseDataLength   = 0;
> +  return EFI_DEVICE_ERROR;
> +}
> +
>  
>  //
>  // The next seven functions implement EFI_EXT_SCSI_PASS_THRU_PROTOCOL
> @@ -472,12 +499,7 @@ VirtioScsiPassThru (
>    //
>    if (VirtioFlush (Dev->VirtIo, VIRTIO_SCSI_REQUEST_QUEUE, &Dev->Ring,
>          &Indices, NULL) != EFI_SUCCESS) {
> -    Packet->InTransferLength  = 0;
> -    Packet->OutTransferLength = 0;
> -    Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
> -    Packet->TargetStatus      = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
> -    Packet->SenseDataLength   = 0;
> -    return EFI_DEVICE_ERROR;
> +    return ReportHostAdapterError (Packet);
>    }
>  
>    return ParseResponse (Packet, &Response);
>