Re: [PATCH v2 03/17] tools/migration: Drop IHDR_VERSION constant from libxc and python

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 05/03/2020 17:11, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [PATCH v2 03/17] tools/migration: Drop 
> IHDR_VERSION constant from libxc and python"):
>> On 24/02/2020 17:25, Ian Jackson wrote:
>>> Andrew Cooper writes ("[PATCH v2 03/17] tools/migration: Drop IHDR_VERSION 
>>> constant from libxc and python"):
>>>> Migration v3 is in the process of being introduced, meaning that the code 
>>>> has
>>>> to cope with both versions.  Use an explicit 2 for now.
>>>>
>>>> For the verify-stream-v2 and convert-legacy-stream scripts, update
>>>> text to say "v2 (or later)".  What matters is the distinction vs
>>>> legacy streams.
>>> Can I request that you use a manifest constant for `2', rather than
>>> sprinkling literal `2's everywhere ?  Something like IHDR_VERSION_2 ?
>>> This may seem pointless, but it will mean that it is possible to grep
>>> the code much more easily for things which are relevant to v2 or v3 or
>>> whatever.
>>>
>>> (I don't it's necessary to go to any great lengths to substitute the
>>> value of IHDR_VERSION_2 into error messages; a literal 2 in the string
>>> is OK I think.)
>> As I reply previously... The value comes out of a pipe, and is checked
>> exactly once.
> I think we are talking at cross purposes.
>
> I am suggesting that you replace every instance of a numeric literal
> `2' in the code with IHDR_VERSION_2 (which would be a #define or enum
> for 2).
>
> I count 4 such literals.

I presume you mean here 2x send IHDR and 2x receive IHDR, one C and one
Python in each case.

I understand what you're suggesting.  I completely disagree with it, as
it obfuscates the logic and introduces a source of bugs for zero gain.

IHDR_VERSION_* isn't grepable.  You've got to find the only instance of
it in code to figure out what to search for.

>> You can already grep for everything, using format_version which is where
>> this number is stashed for the duration of restore.
> None of the things I am talking about have "format_version" nearby.
> They tend to have variants on "version" but that is a poor thing to
> grep for.

"ctx->restore.format_version = ihdr.version;" (the only instance in the
codebase at this point in the series) is immediately after ihdr is
sanity checked, which is sole time where idhr.version has its value
checked (in the restore path.  There is also exactly one place in the
save side, and any more than this would be buggy code).

The only thing IHDR_VERSION_* usefully gets you is the ability to get
the defines accidentally wrong, and introduce bugs that way.

~Andrew