notification: improved API #1850

waht · 2018-03-21T19:55:55Z

Purpose

This PR improves the notification API. It adds new types (both C and kdbtypes.h). It also adds macros for generating API functions for new types and also adds C99 definitions to kdbtypes.h.

Checklist

commit messages are fine (with references to issues)
I added unit tests
I ran all tests and everything went fine
affected documentation is fixed
I added code comments, logging, and assertions (see doc/CODING.md)
release notes are updated (doc/news/_preparation_next_release.md)
rebase when dbus PR (notification: dbus transport plugins #1825) is merged into master.
update notification tutorial: explain emergent misbehavior & add guidelines
update architecture diagram

@markus2330 please review my pull request

waht · 2018-03-22T11:33:37Z

jenkins build multiconfig-gcc-stable please

waht · 2018-03-22T22:08:28Z

jenkins build multiconfig-gcc47-cmake-options please

waht · 2018-03-23T13:19:12Z

jenkins build all please

markus2330 · 2018-03-24T07:25:39Z

Contains commits from #1825 please rebase. I could not do a review because the shown diff is huge.

Please also fix the leftovers of #1825 here.

waht · 2018-03-24T11:05:57Z

Okay, I've fixed the leftovers and rebased the PR - the diff is much smaller now :)

markus2330

looks great, I am looking forward to it.

markus2330 · 2018-03-24T17:25:44Z

doc/Doxyfile

@@ -2039,7 +2039,7 @@ PREDEFINED             = "ELEKTRA_UNUSED="
 # definition found in the source code.
 # This tag requires that the tag ENABLE_PREPROCESSING is set to YES.

-EXPAND_AS_DEFINED      =
+EXPAND_AS_DEFINED      = "ELEKTRA_NOTIFICATION_TYPE_DECLARATION"


great that you considered this!

markus2330 · 2018-03-24T17:26:32Z

doc/tutorials/notifications.md

@@ -11,7 +11,7 @@ Development state:
 - [x] notification wrapper (support for int and callback)
 - [ ] transport plugin zeromq
 - [ ] transport plugin dbus
- - [ ] transport plugin redis
+ - [-] transport plugin redis


simply remove the line

markus2330 · 2018-03-24T17:27:34Z

doc/tutorials/notifications.md

@@ -195,7 +195,7 @@ changed key needs further processing.
 #define ANSI_COLOR_RED			"\x1b[31m"
 #define ANSI_COLOR_GREEN		"\x1b[32m"

-void setTerminalColor (Key * color)
+void setTerminalColor (Key * color, void * context)


ELEKTRA_UNUSED

markus2330 · 2018-03-24T17:30:18Z

src/include/kdbnotificationinternal.h

+#define ELEKTRA_NOTIFICATION_TYPE_DEFINITION(TYPE, TYPE_NAME)                                                                              \
+	ELEKTRA_NOTIFICATION_REGISTER_SIGNATURE (TYPE, TYPE_NAME)                                                                          \
+	{                                                                                                                                  \
+		if (!kdb || !key || !variable)                                                                                             \


please use ELEKTRA_NOT_NULL, it is exactly designed for this purpose

Yes, but I did not use it because this function is user facing.

markus2330 · 2018-03-24T19:31:27Z

src/include/kdbtypes.h

+typedef uint32_t kdb_unsigned_long_t;
+typedef uint64_t kdb_unsigned_long_long_t;
+
+#if SIZEOF_LONG == 4


The #if is not necessary. In C99 inttypes.h defines macros for the printf modifiers, e.g., PRIu8

Great, I didn't know about these!

markus2330 · 2018-03-24T19:33:53Z

src/plugins/internalnotification/internalnotification.c

+		if (checkKeyIsBelowOrSame (check, current))
+		{
+			result = 1;
+			break;


return 1;

markus2330 · 2018-03-24T19:34:25Z

src/plugins/internalnotification/internalnotification.c

+			break;
+		}
+	}
+	return result;


return 0;

markus2330 · 2018-03-24T19:34:48Z

src/plugins/internalnotification/internalnotification.c

+			if (key == NULL)
+			{
+				registeredKey = registeredKey->next;
+				continue;


some refactoring here?

markus2330 · 2018-03-24T19:36:35Z

src/plugins/internalnotification/internalnotification.c

 		}

 		registeredKey = registeredKey->next;
 	}
 }

+// Generate register and conversion functions
+INTERNALNOTIFICATION_TYPE (int, long int, Int, (strtol (string, &end, 10)),


Very compact, well done!

Is this somehow reusable for other places? (maybe as super macro?)

markus2330 · 2018-03-24T19:38:52Z

src/plugins/internalnotification/testmod_internalnotification.c

+	test_doUpdateShouldNotUpdateKeyAbove ();
+	test_doUpdateShouldNotUpdateUnregisteredKey ();
+	test_doUpdateShouldUpdateKeyAbove ();
+
 	printf ("\ntestmod_internalnotification RESULTS: %d test(s) done. %d error(s).\n", nbTest, nbError);


should use the print from the test suite.

markus2330

Please use all-lowercase filenames.

markus2330 · 2018-03-25T15:52:58Z

src/plugins/internalnotification/macros/addType.h

@@ -0,0 +1,71 @@
+#ifndef TYPE


Please all-lowercase filenames, see also #1615

Please add some docu how to use the supermacro.

Will it work for @domhof (#1605) and to replace support.setfuncname in src/tools/gen/template/template.h?

While my macros were inspired by his PR I think the only reusable part would be the conversion function. If needed we can extract it but details like error handling would need to be discussed. Theoretically we could have a lib dedicated to string to value and value to string conversions.

Yes, let us discuss this.

Ok! I'll think about error handling and reply not later than Thursday.

Ok. With a return value we cannot determine wether the conversion failed (e.g. return a value of -1 is within valid range for int, but not for unsigned int). So I suggest we return either the error and have a pointer to the variable to updated or the other way around. The first option would be convenient for integration into my code, but not convenient to use otherwise :)

I think the best approach is to have a pointer to an error value which is updated after the function. If no error occured it will be 0, otherwise it will be 1, if NULL is passed no error information can be passed. The return value of the conversion function is the converted value or undefined in case of an error.

What should be name of the conversion library? elektra-type, elektra-conversion? I would create it and add string to value functions using (super?)macros.

undefined in case of an error.

If possible, the value should stay unchanged.

What should be name of the conversion library?

If you can implement it as (super)macros no extra library is needed. Otherwise elektra-type might be suitable. We should also add keySetLong, keyGetLong, etc. functions.

If possible, the value should stay unchanged.

Ok its better to have additional variable declarations & logic in the macro instead of scattering it in the code base.

If you can implement it as (super)macros no extra library is needed.

Of course! :) A naming macro as input would be required though to avoid name collisions.

We should also add keySetLong, keyGetLong, etc. functions.

For such functions a seperate library would be better suited. Also I think this would broaden the scope of the PR too much. For now I'll better stick to the conversion macros.

I added a supermacro called type_create_to_value for creating "to value" conversion functions in "include/macros". I decided to return the error and have a pointer for the variable because it improves readability:

int variable = 0; if (!myIntConversion (key, &variable)) { // error, variable is still 0 }

is better than

int variable = 0; int error = 0; variable = myIntConversion (key, error) if (error) { // error, variable was changed to 0 because of return }

For such functions a seperate library would be better suited. Also I think this would broaden the scope of the PR too much. For now I'll better stick to the conversion macros.

Yes, of course! I am surprised that you put so much effort in supporting all types. I only mentioned keySetLong to make clear what elektra-type could be used for. But header-only without any lib is the better solution for this case.

Ok, great! Then the PR should be complete :)

markus2330 · 2018-03-25T15:53:49Z

src/plugins/internalnotification/macros/addType.h

+#define CHECK_CONVERSION 1
+#endif
+
+#define CONCAT(X, Y) CONCAT2 (X, Y)


Is this needed as macro?

Yes. It concatenates two tokens. It was not necessary before since it was already encapsulated in a macro before expansion. I'm not sure how to better describe this, so here is an example :)

// in supermacro.h #define CONCAT(X, Y) CONCAT2 (X, Y) #define CONCAT2(X, Y) X##Y #define FUNC_NAME(TYPE_NAME) CONCAT (internalnotificationRegister, TYPE_NAME) #define FUNC_NAME2(TYPE_NAME) internalnotificationRegister##TYPE_NAME #define NESTED(TYPE_NAME) FUNC_NAME2 (TYPE_NAME) FUNC_NAME (TYPE_NAME) FUNC_NAME2 (TYPE_NAME) NESTED (TYPE_NAME) // in some.c #define TYPE_NAME Int #include "macros/supermacro.h" // => gcc -E some.c internalnotificationRegisterInt internalnotificationRegisterTYPE_NAME internalnotificationRegisterInt

Before the "supermacro", the NESTED macro was the macro that generated functions. A solution would be to add another macro (e.g. #define FUNC_SIGNATURE(TYPE_NAME) void FUNC_NAME(TYPE_NAME) (void) but the CONCAT macro is more versatile.

Okay, then we should also add ELEKTRA_CONCAT next to ELEKTRA_STRINGIFY.

markus2330 · 2018-03-25T15:55:02Z

src/plugins/internalnotification/macros/addType.h

+	}
+	else
+	{
+		ELEKTRA_LOG_WARNING ("conversion failed! string=%s, stopped=%c errno=%d", keyString (key), *end, errno);


So the value is silently (unless the logger is enabled) not changed?

Not so ideal...

Yes it is not ideal, but the semantic of the register function is: update the variable if the setting is present and correct. I think it is better to use validation (e.g. the type plugin) to allow only valid values the key database.

I guess we could add a elektraNotificationSetConversionFailedCallback() function or something similar. The callback would recive the Key containing the invalid value.

(e.g. the type plugin) to allow only valid values the key database.

Yes, that is the goal. But there is no type validation for C types.

elektraNotificationSetConversionFailedCallback

It could be useful for applications to log and abort. But it is much better if such situations cannot occur :-)

markus2330 · 2018-03-27T14:54:03Z

Sorry, it seems like I created a merge conflict. Please rebase.

waht · 2018-03-29T11:41:43Z

jenkins build all please

waht · 2018-03-29T14:20:17Z

jenkins build gcc-configure-debian-debug please

waht · 2018-03-29T19:24:57Z

jenkins build mingw64 please

markus2330 · 2018-05-14T07:17:55Z

Is this ready for review? (remove "work in progress" if it is) Please rebase after the release.

waht · 2018-05-15T17:48:12Z

The PR is ready for review. Is it safe to rebase now?

markus2330 · 2018-05-15T20:43:05Z

Yes, release is done, so it is now a good time for rebase!

…ples to /examples

waht · 2018-08-17T16:04:37Z

jenkins build libelektra please

markus2330 · 2018-08-17T20:12:15Z

Looks much better now, is it ready to merge?

markus2330 · 2018-08-18T04:22:03Z

examples/notificationAsync.c

+ *   - Transport plugins (e.g. kdb global-mount dbus announce=once dbusrecv)
+ *
+ * Relevant keys for this example:
+ *   - /sw/elektra/example_notification/#0/current/value: Set to any integer value


If possible, I would prefer to avoid "elektra" in the path (as it is not really an Elektra-specific configuration) and the _ (/ should be used as separator).

So maybe /sw/examples/notification/...

markus2330 · 2018-08-18T04:23:41Z

doc/news/_preparation_next_release.md

@@ -477,6 +486,20 @@ Thanks to Daniel Bugl.
 [Markdown Shell Recorder]: https://master.libelektra.org/tests/shell/shell_recorder/tutorial_wrapper
 [Shell Recorder]: (https://master.libelektra.org/tests/shell/shell_recorder)

+## Compatibility


This section is now duplicated? (It is also below)

markus2330 · 2018-08-18T04:25:33Z

@waht would be great to have this for the upcoming release which hopefully happens today.

markus2330 · 2018-08-18T12:22:06Z

Thank you for the quick and good job! If this compiles I will merge it if it is okay for you?

waht · 2018-08-18T12:46:20Z

Sure! I just added a commit improving the zeromqsend connection timeout resolution.

markus2330 · 2018-08-18T15:31:15Z

Thank you, finally it is done 😄

waht force-pushed the notification_api2 branch from aba451b to 4fcf35d Compare March 22, 2018 22:04

waht added the ready to merge label Mar 23, 2018

markus2330 removed the ready to merge label Mar 24, 2018

waht force-pushed the notification_api2 branch from 4fcf35d to e946167 Compare March 24, 2018 11:04

markus2330 reviewed Mar 24, 2018

View reviewed changes

markus2330 reviewed Mar 25, 2018

View reviewed changes

waht force-pushed the notification_api2 branch from e93a54c to 4976745 Compare March 25, 2018 20:40

waht force-pushed the notification_api2 branch from 4976745 to 337f133 Compare March 28, 2018 15:28

waht force-pushed the notification_api2 branch from 4358443 to ecb8184 Compare March 29, 2018 19:16

waht force-pushed the notification_api2 branch 3 times, most recently from 7b3767f to 8d41195 Compare April 6, 2018 19:04

waht force-pushed the notification_api2 branch 3 times, most recently from 7f841bb to df1bf97 Compare April 15, 2018 09:38

kodebach mentioned this pull request May 7, 2018

Hexnumber #1903

Merged

7 tasks

markus2330 added the work in progress label May 14, 2018

waht removed the work in progress label May 15, 2018

waht added 14 commits August 17, 2018 14:30

I/O binding headers: move to "kdbio" directory

acc58b7

notification: add notification_reload example; move notification exam…

c5e641c

…ples to /examples

I/O binding headers: fix paths & install headers

8ab9741

notification: fix typo & wording

d9d6d9d

notification: add "long long" and "unsigned long long" types

65ce08e

notification: update tutorial & headers, add license

e46d092

docker: add missing deps for I/O bindings

1bd7a21

notification: updated news

1ce62d3

notification: add new tests

49ef711

reformat-cmake

dfc14fb

notification: fix unsigned conversion

4cf4c17

notification: improve unsigned conversion checks

ba65321

io-zeromq-adapter: move header to kdbio directory

6ad02a9

notification: improve docs & examples

5ecc4ce

waht force-pushed the notification_api2 branch from d79697f to 5ecc4ce Compare August 17, 2018 14:38

markus2330 reviewed Aug 18, 2018

View reviewed changes

markus2330 added this to the 0.8.24 milestone Aug 18, 2018

waht added 4 commits August 18, 2018 13:38

invoke/deferred calls: call direct and defer; fix return codes

4cec993

zeromqsend plugin: fix timeout bug

9744189

io-glib: fix for invalid access

d9f855e

notification: add news highlight & improve docs

ffb88ad

zeromqsend plugin: improve connection timeout resolution

a773bdf

markus2330 merged commit 4961215 into ElektraInitiative:master Aug 18, 2018

notification: improved API #1850

notification: improved API #1850

Conversation

waht commented Mar 21, 2018 • edited Loading

Purpose

Checklist

waht commented Mar 22, 2018

waht commented Mar 22, 2018

waht commented Mar 23, 2018

markus2330 commented Mar 24, 2018

waht commented Mar 24, 2018

markus2330 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markus2330 left a comment

Choose a reason for hiding this comment

markus2330 Mar 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waht Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waht Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waht Mar 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markus2330 Mar 25, 2018 • edited Loading

Choose a reason for hiding this comment

markus2330 commented Mar 27, 2018

waht commented Mar 29, 2018

waht commented Mar 29, 2018

waht commented Mar 29, 2018

markus2330 commented May 14, 2018

waht commented May 15, 2018

markus2330 commented May 15, 2018

waht commented Aug 17, 2018

markus2330 commented Aug 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markus2330 commented Aug 18, 2018 • edited Loading

markus2330 commented Aug 18, 2018

waht commented Aug 18, 2018 • edited Loading

markus2330 commented Aug 18, 2018

waht commented Mar 21, 2018 •

edited

Loading

markus2330 Mar 25, 2018 •

edited

Loading

waht Mar 27, 2018 •

edited

Loading

waht Mar 29, 2018 •

edited

Loading

waht Mar 25, 2018 •

edited

Loading

markus2330 Mar 25, 2018 •

edited

Loading

markus2330 commented Aug 18, 2018 •

edited

Loading

waht commented Aug 18, 2018 •

edited

Loading