There are some missing sqlite3 in the buildsystem now. What has been expected :-) How to fix it ?

How to fix it ?

The way readline is handled in configure.ac, Makefile.am and meson.build is probably a good example.
Search for READLINE_LIBS in autotools files and 'readline' in meson files.

I recommend to start with only one of the buildsystems and get it to work and reviewed.

How to fix it ?

The way readline is handled in configure.ac, Makefile.am and meson.build is probably a good example. Search for READLINE_LIBS in autotools files and 'readline' in meson files.

I recommend to start with only one of the buildsystems and get it to work and reviewed.

But this would mean that lastlog2 gets never tested, as you have to completely disable it in the build process for all CI tests.
Wouldn't it be better to add sqlite3-dev (or however the Ubuntu package name is) to the package list in .github/workflows/cibuild-setup-ubuntu.sh?

Wouldn't it be better to add sqlite3-dev

sqlite3 should already be pulled in transitively (it should also be added explicitly, though).
Currently the sqlite headers are already used and compilation itself works..

It then fails during linking because the makefiles just don't link with libsqlite.
Before merging both buildsystems should be finished.

Could you also hook up the tests to the testsuite in tests/ts/liblastlog2/... ?

Could you also hook up the tests to the testsuite in tests/ts/liblastlog2/... ?

I have added it.

The library can be used without any reference to time_t at all.
So an application can use it on a platform with 32bit time_t through the explicit int64_t type and still be fine.

That's correct for the application, but

We should also adapt all callers of liblastlog2 inside util-linux to use explicit int64_t.

You cannot call the time functions with int64_t as argument, at least s390 would break horrible this way. That's why there are a few places in the code which use time_t and not int64_t. That's the only way to make sure that the time functions work correct on all architectures.
For the standalone lastlog2 the test was necessary to make sure, time_t is always 64bit independent of the architecture. But with the integration of util-linux the test doesn't make much sense, I agree. Or we build lastlog2 code with 64bit time_t even on 32bit architectures, but not the rest of util-linux.
But as written: for now we can live with a 32bit time_t and switch later to a 64bit time_t, this will neither break the ABI nor the internal data structures.

You cannot call the time functions

liblastlog2 is not calling any time functions, or?

few places in the code which use time_t

These are currently failing to build on i386 exactly because it's passing mismatched pointers around.

But as written: for now we can live with a 32bit time_t and switch later to a 64bit time_t, this will neither break the ABI nor the internal data structures.

Sounds good.

Except the problem that the tests are not built under meson.

Yes indeed ! I have fixed it.

Hi,
may I ask if there is still something missed from my side :-)

There are still unaddressed old review comments. Even though some are marked as resolved.

Ups yes, I have found them and will take care about it. Thanks !

It looks good. I have some notes:

I think the best design for APIs is to use some generic opaque handler. The reason is that such a library is possible to extend/change in arbitrary ways with minimal impact on the library's backward compatibility.

(For example, I can imagine that one day, you decide on a different layout for the DB table or/and you will want to support multiple tables in the DB. You will be happy with a handler to change a table name for all ll2_ API functions.)

In the case of liblastlog2, lastlog2_path is everywhere as the first argument for API functions. I'd suggest using any struct ll2_context (or using any better name than context) rather than the path.

cxt = ll2_new_context("/path/to/db");
ll2_import_lastlog(cxt,  "/var/log/lastlog", &error);
ll2_unref_context(cxt);

You can also support NULL as context to get a default hardcoded behavior. ( ll2_import_lastlog(NULL, "/var/log/lastlog", &error); ).

It seems the context would be usable, for example, as an application-defined pointer for sqlite3_exec() to generate an error message in the callback() on broken entries. Now it prints to stderr.

--
Please do not use fprintf(stderr) in the shared library. It's fine to compose an error message and return it by &error pointer, but never write to stderr or stdout in the shared library.

cxt = ll2_new_context("/path/to/db");
ll2_import_lastlog(cxt, "/var/log/lastlog", &error);
ll2_unref_context(cxt);

I like the name "context" :-) and have adapted your suggestion to the API.

-- Please do not use fprintf(stderr) in the shared library. It's fine to compose an error message and return it by &error pointer, but never write to stderr or stdout in the shared library.

Yes, good point. I have removed all stdout,stderr calls.

Hm, somehow three tests are failing:

fdisk/mbr-nondos-mode
fdisk/sunlabel
libfdisk/gpt

But I think that it is not an issue of this PR because other PRs have the same problems. e.g.:
#2647

So far I have tried to adapt all your request to the code. Thank you for your hints !
I have not rebased the code. Perhaps it helps you to see the changes faster.
Of course, I will have to rebase it again after you have checked the changes. :-)

Thanks! I have a few notes:

• It's better not to use typedef for structs. API is usually easier to use if you know that the type is struct. For more details see: https://www.kernel.org/doc/html/v4.10/process/coding-style.html#typedefs

• The optimal solution is to introduce lastlog2P.h (private library header file) and keep the struct definition there. It will also be easy to work with code in the future if you have public and private header files. The private file also makes keeping code in more .c files simple.

• Public lastlog2.h header file:

struct ll2_context;

• Private lastlog2P.h header file:

#include "lastlog2.h"

struct ll2_context {
    char *lastlog2_path;
};

• For the future extendability of the library, it would be better to be careful with const for the context struct. For example, in libmount, we almost nowhere use const for the context struct because, one day, it may be necessary to modify the struct for some reason in some of the API functions.

• Yes, ignore regression tests not related to lastlog2 ;-)

@schubi2 you need to add lastlog2P.h to liblastlog2/src/Makemodule.am:liblastlog2_la_SOURCES; otherwise, it will not be included in tarball (and make distcheck will not be happy).

The rest looks good.

@t-8ch any review & comment? ;-)

@schubi2 you need to add lastlog2P.h to liblastlog2/src/Makemodule.am:liblastlog2_la_SOURCES; otherwise, it will not be included in tarball (and make distcheck will not be happy).

Yes, thanks for the hint !

The wall.c problem should be fixed now; it's not your fault.

One last design discussion:
I'm not entirely sure about the context argument being optional.
It could make future changes to the library harder.

@schubi2 That last rebase seems to have gone wrong.

One last design discussion: I'm not entirely sure about the context argument being optional. It could make future changes to the library harder.

Hm, I like that way. But it is up to you what you are prefering. :-)

@schubi2 That last rebase seems to have gone wrong.

Yes, "git rebase" is sometimes tricky. At least for me ;-)

Regarding the build error. Some other PRs have the same problem like:
#2711
So, I fear there is another reason for it than the current PR. Or I am wrong ?

See #2718
Thanks for caring!