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.
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".
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?
> 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
