Re: [PATCH 11/12] OvmfPkg/SmmControl2Dxe: save fw_cfg boot script with QemuFwCfgS3Lib

[Laszlo Ersek <lersek@redhat.com>]

On 03/13/17 22:32, Jordan Justen wrote:
> On 2017-03-10 11:36:36, Laszlo Ersek wrote:
>> On 03/10/17 11:14, Jordan Justen wrote:
>>> On 2017-02-22 17:48:13, Laszlo Ersek wrote:
>>>> +  //
>>>> +  ScratchBuffer->Features = mSmiFeatures;
>>>> +  Status = QemuFwCfgS3WriteBytes (mRequestedFeaturesItem,
>>>
>>> I noticed a fair amount of function calls where the first args are
>>> appearing on the first line, and then the rest appear on subsequent
>>> lines indented two spaces.
>>
>> Yes, that's intentional. The coding style nominally requires each
>> argument on a separate line (if they don't all fit on a single
>> line), but that wastes a huge amount of vertical space (which is bad
>> because less code fits in a screenful). So I frequently follow a
>> semi-compressed style where I place the arguments continuously, but
>> wherever I have to break the line (because of the 80 char limit) I
>> stick with the indentation required by the coding style. Also, when
>> I apply this style, I don't break the final closing paren off to a
>> separate line (because this style is primarily contiguous).
>>
>> I mostly employ this style when the arguments are otherwise simple
>> (i.e., when the "one arg per line" style does not improve clarity,
>> just wastes a lot of space).
>>
>> This is not new, I've been coding like this in edk2 virtually
>> forever.
>>
> 
> I understand that I've given an r-b in the past for code like this. I
> have noticed it previously, but in some cases it doesn't look as
> strange. For example, with the DEBUG macro, the error level seems less
> important to move to the next line. I guess it is probably bad to make
> an exception there.
> 
> I do think that lining up the arguments should be prioritized over
> saving vertical whitespace. I think we just have to accept that EDK II
> is not efficient in terms of vertical space. In fact, EDK II is just
> plain verbose in general. :)
> 
> At least we don't follow the kernel's strict 8 char indent and 80
> columns limit. (Although, there are some good arguments in favor of
> this in terms of forcing code to be broken down into smaller
> functions.)
> 
> I would like to know if Mike has any opinion on whether this is a bug
> in the coding style spec to not address this more definitively.

"CCS_2_1_Draft.pdf" seems to be the most recent version; in it, I find:

(1)

5.2.2.4 Subsequent lines of multi-line function calls should line up
        one or two tab-stops from the beginning of the function name

        Use either one or two tab stops to ensure that each parameter
        is indented at least two spaces after the function name. Either
        of the below examples is acceptable:

        Status = gBS->AllocatePool (
                        EfiBootServicesData,
                        sizeof (DRIVER_NAME_INSTANCE),
                        &PrivateData
                      );

       Status = gBS->AllocatePool (
                       EfiBootServicesData,
                       sizeof (DRIVER_NAME_INSTANCE),
                       &PrivateData
                     );

My notes on this:

- my code complies with the indentation requirement

- the passage is silent on whether each argument should be broken off
  to a new line. The examples are called "acceptable", not "required".
  I guess this is what your question to Mike is about.

- I don't see what the difference is between the two examples. To me
  they look identical.

- In the examples, the closing parens line up with "AllocatePool", not
  with "EfiBootServicesData". I think I have never ever seen this style
  in edk2.

(2)

6.5.2.4 Comments are allowed on the parameters of a function call.

        These comments provide supplemental information about the
        parameters for that specific function call. The information in
        parameter comments should not repeat the information in the
        descriptive text for the @param entries in the special
        documentation block describing the function. Function call
        parameter comments are never Doxygen comments.

        Status = TestString (
                   String, // Comment for first parameter
                   Index + 3, // Comment for second parameter
                   &Value // Comment for third parameter
                   );

My notes on this:

- the closing paren's indentation is inconsistent with the examples
  given under 5.2.2.4 (but consistent with edk2 practice).


If the agreement is to break every single argument in a multi-line
function call (or function-like macro call) off to a separate line, then
please let us file a BZ for that (EDK2/Documentation I guess?), and then
I'll comply with that *new* requirement in my patches that I post
*after* the BZ is filed.

I see a number of open items for FDF/INF/etc specs:

https://bugzilla.tianocore.org/buglist.cgi?component=Documentation&product=EDK2&query_format=advanced

so I think the TianoCore bugzilla is the right place to track coding
style spec reports as well.

Thanks
Laszlo

> 
> -Jordan
> 
>>>
>>>> +             sizeof ScratchBuffer->Features);
>>>> +  if (RETURN_ERROR (Status)) {
>>>> +    goto FatalError;
>>>> +  }
>>>> +
>>>
>>> I can't say that the coding style prohibits this, but it does seem odd
>>> looking. I would expect to see:
>>>
>>> [1]
>>>
>>>   Status = QemuFwCfgS3WriteBytes (
>>>              mRequestedFeaturesItem,
>>>              sizeof ScratchBuffer->Features
>>>              );
>>>
>>> The coding style examples look like this:
>>>
>>>   Status = QemuFwCfgS3WriteBytes (
>>>              mRequestedFeaturesItem,
>>>              sizeof ScratchBuffer->Features
>>>            );
>>>
>>> But this looks less like what I'm used to actually seeing in EDK II.
>>> (Look at DXE Core, for example. It looks like [1].)
>>>
>>> I personally think this is okay to save a line, but it doesn't seem to
>>> follow the coding style doc:
>>>
>>>   Status = QemuFwCfgS3WriteBytes (
>>>              mRequestedFeaturesItem,
>>>              sizeof ScratchBuffer->Features);
>>>
>>> Now, we all know EDK II has a style of it's own, but outide EDK II, I
>>> might expect something like:
>>>
>>>   Status = QemuFwCfgS3WriteBytes(mRequestedFeaturesItem,
>>>                                  sizeof ScratchBuffer->Features);
>>>
>>> Which once again shows arguements on subsequent lines are lined up.
>>> Based on that, I think [1] is the best style for EDK II code.
>>
>> Yes, [1] is the official style, and I stick with that when the arguments are complex or long, or require separate comments.
>>
>> However, as I said above, when the arguments are simple, it makes sense to place them contiguously (together with the final paren), while preserving the edk2 indentation.
>>
>> Again, I've been doing this for years, consistently, and you've never seemed to take issue with it. For example, some snippets from the virtio block driver:
>>
>> fd51d75991731 (jljusten        2012-10-08 07:32:59 +0000  290)   //
>> fd51d75991731 (jljusten        2012-10-08 07:32:59 +0000  291)   // virtio-blk header in first desc
>> fd51d75991731 (jljusten        2012-10-08 07:32:59 +0000  292)   //
>> 7fcacd6c92616 (jljusten        2012-10-12 18:54:17 +0000  293)   VirtioAppendDesc (&Dev->Ring, (UINTN) &Request, sizeof Request,
>> e371e7e545b26 (jljusten        2012-10-12 18:54:35 +0000  294)     VRING_DESC_F_NEXT, &Indices);
>>
>>
>> fd51d75991731 (jljusten        2012-10-08 07:32:59 +0000  310)     //
>> fd51d75991731 (jljusten        2012-10-08 07:32:59 +0000  311)     // VRING_DESC_F_WRITE is interpreted from the host's point of view.
>> fd51d75991731 (jljusten        2012-10-08 07:32:59 +0000  312)     //
>> 7fcacd6c92616 (jljusten        2012-10-12 18:54:17 +0000  313)     VirtioAppendDesc (&Dev->Ring, (UINTN) Buffer, (UINT32) BufferSize,
>> fd51d75991731 (jljusten        2012-10-08 07:32:59 +0000  314)       VRING_DESC_F_NEXT | (RequestIsWrite ? 0 : VRING_DESC_F_WRITE),
>> e371e7e545b26 (jljusten        2012-10-12 18:54:35 +0000  315)       &Indices);
>> fd51d75991731 (jljusten        2012-10-08 07:32:59 +0000  316)   }
>>
>>
>> fd51d75991731 (jljusten        2012-10-08 07:32:59 +0000  318)   //
>> fd51d75991731 (jljusten        2012-10-08 07:32:59 +0000  319)   // host status in last (second or third) desc
>> fd51d75991731 (jljusten        2012-10-08 07:32:59 +0000  320)   //
>> 7fcacd6c92616 (jljusten        2012-10-12 18:54:17 +0000  321)   VirtioAppendDesc (&Dev->Ring, (UINTN) &HostStatus, sizeof HostStatus,
>> e371e7e545b26 (jljusten        2012-10-12 18:54:35 +0000  322)     VRING_DESC_F_WRITE, &Indices);
>>
>>
>> 6476804e3cd2e (Laszlo Ersek    2013-12-18 19:57:46 +0000  674)     Status = VIRTIO_CFG_READ (Dev, Topology.PhysicalBlockExp,
>> 6476804e3cd2e (Laszlo Ersek    2013-12-18 19:57:46 +0000  675)                &PhysicalBlockExp);
>>
>>
>> More recent code from e.g. "SmmAccessPei.c":
>>
>>
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 186)   return SmramAccessGetCapabilities (This->LockState, This->OpenState,
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 187)            SmramMapSize, SmramMap);
>>
>>
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 267)     DEBUG ((EFI_D_ERROR, "%a: no SMRAM with host bridge DID=0x%04x; only "
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 268)       "DID=0x%04x (Q35) is supported\n", __FUNCTION__, HostBridgeDevId,
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 269)       INTEL_Q35_MCH_DEVICE_ID));
>>
>>
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 309)   //
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 310)   // Given the zero graphics memory sizes configured above, set the
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 311)   // graphics-related stolen memory bases to the same as TOLUD.
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 312)   //
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 313)   PciWrite32 (DRAMC_REGISTER_Q35 (MCH_GBSM),
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 314)     TopOfLowRamMb << MCH_GBSM_MB_SHIFT);
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 315)   PciWrite32 (DRAMC_REGISTER_Q35 (MCH_BGSM),
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 316)     TopOfLowRamMb << MCH_BGSM_MB_SHIFT);
>>
>>
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 348)   Status = SmramAccessGetCapabilities (mAccess.LockState, mAccess.OpenState,
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 349)              &SmramMapSize, SmramMap);
>>
>>
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 376)   CopyMem (GuidHob, &SmramMap[DescIdxSmmS3ResumeState],
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 377)     sizeof SmramMap[DescIdxSmmS3ResumeState]);
>>
>>
>> Brand new code from "OvmfPkg/AcpiPlatformDxe/BootScript.c":
>>
>>
>> df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 107)   Context->WritePointers = AllocatePool (WritePointerCount *
>> df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 108)                              sizeof *Context->WritePointers);
>>
>>
>> df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 191)   DEBUG ((DEBUG_VERBOSE, "%a: 0x%04x/[0x%08x+%d] := 0x%Lx (%Lu)\n",
>> df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 192)     __FUNCTION__, PointerItem, PointerOffset, PointerSize, PointerValue,
>> df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 193)     (UINT64)S3Context->Used));
>>
>>
>> df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 247)   Status = gBS->LocateProtocol (&gEfiS3SaveStateProtocolGuid,
>> df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 248)                   NULL /* Registration */, (VOID **)&S3SaveState);
>>
>> I've been entirely consistent about this.
>>
>> Thanks
>> Laszlo
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>