Re: [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib

[Laszlo Ersek <lersek@redhat.com>]

On 03/10/17 10:55, Jordan Justen wrote:
> On 2017-03-06 10:41:31, Laszlo Ersek wrote:
>> On 03/06/17 10:19, Jordan Justen wrote:
>>
>>> Is there any reason we couldn't add a
>>>    pointer within the scatch buffer for reading/writing to? It just
>>>    seems to make the interface a bit more generic/intuitive.
>>
>> That's how I initially designed the interface, but while implementing
>> the functions, I realized there was no good use for that feature.
>>
>> The only use case for storing the data read from fw_cfg at a nonzero
>> offset in the scratch buffer, and for writing data to fw_cfg from a
>> nonzero offset in the scratch buffer, would be that you have "something
>> else" at offset zero. What could that thing be?
>>
> 
> We discussed this on irc a bit. I agree that we can proceed with your
> current design.
> 
> I still think the library interface would be clearer if it had the
> buffer passed to the read/write routines, but I concede that it would
> complicate and grow the code for no other gain than a clearer library
> interface.
> 
> I think the helper callback adds to the confusion in the library
> interface, but if you don't have the buffer passed into the read/write
> routines, then you need the scratch buffer, and then the callback makes
> sense.
> 
> I think we could have one helper to 'call me back when boot script is
> available', but didn't have the scratch allocation aspect. Of course,
> having torn out all the fw-cfg context, this now seems like something
> that some generic boot script lib might provide.
> 
> Then I'd leave it up to the user of the lib to allocate a buffer and
> pass it into the fw-cfg boot script routines to be logged. The library
> would also have to allocate the DMA transfer buffer, which is just one
> way that this would be less efficient.
> 
> Once again, I agree this would be less efficient, but I think it
> easier to follow. (Or, at least more intuitive from a library
> interface perspective.) And, once again, I am okay with you proceeding
> with your current design.

Thank you very much for taking the time to review this and for the
suggested improvements. I'd like to post v2 with the following changes:

- rename QemuFwCfgS3TransferOwnership to
  QemuFwCfgS3CallWhenBootScriptReady

- rename FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION to
  FW_CFG_BOOT_SCRIPT_CALLBACK_FUNCTION

- similarly, rename Append to BootScriptCallback

- rename QemuFwCfgS3Xxx to QemuFwCfgS3ScriptXxx

I wouldn't like to change the following aspects:

- Indentation of function calls -- I've been consistent about those for
several years, they satisfy the requirements of the coding std, and code
I wrote like that had been accepted even in MdeModulePkg
(PciHostBridgeDxe) and MdePkg (BaseOrderedCollectionRedBlackTreeLib).

My main argument against *exclusively* using the one-arg-per-line style
is that it wastes a lot of vertical space, especially because I'm
unwilling to write longer than 80char lines. (Those who are willing to
go up to 120 -- that is, 50% more! -- chars per line have it much
easier, because they need to break up a *lot* fewer function calls.)

- The way the scratch buffer is allocated and propagated. I entirely
agree with you on both characteristics of the v1 set: it is a bit more
efficient, and a bit harder to follow, than the alternative you
describe. I think arguments can be made equally well in favor of either
approach.

Unfortunately, I'm again out of time (with downstream responsibilities
rearing their ugly heads -- the rebase is done, but more churn is to
follow). I'll be happy if I can squeeze out a v2 with the above
non-intrusive changes early next week. (I wonder if the mental image of
the upcoming chaos is going to stress me enough to post v2 over the
weekend. Sigh.)

Thank you,
Laszlo