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

[Laszlo Ersek <lersek@redhat.com>]

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.

> 
>> +             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