Re: [edk2] [PATCH] XenPvBlk: handle empty cdrom drives

[Laszlo Ersek <lersek@redhat.com>]

(1) Please make the subject line say:

  OvmfPkg: XenPvBlkDxe: handle empty cdrom drives

On 10/20/15 17:03, Stefano Stabellini wrote:
> Empty cdroms are not going to connect, avoid waiting for the backend to
> switch to state 4, which is never going to happen, and return
> EFI_NO_MEDIA instead.

(2) I suggest to name the function here that should return EFI_NO_MEDIA
-- XenPvBlockFrontInitialization().

(3) Because the return status will ultimately be forwarded outside by
the driver binding's Start() function, if possible we should stick with
status codes listed by the UEFI spec. The relevant section is

  10.1 EFI Driver Binding Protocol
  EFI_DRIVER_BINDING_PROTOCOL.Start()

So, as long as we are setting the error code manually in
XenPvBlockFrontInitialization() -- ie. not propagating an error code
from another function we call there --, I think we should stick with
EFI_DEVICE_ERROR.

Sorry that I didn't point this out in my earlier message on qemu-devel.

> Detect an empty cdrom by looking at the "params"
> node on xenstore, which is set to  "" or "aio:" for empty drives by libxl.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> diff --git a/OvmfPkg/XenPvBlkDxe/BlockFront.c b/OvmfPkg/XenPvBlkDxe/BlockFront.c
> index 256ac55..5a52a03 100644
> --- a/OvmfPkg/XenPvBlkDxe/BlockFront.c
> +++ b/OvmfPkg/XenPvBlkDxe/BlockFront.c
> @@ -169,6 +169,8 @@ XenPvBlockFrontInitialization (
>    XEN_BLOCK_FRONT_DEVICE *Dev;
>    XenbusState State;
>    UINT64 Value;
> +  EFI_STATUS Ret = EFI_DEVICE_ERROR;

So this shouldn't be necessary.

(As a side point I'll mention that initialization of variables with
automatic storage duration is forbidden by the edk2 coding style;
separate assignments are required. I'm aware that this file already
doesn't comply with that, but that's no excuse. :))

> +  CHAR8 *Params;
>  
>    ASSERT (NodeName != NULL);
>  
> @@ -186,6 +188,17 @@ XenPvBlockFrontInitialization (
>    }
>    FreePool (DeviceType);
>  
> +  if (Dev->MediaInfo.CdRom) {
> +    XenBusIo->XsBackendRead (XenBusIo, XST_NIL, "params", (VOID**)&Params);
> +    if (AsciiStrLen (Params) == 0 || AsciiStrCmp (Params, "aio:") == 0) {

(4) Please verify that XsBackendRead() returns XENSTORE_STATUS_SUCCESS.
I think you can use the "Status" variable for that.

If the read fails, you should assume one of "cdrom empty" vs. "cdrom not
empty" -- your call.

... I briefly considered that maybe XsBackendRead() is guaranteed to set
*Params to \0 on failure, but the typedef docs of the protocol member
function in question (XENBUS_XS_BACKEND_READ in
"OvmfPkg/Include/Protocol/XenBus.h") don't seem to guarantee that, so
please check the return status.

> +      FreePool (Params);
> +      DEBUG ((EFI_D_INFO, "XenPvBlk: Empty cdrom\n"));

(5) If this is a normal / expected condition, then EFI_D_INFO is fine.
If it is a genuine error, then EFI_D_ERROR should be used. I think
EFI_D_INFO is right though.

(6) Suggest to replace "XenPvBlk" with the "%a" conversion
specification, and pass in __FUNCTION__ as its argument. ("%a" formats
CHAR8 strings, while "%s" formats CHAR16 strings.)

__FUNCTION__ will expand to "XenPvBlockFrontInitialization", which is
both telling, and allows people with ctags- or cscope-compatible editors
to jump to the function immediately.

> +      Ret = EFI_NO_MEDIA;
> +      goto Error;

See (3) -- I think the goto suffices.

> +    }
> +    FreePool (Params);
> +  }
> +
>    Status = XenBusReadUint64 (XenBusIo, "backend-id", FALSE, &Value);
>    if (Status != XENSTORE_STATUS_SUCCESS || Value > MAX_UINT16) {
>      DEBUG ((EFI_D_ERROR, "XenPvBlk: Failed to get backend-id (%d)\n",
> @@ -318,7 +331,7 @@ AbortTransaction:
>    XenBusIo->XsTransactionEnd (XenBusIo, &Transaction, TRUE);
>  Error:
>    XenPvBlockFree (Dev);
> -  return EFI_DEVICE_ERROR;
> +  return Ret;
>  }
>  
>  VOID
> 

See (3) again.

I'm looking forward to version 2.

Thanks!
Laszlo