Enable CLOEXEC if possible

[NiLuJe]

Noticed lingering fds in the WiFi scripts for the book & CRe cache ;).

Windows support untested, obviously, but I tried to handle it sanely ;o).

---

This change is 

[NiLuJe]

Note to self: need to disable this on Kindle legacy, because it's glibc 2.6+.

So I need a visible preprocessor constant defined there.

[poire-z]

We don't use most of the things you updated (mostly debug stuff).
The only ones that seem to handle all your real life cases must be the ones in lvstream.cpp.

[NiLuJe]

Yep, but I figured why not do everything while I'm there ;).

[poire-z]

Yep, but I figured why not do everything while I'm there ;).

Because you are not testing each of them :)

[NiLuJe]

Okay, ready for real :].

[NiLuJe]

Yep, but I figured why not do everything while I'm there ;).

Because you are not testing each of them :)

There's no magic involved anywhere: if it builds, it works. If it works in one place, it works everywhere.

[poire-z]

Still feels awkward going at updating some old, obsolete, or commented out code that may not even compile if decommented :) and leave traces in git history that it's been updated in 2021 and so probably was tested and worked in 2021.
A bit more bothering are the 40 lines of low-level technicalities added to lvtypes.h - which is some rather crengine-business code (lvPoint, lvRect) and logically not the place for that kind of stuff - which should probably be in the .cpp that implements the stuff that needs it, so logically lvstream.cpp, if you didn't need it to be that globally accessible. But I can't find another .h that could welcome that.

[NiLuJe]

There's really zero chance for surprises.

Agreed about the header, but unfortunately we don't have a global "internal" header that's included everywhere. Might be time for one, I'll see what I can do

[NiLuJe]

Hmm, header situation isn't great.

I can see a couple of choices, but I'll start by saying that the current approach seems like the best one.

1. Move it to crsetup.h. Meh.
2. Move it to a new dedicated crengine.h header, make that include lvtypes.h & crsetup.h, and clean everything up to only include crengine.h

[poire-z]

Move it to a new dedicated crengine.h header, make that include lvtypes.h & crsetup.h, and clean everything up to only include crengine.h

There is already a crengine.h, that includes everything. But I guess it's for external frontends use.
The current situation is what it is, and works for the various frontends.
And base/cre.cpp includes lvtypes.h somehow - I guess your stuff should not leak into frontends.
So, I guess for such a little thing, don't change the whole world. I'd say put it in crengine/include/lvmisc.h or lvlowlevel.h and just include that where you need it.
(If you give up wanting to add it to every other dead code or debug stuff that really don't need CLOEXEC, you might end up just adding it in lvstream.cpp which is the only place where it really matters. :)

[NiLuJe]

I'm fine with it there, after all, it's right below the path separator define, which is also platform and I/O related, so, there's a precedent ;).

[poire-z]

(Even if I promise to replace 10% of my future ints with proper size_t ?:)

[NiLuJe]

The idea is that it needs to be accessible everywhere (back/front), so lvtypes.h fits the bill just right, without having to tweak anything re: new includes or existing includes handling (especially as far as the seven billion different build system goes, not having to touch any of that is a plus).

It's also completely inert until code actually explicitly asks for STDIO_CLOEXEC in fopen calls, so it's perfectly harmless.

[poire-z]

The idea is that it needs to be accessible everywhere (back/front)

What do you mean ? base & front ? Except cre.cpp, nothing should use things from crengine.
The only thing I see in koreader/koreader-base#1349 related is add_definitions(-DDISABLE_CLOEXEC) which would reach anywhere.

It's also completely inert until code

Except for polluting lvtypes.h.
My understanding is that any business code in crengine should go use lvstream.h/.cpp to handle anything "files". Everything else is commented out/dead/debug code that the programmer was (rightfully) too lazy to go thru the overhead of using lvstream. But any serious stuff (hyphenation dicts, reading books, cache files) use lvstream.cpp.

But if I can't convince you, go ahead. I find this quite ugly, but well, at least I don't have to keep that promise :)

[NiLuJe]

The fact that it concerns mainly I/O is secondary: it's highly generic platform-specific stuff, which is exactly what lvtypes deals with ;).

[NiLuJe]

The idea is that it needs to be accessible everywhere (back/front)

What do you mean ? base & front ?

I simply meant anything that uses CRe. i.e., CRe itself (emphasis on the "e", i.e., the engine itself), and CRe frontends (in our case, yeah, that's cre.cpp).

[poire-z]

Mhhhh, either you're not reading me (so, even if I want to give up, I feel like I should insist repeating stuff so you can make an educated decision :), or you have other ideas/plans for the future related to this you're not telling (which I've seen before :)

The fact that it concerns mainly I/O is secondary: it's highly generic platform-specific stuff, which is exactly what lvtypes deals with ;).

Uh! The fact that it concerns mainly I/O is primary, that's what it deals with.
That it's highly generic platform-specific stuff, that's secondary and what makes it so long.
And it's not exactly what lvtypes.h deals with: it primary deals with types :)
Platform specific stuff is in each file that handles a specific domain: lvstream, lvfntman, lvstring, lvthread.
So, that's exactly what lvstream deals with :)

The idea is that it needs to be accessible everywhere (back/front)
I simply meant anything that uses CRe. i.e., CRe itself (emphasis on the "e", i.e., the engine itself), and CRe frontends (in our case, yeah, that's cre.cpp).

Well, no. CLOEXEC stuff does not need to be accessible everywhere. CRe and cre.cpp do not need it: they should use lvstream, which hides the implementation and any platform specific stuff.
There is ZERO *open() call in cre.cpp. There are 4 LVOpenFileStream() (to handle cr3.ini and loading a background image). Everything else is commented out debug code that should not get compiled into releases and don't need CLOEXEC.

[NiLuJe]

I indeed feel like we're talking about vastly different things, for some reason I can't really comprehend.

It's low-level platform specific stuff. It should be visible everywhere (compare it to system-level includes, because that's what it is: an extension of <fcntl.h>, whether you use it or not, as long as fcntl itself was included: the point is that if you want to, you can, without requiring anything extra, because CRe took care of it for you (... via lvtypes.h ;p)).

I mean, I can move it to a new lvplatform.h and include that in lvtypes.h, but that feels a bit silly and doesn't really change anything in the grand scheme of things (... because nothing needs to ^^).

[poire-z]

It should be visible everywhere [...] whether you use it or not [...] the point is that if you want to, you can [...] because CRe took care of it for you

Mhhh, that's a bit excessive caring :)
Should I forward freetype/ftsynth.h to any frontend because if it wants to synthetize glyphs, then, it could. Obviously not. It should use crengine public APIs from lvtext.h.
Same, if some frontend would feel the need to use CLOEXEC, no: it should use crengine public file I/O APIs from lvstream.h.
Given how involved you are in low-level stuff, I understand it feels like a needed toolkit. But as crengine being a library that does its thing, I think it shouldn't be the toolkit you need (and polute importers with low-level extensions while crengine itself provides a higher level toolkit for this).

I mean, I can move it to a new lvplatform.h

That makes sense.

and include that in lvtypes.h,

That doesn't.
Include it in lvstream.cpp (and in the other files that have dead code that would use it if uncommented) - let cre.cpp be able to include it the day one of use decides to do low level i/o stuff in there.

[NiLuJe]

Okay, then, completely unnecessary revamp of headers inclusion incoming ;).

[poire-z]

Why not credit yourself ? :)

[NiLuJe]

Meh, everything's out of date anyway, and it essentially boils down to

#define PATH_SEPARATOR_CHAR '/'
#define STDIO_CLOEXEC "e"

in 99% of our builds.

So, nothing really all that tangible ^^.

[poire-z]

Thanks, that looks more like how it should be :) including the lvstring/lvtypes fixup.

How did you find out what needed which header? Just compilations, and fixing errors as they were spitted out ?

[NiLuJe]

With the help of my best friend ripgrep ;).

(i.e., anything that uses PATH_SEPARATOR_CHAR/STDIO_CLOEXEC needs lvplatform; and anything that uses lInt/lUint/lChar needs lvtypes).

[NiLuJe]

Good to go @poire-z?

[poire-z]

Yes, go ahead.