On Sat, Dec 14, 2013 at 5:23 AM, Laszlo Ersek <[email protected]> wrote:
> I think this patch improves QemuVideoDxe, but I have a few remarks:
...
> This seems to be in the right place.
>
> (1) But, I think the third parameter should be a pointer to a new
> variable that is not used for anything else, such as:
>
> EFI_PCI_IO_PROTOCOL *ChildPciIo;
>
> /* ... */
>
> Status = gBS->OpenProtocol (
> Controller,
> &gEfiPciIoProtocolGuid,
> (VOID **) &ChildPciIo, // here
> This->DriverBindingHandle,
> Private->Handle,
> EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER
> );
>
> Otherwise I think this call overwrites Private->Handle with the address
> of Controller's PciIo protocol instance (ie. the same value as held in
> Private->PciIo).
Yes this was clearly a mistake. At one point I had attempted to pass
NULL here to avoid carrying unused/redundant data around but the core
wouldn't allow that for this particular attribute. I must have
absent-mindedly pasted back the wrong line. Thanks for catching it.
>
>> + This->DriverBindingHandle,
>> + Private->Handle,
>> + EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER
>> + );
>> +
>> + if (EFI_ERROR (Status)) {
>> + goto Error;
>> + }
>> }
>
> This goto is not necessary, because the Error label is right below. But
> it doesn't hurt and might even prove helpful if further code is added.
>
Agree this is just resilient coding practice and it can be optimized
out by the compiler.
...
>
> Seems fine. We can reach this point without having opened Controller's
> PciIo with BY_CHILD_CONTROLLER, but the protocol database should catch
> that and we can simply ignore the EFI_NOT_FOUND here.
Yes this relies on the core taking no action on the spurious call,
rather than tracking more state to be precise about error handling.
...
> Can you please post a v4 with (1) fixed?
Posted, thanks for the review!
Chris
------------------------------------------------------------------------------
Rapidly troubleshoot problems before they affect your business. Most IT
organizations don't have a clear picture of how application performance
affects their revenue. With AppDynamics, you get 100% visibility into your
Java,.NET, & PHP application. Start your 15-day FREE TRIAL of AppDynamics Pro!
http://pubads.g.doubleclick.net/gampad/clk?id=84349831&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
