* [edk2] [PATCH] XenPvBlk: handle empty cdrom drives @ 2015-10-20 15:03 Stefano Stabellini 2015-10-20 15:30 ` Laszlo Ersek 0 siblings, 1 reply; 3+ messages in thread From: Stefano Stabellini @ 2015-10-20 15:03 UTC (permalink / raw) To: edk2-devel; +Cc: fabio.fantoni, lersek, xen-devel 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. 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; + 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) { + FreePool (Params); + DEBUG ((EFI_D_INFO, "XenPvBlk: Empty cdrom\n")); + Ret = EFI_NO_MEDIA; + goto Error; + } + 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [edk2] [PATCH] XenPvBlk: handle empty cdrom drives 2015-10-20 15:03 [edk2] [PATCH] XenPvBlk: handle empty cdrom drives Stefano Stabellini @ 2015-10-20 15:30 ` Laszlo Ersek 2015-10-21 11:30 ` Stefano Stabellini 0 siblings, 1 reply; 3+ messages in thread From: Laszlo Ersek @ 2015-10-20 15:30 UTC (permalink / raw) To: Stefano Stabellini, edk2-devel; +Cc: fabio.fantoni, xen-devel (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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [edk2] [PATCH] XenPvBlk: handle empty cdrom drives 2015-10-20 15:30 ` Laszlo Ersek @ 2015-10-21 11:30 ` Stefano Stabellini 0 siblings, 0 replies; 3+ messages in thread From: Stefano Stabellini @ 2015-10-21 11:30 UTC (permalink / raw) To: Laszlo Ersek; +Cc: edk2-devel, fabio.fantoni, xen-devel 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@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 > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-10-21 11:30 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-10-20 15:03 [edk2] [PATCH] XenPvBlk: handle empty cdrom drives Stefano Stabellini 2015-10-20 15:30 ` Laszlo Ersek 2015-10-21 11:30 ` Stefano Stabellini
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.