On Fri, Sep 28, 2012 at 3:52 AM, Laszlo Ersek <ler...@redhat.com> wrote:
> On 09/28/12 08:46, Jordan Justen wrote:
>
>> Would it be easy to move 'virtio-0.9.5' definitions and structures to
>> a separate Virtio.h file?
>>
>> The idea would be to have spec type definitions and structures in a
>> separate place from the driver implementation definitions and
>> structure.
>
> I think it's impossible to get the interface extraction right when
> there's only one client, so I wanted to postpone that until the second
> client was going to come along (virtio-scsi, presumably). Anyway I can
> try it now.
>
> I think the Virtio.h file should go into OvmfPkg/Include/Library/,
> whereas any function bodies I might extract should go under a new
> directory called OvmfPkg/Library/Virtio.
Really OvmfPkg/Include/IndustryStandard would more follow with what
I'm meaning. But, for now, I think just keeping Virtio.h in the driver
directory would be fine.
For Virtio.h, it would be very generic definitions and structures,
coming from the spec. (Refer to MdePkg/Include/IndustryStandard)
> I might need help with the INF file for the library (and other affected
> meta-files, fdf's and dsc's, maybe).
>
> On 09/28/12 09:05, Jordan Justen wrote:
>
>>> +#define CONFIG_WRITE(Field, Value, Failed) \
>>> + do { \
>>> + Status = IoWrite ( \
>>> + Dev->PciIo, \
>>
>> I don't think we should rely on a local Dev variable in the macro.
>
> OK.
>
>>
>>> + OFFSET_OF_VHDR (Field), \
>>> + SIZE_OF_VHDR (Field), \
>>> + (Value) \
>>> + ); \
>>> + if (EFI_ERROR (Status)) { \
>>> + goto Failed; \
>>> + } \
>>> + } while (FALSE)
>>
>> Can you move this loop code inside a function?
>
> This is not a "real" loop; do {...} while(FALSE) is the usual way to
> place multiple statements into a block inside a macro and still allow
> the macro user to put a semicolon after the macro invocation, right
> before an "else".
Oh, yeah. Well, similar to the "Dev" situation, I don't think it is a
good practice for a macro to refer to a local label.
> Plus the "goto" cannot go into another function.
>
>>
>> I don't think the goto in a macro is a good idea.
>
> The goto is the essence of this macro. More precisely, the macro has two
> uses:
> - make sure everything dependent on the field (size, offset etc) is
> derived from a single naming of the field,
> - make sure error checking is never forgotten, while keeping the call
> site reasonably compressed.
>
> Normally I like explicit error checking, but:
> - Due to the many PCI accesses, VirtioBlkInit() is quite linear, already
> pretty long, and not really splittable. I introduced the above macros
> partially in order to save repeating the error checking if/goto over and
> over. (My personal style would forbid goto altogether and prescribe me
> to use nested "if"s, but you would definitely refuse a sixfold nested
> function.)
> - It's impossible to misuse this macro, because the "goto" requires a
> label, and labels have a separate namespace in C (and you can only jump
> within a function).
>
> I can of course open-code the if/goto each time -- do you really want me to?
Yes, I think this would be clearer code than using a goto in the macro.
-Jordan
>> I think the main advantage of the macro will be in shortening the
>> field offset/size.
>
> Yes, that's one advantage of the two.
>
>> What about VIRTIO_CFG_WRITE for a less generic name?
>
> Sure.
>
> (I'll come to the rest later.)
>
> Thanks!
> Laszlo
------------------------------------------------------------------------------
Got visibility?
Most devs has no idea what their production app looks like.
Find out how fast your code is with AppDynamics Lite.
http://ad.doubleclick.net/clk;262219671;13503038;y?
http://info.appdynamics.com/FreeJavaPerformanceDownload.html
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
