Re: [Xen-devel] [PATCH] XenPvBlk: handle empty cdrom drives

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

I just want to say that this is one of the best reviews I have ever
received, very clear and useful. Thanks Laszlo!

On Tue, 20 Oct 2015, Laszlo Ersek wrote:
> (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@xxxxxxxxxxxxx>
> > 
> > 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
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel