intel / ipmctl

BSD 3-Clause "New" or "Revised" License
184 stars 62 forks source link

Drop unused support for block aperture, fixing build with kernel 5.18 #194

Closed kilobyte closed 10 months ago

kilobyte commented 2 years ago

With current userspace kernel headers, the build fails:

/<>/src/os/linux/lnx_system.c: In function ‘get_supported_block_sizes’: /<>/src/os/linux/lnx_system.c:336:52: error: ‘ND_DEVICE_NAMESPACE_BLK’ undeclared (first use in this function); did you mean ‘ND_DEVICE_NAMESPACE_IO’? 336 | (nstype == ND_DEVICE_NAMESPACE_BLK)) | ^~~~~~~ | ND_DEVICE_NAMESPACE_IO compilation terminated due to -Wfatal-errors. make[3]: *** [CMakeFiles/ipmctl_os_interface.dir/build.make:149: CMakeFiles/ipmctl_os_interface.dir/src/os/linux/lnx_system.c.o] Error 1

As block sizes we fetch have never been used for anything, let's just drop the query.

StevenPontsler commented 2 years ago

Thanks, we will take a look.

kilobyte commented 2 years ago

Here's the original report, BTW.

StevenPontsler commented 2 years ago

The name ND_DEVICE_NAMESPACE_BLK is defined in ndctl.h as is ND_DEVICE_NAMESPACE_IO.

Looking at the report linked to it appears like the file should be coming from one of these packages

Get:87 http://127.0.0.1:12990/debian sid/main amd64 libndctl6 amd64 73-2 [61.8 kB]
Get:88 http://127.0.0.1:12990/debian sid/main amd64 libndctl-dev amd64 73-2 [12.3 kB]

which I believe is https://packages.debian.org/source/sid/ndctl

looking in http://deb.debian.org/debian/pool/main/n/ndctl/ndctl_73.orig.tar.gz it appears like ND_DEVICE_NAMESPACE_BLK is defined

So I think the code as exists should compile. I am missing something?

kilobyte commented 2 years ago

The define comes from #include <linux/ndctl.h> rather than ndctl/ndctl.h. The former is shipped by linux-libc-dev which already deleted it, rather than by ndctl which had no release that corresponds to 5.18 kernel yet.

But even if ndctl/ndctl.h keeps the historic define, the values queries have never been actually used by ipmctl.

nolanhergert commented 2 years ago

You are correct, we don't expose the values to any external API. Making your change as well as some other sections. It'll take a while, since we need to start the change internally. Thanks!

kilobyte commented 2 years ago

I'll use my patch as-is for Debian packaging then (as maintainers get shouted a lot for "does not build" bugs), you can take your time.

vynu commented 11 months ago

Hello, is this issue going to be fixed any time soon ??

mikolajkolakowski commented 11 months ago

@vynu , we are not planning to spin new version of ipmctl at this time. What is the rationale for this change to be released?

kilobyte commented 11 months ago

Every downstream project that builds ipmctl from source needs to apply the patch.

For some projects, applying patches is a routine matter, for others it requires tedious manual hacks. But in every case, it's unnecessary work that's multiplied for every downstream.

nolanhergert commented 10 months ago

Done, thanks @kilobyte! Let me know if there is another step I should do.