Re: [PATCH v2 2/8] hw/s390x/ipl: Fix incorrect PCI IPL block length constant

[Matthew Rosato <mjrosato@linux.ibm.com>]

>>>>
>>> Given that, should the #define be sizeof(IplBlockPci) + 24?
>>>
>> If IplBlockPci is likely to expand in the future, it might make sense to
>> derive this value from the structure layout. For example,
>> offsetof(IplParameterBlock, pci) + sizeof(IplBlockPci) could be more
>> accurate. Otherwise, keeping it as a constant like the other definitions
>> seems reasonable as well.
> 
> All of those suggestions would be valid, but I'm leaning toward keeping it
> defined without using sizeof() for the sake of consistency if nothing
> else.  The other IPLB types are already defined as fixed numbers and for
> the IplBlockQemuScsi being used later in this patch series, it actually has
> a minimum value that is less than the sizeof() itself due to the same
> struct
> servicing both PCI and CCW controllers, where the minimum size for CCW is
> less than PCI.
> 
> I can envision several ways to improve the definitions and/or naming
> conventions, but I think it is outside the scope of this series because the
> changes should be uniformly applied to all definitions, not just PCI.

Ehh...  If you already think the other definitions should be fixed then
convention is not a good enough reason to propagate the bad practice to
new code.  Doing the new definition the right way now in this patch is
certainly within the scope of this series.

If we have structures that define the entire 336 bytes, I personally
would much prefer to see that written out with sizeof()s vs a magic
number that coincidentally must line up with the size of one or more
well-defined structures.

As for existing definitions, I agree that is out of scope: you could
follow-up later with one or more patches that cleanup the existing
definitions.

Thanks,
Matt