martijnvanbrummelen / nwipe

nwipe secure disk eraser
GNU General Public License v2.0
682 stars 77 forks source link

move the define off64_t #516

Closed xambroz closed 9 months ago

xambroz commented 9 months ago

Move the definition of the off64_t using int64_t bellow the include stdint.h, which is actually defining it.

Without change wrong order is breaking the build on RHEL7/8/9.

PartialVolume commented 9 months ago

Will take a look at this tomorrow, after I've run a few tests with your changes. Are those fedora workarounds still needed if stdint.h already defines it?

Just wondering why the difference between Debian based distros and RHEL

xambroz commented 9 months ago

Hello, this patch is needed for rhel7/8. The int64_t seems not to be defined at that time when being processed.

Just wondering why the difference between Debian based distros and RHEL

rhel7 is using much older gcc I guess and possibly some slightly different setup of the types.

xambroz commented 9 months ago

Are those fedora workarounds still needed if stdint.h already defines it?

On Fedora there is nothing which would define off64_t, so I guess it is still needed.

$ grep -R -e "define.*off64_t" /usr/include/ /usr/include/zconf.h:# define z_off64_t off64_t /usr/include/zconf.h:# define z_off64_t int64 /usr/include/zconf.h:# define z_off64_t z_off_t /usr/include/unistd.h:# if defined USE_LARGEFILE64 && !defined off64_t_defined /usr/include/unistd.h:# define off64_t_defined /usr/include/sys/types.h:#if defined USE_LARGEFILE64 && !defined off64_t_defined /usr/include/sys/types.h:# define off64_t_defined /usr/include/stdio.h:# if defined __USE_LARGEFILE64 && !defined off64_t_defined /usr/include/stdio.h:# define off64_t_defined /usr/include/fcntl.h:#if defined USE_LARGEFILE64 && !defined off64_t_defined /usr/include/fcntl.h:# define off64_t_defined

On RHEL8/7 the definiton of int64_t comes later in the includes, hence the move around

PartialVolume commented 9 months ago

Are those fedora workarounds still needed if stdint.h already defines it?

On Fedora there is nothing which would define off64_t, so I guess it is still needed.

On RHEL8/7 the definiton of int64_t comes later in the includes, hence the move around

I just took a look at where we use off64_t, I'm just wondering whether it can be replaced in our code. Seems to be a single use on a variable called offset

Screenshot_20231104_152901

PartialVolume commented 9 months ago

Are those fedora workarounds still needed if stdint.h already defines it?

On Fedora there is nothing which would define off64_t, so I guess it is still needed.

On RHEL8/7 the definiton of int64_t comes later in the includes, hence the move around

I just took a look at where we use off64_t, I'm just wondering whether it can be replaced in our code. Seems to be a single use on a variable called offset

Screenshot_20231104_152901

I don't think we could replace it with anything else as we need the 64 bit fixed width type as we are also building on 32 bit systems, unless we used long long which would be 8 bytes on both 32 bit and 64 bit systems.

PartialVolume commented 9 months ago

Not keen on the definition of #define off64_t off_t as wouldn't this define a 32 bit variable on a 32 bit system when we need a 64 bit variable as the variable holds the disc size in bytes.

As it happens this probably doesn't create an issue on the current version of nwipe as the section of code that this variable is used in is only active when the buffer of data we are reading/writing is less than the blocksize of the device. As we write data in blocks of either 512 bytes or the reported block size of the device the section of code where this offset variable is used will never run. (Assuming I've understood the code correctly).

However at some future date if we ever increase the size of the buffer of data to a multiple of the block size of the device then this code will become activated and would fail on a 32 bit system if the device was larger than 2GB which it probably will be.

On 64 bit systems none of this is a problem.

So for now this workaround is ok, however it would be nicer if this workaround wasn't necessary.

/* workaround for Fedora */
#ifndef off64_t
#ifndef off_t
#define off64_t int64_t
#else
#define off64_t off_t
#endif
#endif
PartialVolume commented 9 months ago

Just wondering but wouldn't either of the following be better as they wouldn't define off64_t as a 32 bit type on a 32 bit system.

Indented, just so I can understand it a bit better.

#ifndef off64_t
    #ifndef off_t
        #define off64_t int64_t
    #else
        #define off64_t int64_t
    #endif
#endif

or

#ifndef off64_t
    #ifndef off_t
        #define off64_t int64_t
    #else
        #define off64_t long long
    #endif
#endif
mdcato commented 9 months ago

OK, I"m going to open this can of worms, fire away, shields are on. Blame it on the discussion around 32/64-bit integer types.

Is it time to drop 32-bit support? My basic premise is that using nwipe is time consuming, and I/O is the biggest factor, not CPU.

A Linux Kernel 6.7 proposed feature change is to support 32- and 64-bit, but 32-bit will be OFF be default.

It's "nice" to be able to press that Pentium-4 box sitting in the corner into service, but anyone who values their time over re-using old equipment will prefer newer systems with better I/O performance without having to depend on backward compatibility of SATA-III to -II or -I. Some of the older BIOSes from the 32-bit era don't support booting from USB drives. Though I do see the benefit of older systems accommodating SAS and other controllers.

It's also "nice" to provide solutions for older equipment, thus covering a larger portion of the globe.

shredOS could drop the 32-bit build.

I think ARM-64 might be a better target to spend time on than X86-32bit. There's are already full-fledged ARM PCs (laptops) & servers (mostly cloud providers), not just ChromeBooks.

Throw your spit-balls or other virtual ammunition, but well thought out arguments are appreciated!


From: PartialVolume @.> Sent: Saturday, November 4, 2023 12:43 To: martijnvanbrummelen/nwipe @.> Cc: Subscribed @.***> Subject: Re: [martijnvanbrummelen/nwipe] move the define off64_t (PR #516)

Just wondering but wouldn't either of the following be better as they wouldn't define off64_t as a 32 bit type on a 32 bit system.

Indented, just so I can understand it a bit better.

ifndef off64_t

#ifndef off_t
    #define off64_t int64_t
#else
    #define off64_t int64_t
#endif

endif

or

ifndef off64_t

#ifndef off_t
    #define off64_t int64_t
#else
    #define off64_t long long
#endif

endif

— Reply to this email directly, view it on GitHubhttps://github.com/martijnvanbrummelen/nwipe/pull/516#issuecomment-1793505434, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ANGK2PU74J3GYNZTXALLAFLYCZ5FXAVCNFSM6AAAAAA65AU3Z6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJTGUYDKNBTGQ. You are receiving this because you are subscribed to this thread.Message ID: @.***>

PartialVolume commented 9 months ago

@mdcato I can't disagree with anything you have said.

From a personal point of view I'd much rather spend the time building an arm version of ShredOS than continue building 32bit x86 versions.

I just don't know how many out there still use 32 bit. From the ShredOS download counters most downloads are 64 bit but 32 bit does still get downloaded. Just nowhere near as many.

mdcato commented 9 months ago

I wonder if some of the 32 bit ShredOS downloads are "I'll just get them both for sake of completeness".

After sending my email, I started looking at cost of ARM computers. While there is a major focus on ARM-based servers to increase performance and reduce power, the Raspberry Pi 5 could make a reasonable development/test rig with it's 4-core ARM Cortex-A76 2.4GHz CPU, and PCIExpress FPC connector. Still in it's infancy, but a lot cheaper for volunteer developers. I'm curious whether a (or a couple) USB3-to-SATA adapter(s) would work. From what I've read, the PCIe is functional, the hard part is adapter boards to full-sized PCIe connectors, but that will come (shortly I hope).

ARM-based servers are expensive, as would be expected since the hot market is dropping power consumption while increasing performance and density.


From: PartialVolume @.> Sent: Saturday, November 4, 2023 18:19 To: martijnvanbrummelen/nwipe @.> Cc: Mike Cato / Hays Technical Services @.>; Mention @.> Subject: Re: [martijnvanbrummelen/nwipe] move the define off64_t (PR #516)

@mdcatohttps://github.com/mdcato I can't disagree with anything you have said.

From a personal point of view I'd much rather spend the time building an arm version of ShredOS than continue building 32bit x86 versions.

I just don't know how many out there still use 32 bit. From the ShredOS download counters most downloads are 64 bit but 32 bit does still get downloaded. Just nowhere near as many.

— Reply to this email directly, view it on GitHubhttps://github.com/martijnvanbrummelen/nwipe/pull/516#issuecomment-1793578676, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ANGK2PQZFQ4AJUIFXT23CNTYC3EQJAVCNFSM6AAAAAA65AU3Z6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJTGU3TQNRXGY. You are receiving this because you were mentioned.Message ID: @.***>