highlevel: remove most of error handling from public API #2432

kodebach · 2019-02-20T12:18:17Z

As discussed in #2399, this removes most of the current error access functions from the public API.

It also adds border tests for integer types and therefore closes #2430.

@markus2330 please review my pull request

kodebach · 2019-02-20T19:29:29Z

jenkins build all please

kodebach · 2019-02-20T23:15:31Z

jenkins build all please

markus2330

Thank you! If possible we should avoid having to include kdbprivate.h, otherwise everything looks very fine.

Is the high-level API ready to be released after this PR?

markus2330 · 2019-02-22T08:19:36Z

src/error/exporterrors.cpp

@@ -524,6 +524,7 @@ static ostream & printPrivate (ostream & os, parse_t & p)
 	   << endl
 	   << "#include <elektra/types.h>" << endl
 	   << "#include <elektra/error.h>" << endl
+	   << "#include <kdbprivate.h>" << endl


Why is this needed? It would be good if we can avoid it. If it is only about the enum, we better move the enum within here.

It's needed for ElektraErrorSeverity, which is used in the generated function elektraErrorLowLevel. The function is used to create the ElektraError struct for low-level errors.

I don't think it is a problem to include kdbprivate.h here. The generated file is elektra/errorsprivate.h and is not intended for external use. It is not even included in any CMake install targets. It is not referenced in any header files, so it basically doesn't exist outside of compiling Elektra.

markus2330 · 2019-02-22T08:19:50Z

src/include/elektra/error.h

-
-typedef const char * ElektraKDBErrorGroup;
-typedef const char * ElektraKDBErrorModule;
-
 typedef void (*ElektraErrorHandler) (ElektraError * error);

 ElektraErrorCode elektraErrorCode (const ElektraError * error);


Remove this also?

markus2330 · 2019-02-22T08:23:14Z

tests/kdb/testkdb_highlevel.cpp

@@ -12,6 +12,7 @@

 #include <gtest/gtest-elektra.h>
 #include <kdbhelper.h>
+#include <kdbprivate.h>


We also should have ABI tests, i.e. tests that only use the public interface.

The tests themselves do only access the public interface. Some use the low-level API too to check the results, but kdbprivate.h is only used for the fatal error handler that outputs error messages. In particular we only use the elektraError* and elektraKDBError* functions that were part of the public interface before this PR.

For ABI tests it would be needed that no methods of kdbprivate.h is used at all. Otherwise the ABI tests would also fail on changes within kdbprivate.h.

markus2330 · 2019-02-22T08:23:45Z

tests/kdb/testkdb_highlevel.cpp

+
+	createElektra ();
+
+	EXPECT_EQ (elektraGetOctet (elektra, "octet/min"), 0) << "octet/min wrong";


Thank you for adding the tests!

kodebach · 2019-02-22T17:45:10Z

Is the high-level API ready to be released after this PR?

I was thinking, now that we decided to use the specload plugin for code generation, the defaults parameter of elektraOpen causes more problems than it solves. We might want to remove it. But I wouldn't actually remove the parameter. I would just not use it and rename it to config. That way we are prepared, if we need to pass more configuration parameters to elektraOpen in the future (e.g. enabling/disabling certain global plugins, or anything else that has to be done between kdbOpen and kdbGet).

kodebach · 2019-02-22T18:29:49Z

That way we are prepared, if we need to pass more configuration parameters to elektraOpen in the future

Supporting the notification API through the highlevel API (including code generation) for example would require configuration between kdbOpen and kdbGet. In this case no reasonable defaults exist, unlike with cmd-line options, so configuration has to be user supplied.

We should also probably provide a elektraGetKDB function that returns the KDB handle used by the highlevel API, so that it can be combined with the low-level API more easily. Possibly we also expose the internal KeySet, but I am not sure about that. Although both of these can be added in a future release.

markus2330 · 2019-02-22T22:08:36Z

I was thinking, now that we decided to use the specload plugin for code generation, the defaults parameter of elektraOpen causes more problems than it solves.

The default parameter is still needed because also kdbOpen() or some unrelated part within kdbGet() can fail. The goal would be that the application is always be able to start, independent of KDB.

Supporting the notification API through the highlevel API (including code generation) for example would require configuration between kdbOpen and kdbGet.

You mean for elektraIoSetBinding? I don't think that "configuration" would be an elegant solution, it seems to be more straight-forward to call functions because you need to pass function pointers.

[get internal KDB and KeySet] Although both of these can be added in a future release.

Yes, let us only add it later when it is clear that it will be needed. (e.g. if we decide not to wrap functions like elektraIoSetBinding)

kodebach · 2019-02-23T00:11:12Z

I don't think that "configuration" would be an elegant solution, it seems to be more straight-forward to call functions because you need to pass function pointers.

Yes, passing any sort of non-string data is very ugly via a KeySet, but I don't see a better solution other than using a custom struct which has to be allocated again. Any configuration before kdbOpen or before kdbGet has to be done by elektraOpen, so the user has to give that information somehow. We also can't delay the kdbGet call until the first elektraGet* because then elektraGet* could fail.

kodebach · 2019-02-23T00:49:17Z

The default parameter is still needed because also kdbOpen() or some unrelated part within kdbGet() can fail. The goal would be that the application is always be able to start, independent of KDB.

Currently elektraOpen always returns NULL if kdbOpen or kdbGet fails.

markus2330 · 2019-02-23T10:29:56Z

Yes, for severe problems elektraOpen is allowed to fail. But for a later elektraGet also kdbGet (or even kdbOpen in case of new mountpoints) might fail. But then we should continue to run, as we do not want to fail during run-time.

kodebach · 2019-02-24T00:20:24Z

jenkins build all please

kodebach · 2019-02-24T01:42:23Z

jenkins build all please

kodebach · 2019-02-24T14:49:38Z

Contrary to what github thinks, all build jobs where successful. One of the Cirrus CI jobs is listed twice, clicking on Details and viewing on Cirrus CI reveals that the job finished.

markus2330 · 2019-02-25T17:54:44Z

Thank you for the great work!

kodebach added ready to merge work in progress and removed ready to merge labels Feb 20, 2019

kodebach added 2 commits February 20, 2019 17:58

highlevel: remove most of error handling from public API

67e26c1

highlevel: add integer border tests

f42bb34

kodebach force-pushed the elektra_api branch from 1b29167 to f42bb34 Compare February 20, 2019 17:50

kodebach added ready to merge and removed work in progress labels Feb 21, 2019

markus2330 requested changes Feb 22, 2019

View reviewed changes

markus2330 removed the ready to merge label Feb 22, 2019

kodebach added the work in progress label Feb 22, 2019

highlevel: remove error code from public API

fe616c4

markus2330 mentioned this pull request Feb 23, 2019

Error message concept #2435

Merged

highlevel: fix memleak

8115e3c

kodebach added ready to merge and removed work in progress labels Feb 24, 2019

markus2330 approved these changes Feb 25, 2019

View reviewed changes

markus2330 merged commit 90ab22e into ElektraInitiative:master Feb 25, 2019

kodebach deleted the elektra_api branch June 13, 2019 18:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

highlevel: remove most of error handling from public API #2432

highlevel: remove most of error handling from public API #2432

kodebach commented Feb 20, 2019 •

edited

kodebach commented Feb 20, 2019

kodebach commented Feb 20, 2019

markus2330 left a comment

markus2330 Feb 22, 2019

kodebach Feb 22, 2019

markus2330 Feb 22, 2019

markus2330 Feb 22, 2019

kodebach Feb 22, 2019

markus2330 Feb 22, 2019

markus2330 Feb 22, 2019

kodebach commented Feb 22, 2019

kodebach commented Feb 22, 2019

markus2330 commented Feb 22, 2019

kodebach commented Feb 23, 2019

kodebach commented Feb 23, 2019

markus2330 commented Feb 23, 2019

kodebach commented Feb 24, 2019

kodebach commented Feb 24, 2019

kodebach commented Feb 24, 2019

markus2330 commented Feb 25, 2019


		createElektra ();

		EXPECT_EQ (elektraGetOctet (elektra, "octet/min"), 0) << "octet/min wrong";

highlevel: remove most of error handling from public API #2432

highlevel: remove most of error handling from public API #2432

Conversation

kodebach commented Feb 20, 2019 • edited

kodebach commented Feb 20, 2019

kodebach commented Feb 20, 2019

markus2330 left a comment

Choose a reason for hiding this comment

markus2330 Feb 22, 2019

Choose a reason for hiding this comment

kodebach Feb 22, 2019

Choose a reason for hiding this comment

markus2330 Feb 22, 2019

Choose a reason for hiding this comment

markus2330 Feb 22, 2019

Choose a reason for hiding this comment

kodebach Feb 22, 2019

Choose a reason for hiding this comment

markus2330 Feb 22, 2019

Choose a reason for hiding this comment

markus2330 Feb 22, 2019

Choose a reason for hiding this comment

kodebach commented Feb 22, 2019

kodebach commented Feb 22, 2019

markus2330 commented Feb 22, 2019

kodebach commented Feb 23, 2019

kodebach commented Feb 23, 2019

markus2330 commented Feb 23, 2019

kodebach commented Feb 24, 2019

kodebach commented Feb 24, 2019

kodebach commented Feb 24, 2019

markus2330 commented Feb 25, 2019

kodebach commented Feb 20, 2019 •

edited