Fix deprecated use of hdr.Xattrs (SA1019)

[Honny1]

This PR fixes warning deprecated use of hdr.Xattrs (SA1019) found by golangci when the staticcheck linter is enabled.

Partially fixes:

• Consider allowing more linters #1579

[rhatdan]

/approve
LGTM
@giuseppe @nalind @mtrmac @saschagrunert PTAL

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Honny1, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[mtrmac]

Thanks!

This LGTM locally, but this is fairly critical low-level code. Could you check that the packages do have some tests that cover the changed code, please? (One possible approach would be, for each instance, to just delete the Xattr handling code, and ensure that at least one c/storage test fails. Ideally the tests that fail should go all the way between a local file and a tar stream, not just a local unit test on ReadSecurityXattrToTarHeader or so.)

Also Cc: @nalind for expert insights.

[rhatdan]

@mtrmac PTANL

[mtrmac]

I’m fine with the code, but I’d like to hear an assurance that this is covered by tests. (Yes, I could go verify that myself…)

[Honny1]

I checked the unit tests by commenting out the parts of the code that were changed if the tests fail. The tests did not fail for the readUserXattrToTarHeader, (o overlayWhiteoutConverter) ConvertWrite, writeZstdChunkedStream functions. I haven't tried code changes that are specific to the darwin os. Should I also try to run unit tests of padman with the imported local storage?

[mtrmac]

I’d guess that if c/storage does not test these somewhat obscure aspects of layer extraction itself, Podman is unlikely to; but triggering a Podman CI run with a modified c/storage is probably much cheaper than manual analysis, so, sure, go for it.

[giuseppe]

please use xattrKey if you really need to change it

[Honny1]

@mtrmac I ran podman CI with imported c/storage with changes in this PR. CI passes see the PR that runs the tests.

[Honny1]

I checked the unit tests by commenting out the parts of the code that were changed if the tests fail. The tests did not fail for the readUserXattrToTarHeader, (o overlayWhiteoutConverter) ConvertWrite, writeZstdChunkedStream functions. I haven't tried code changes that are specific to the darwin os. Should I also try to run unit tests of padman with the imported local storage?

I added an xattr to TestTarUntarWithXattr which is a user xattr. So the test fails for the commented change in readUserXattrToTarHeader.

[mtrmac]

If I understand it correctly, with this PR we have xattr test coverage for archive.go. Great!

---

I can’t see any c/storage test at all exercising compressor.ZstdCompressor; I can’t see any for the whiteout converter either, but there it’s quite possible I have missed one.

*shrug* if that’s the case, I think it can be reasonably argued it’s not the responsibility of this PR to add missing tests to entire large features.

[mtrmac]

/lgtm

Thanks!

[mtrmac]

/lgtm

[mtrmac]

@Honny1 the tests are failing

[Honny1]

@mtrmac The tests are passing.

[mtrmac]

/lgtm