[flashrom-gerrit] [XS] Change in flashrom[main]: fmap: Update major/minor version check

[Brian Norris (Code Review)]

Attention is currently required from: Anastasia Klimchuk.

Brian Norris has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79060?usp=email )

Change subject: fmap: Update major/minor version check
......................................................................

Patch Set 2:

(1 comment)

Commit Message:

https://review.coreboot.org/c/flashrom/+/79060/comment/9a18351f_755b13f2 :
PS2, Line 9: It's not valid to separately check the major and minor versions. The
           : proper minor check would be something like:
           : 
           :   if (fmap->ver_major == FMAP_VER_MAJOR &&
           :       fmap->ver_minor > FMAP_VER_MINOR)
Sorry for some delay. I've mostly moved on from this, because the primary bug was that libfmap was using the wrong version. Now that I've fixed that up, everyone I'm aware of is using major=1 minor=1, so none of these ugly comparisons have much more impact at the moment.

I guess the real question would be what should happen if anybody ever bumps the minor version for some reason. (Would they still be backward compatible? If so, then why bump it at all?) I'm not aware of any versioning policy for this, but if
look at "semantic versioning" (https://semver.org/), I believe their definitions ("Minor version Y (x.Y.z | x > 0) MUST be incremented if new, backward compatible functionality is introduced") would suggest we should ignore minor versions for the purpose of compatibility.
> Brian, I like this idea! Maybe you can move the code from commit message into the code? :)

The rest of my commit message describes why I don't: because cbfstool only has a major check, and I preferred to not develop a 3rd form of this check.
> I looked into the chromiumos patch you linked, it still checks both major and minor versions.

Yeah, I got enough run-around here that I fixed my problems elsewhere, and punted on the question of version comparisons.

I still think it's bad to leave flashrom as it is, while libfmap was less important. (Or, I can go change the ChromiumOS libfmap much more easily after y'all upstream folks agree on how flashrom and/or cbfstool should look.)
> The diff I see is that the check is more strict (both major and minor version expected to be exact equal) while initial fmap.c checks that the versions are not greater than.
>
> I can agree to make the check more strict, but I don't see why checking minor version has to be dropped, I think it can stay?
>
> I admit I haven't looked into cbfstool code, is there a way you can link a relevant piece? (if you think it's useful. if not then not)

Sure, here you go!
https://review.coreboot.org/plugins/gitiles/coreboot/+/354a54ac84a934c1ee606...

```
    /* strings containing the magic tend to fail here */
    if (fmap->ver_major != FMAP_VER_MAJOR)
    	return 0;
```

Summary reasons for dropping the minor:

1. I was matching cbfstool
2. I don't want a 3rd version of this comparison (perhaps I can change cbfstool too if you'd like)
3. it's unclear what a version bump of any kind would mean w.r.t. compatibility; but minor version changes are considered backwards compatible by some definitions

But I don't have strong opinions. At this point, I might just as well drop the CL, as it no longer has much significance despite the existing code being logically strange.