-
Notifications
You must be signed in to change notification settings - Fork 123
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very clean and nice code but some design choices are to be discussed. Please prefer to first discuss the design of meta data before implementing a full plugin.
Please also make sure to write a line about your changes in the release notes.
cmake/Modules/LibAddMacros.cmake
Outdated
@@ -573,7 +573,7 @@ function (generate_readme p) | |||
STRING(REGEX REPLACE "\n" "\\\\n\"\n\"" contents "${contents}") | |||
STRING(REGEX REPLACE "- infos = ([a-zA-Z0-9 ]*)\\\\n\"" "keyNew(\"system/elektra/modules/${p}/infos\",\nKEY_VALUE, \"\\1\", KEY_END)," contents "${contents}") | |||
STRING(REGEX REPLACE "\"- +infos/licence *= *([a-zA-Z0-9 ]*)\\\\n\"" "keyNew(\"system/elektra/modules/${p}/infos/licence\",\nKEY_VALUE, \"\\1\", KEY_END)," contents "${contents}") | |||
STRING(REGEX REPLACE "\"- +infos/author *= *([.@<>a-zéA-Z0-9 %_-]*)\\\\n\"" "keyNew(\"system/elektra/modules/${p}/infos/author\",\nKEY_VALUE, \"\\1\", KEY_END)," contents "${contents}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As said, everything that is invalid within C-strings, must be invalid, too. E.g. "
.
src/plugins/hexnumber/README.md
Outdated
## Introduction | ||
|
||
This plugin can be used to read configuration files that use hexadecimal values. All hexadecimal values (strings starting with 0x) will be | ||
converted into decimal when the Elektra reads values from the mounted file. When Elektra writes back to the file all values originally |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
long sentence
src/plugins/hexnumber/README.md
Outdated
- To mount a simple backend that uses hexadecimal numbers, you can use: | ||
|
||
```sh | ||
sudo kdb mount test.ecf /examples/hexnumber/test hexnumber |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please give an example.
src/plugins/hexnumber/README.md
Outdated
```sh | ||
sudo kdb umount /examples/hexnumber/test | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are there any limitations?
src/plugins/hexnumber/README.md
Outdated
|
||
This plugin can be used to read configuration files that use hexadecimal values. All hexadecimal values (strings starting with 0x) will be | ||
converted into decimal when the Elektra reads values from the mounted file. When Elektra writes back to the file all values originally | ||
stored as hexadecimal will be converted back and stored as before. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if a string starts with 0x and is not a number? (0xX) Shouldn't we use type and check/type to be sure it is a number?
Are lower and upper characters A-F allowed? Is 0X allowed?
src/plugins/hexnumber/hexnumber.c
Outdated
/** | ||
* Establish the plugin contract and convert all hexadecimal values in the KeySet to decimal. | ||
* | ||
* @note The plugin will attempt to convert ALL values starting with 0x from hexadecimal into decimal. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notes here will not be read by the user of the plugin.
The feature seems to dangerous, see other discussions and #1398.
src/plugins/hexnumber/hexnumber.c
Outdated
int status = ELEKTRA_PLUGIN_STATUS_NO_UPDATE; | ||
while ((cur = ksNext (returned)) != NULL) | ||
{ | ||
if (!keyIsString (cur) || !isHexString (cur)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please invert the expression, then you get rid of continue.
src/plugins/hexnumber/hexnumber.c
Outdated
int status = ELEKTRA_PLUGIN_STATUS_NO_UPDATE; | ||
while ((cur = ksNext (returned)) != NULL) | ||
{ | ||
if (!keyIsString (cur) || !hasHexType (cur)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, please invert logic.
src/plugins/hexnumber/hexnumber.h
Outdated
|
||
#include <kdbplugin.h> | ||
|
||
#define ELEKTRA_HEXNUMBER_META_TYPE "hexnumber" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have to separate meta data for internal purposes and for the user to be specified.
- To remember what you have transformed to transform it back please use meta keys in
internal/<plugin>
- For what the user needs to specify, we should discuss this in units-of-measurements plugin #1398 first.
For non-internal meta-data please add the specification in doc/METADATA.ini and in the plugin's contract.
init (argc, argv); | ||
|
||
test_basics (); | ||
test_default (); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check more for errors and the validation parts of this plugin.
src/plugins/hexnumber/README.md
Outdated
@@ -0,0 +1,31 @@ | |||
- infos = Information about the template plugin is in keys below |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please replace ”template“ with the name of the plugin (“hexnumber“).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
The ../../scripts/copy-template
approach avoids such problems.
Is this ready for review? (remove "work in progress" if it is) Please rebase after the release. |
@markus2330 everything you noted in your original review should be fixed now, please review again I am not removing the "work in progress" tag, because the |
The "work in progress" basically only means "I am still working on it, please do not review". If build jobs do not work is a separate problem (except you are still trying to fix the build jobs) The build server might have less packages and environment variables. Furthermore, it is guaranteed to make a clean build with only the files as pushed in the repo. The first step to reproduce is to rebuild everything from scratch locally. |
Basically the Shell Recorder executes each line via I executed some of the commands from the ReadMe in my shell. While the following works: kdb mount test.ecf user/examples/hexnumber/test hexnumber
kdb set user/examples/hexnumber/test/hex 0x1F
kdb get user/examples/hexnumber/test/hex
#> 0x1F , the commands: kdb mount test.ecf user/examples/hexnumber/test hexnumber
kdb set user/examples/hexnumber/test/hex 0x1F
kdb setmeta user/examples/hexnumber/test/hex type int
kdb set user/examples/hexnumber/test/dec 26 # Without this command everything works fine
kdb get user/examples/hexnumber/test/hex # This command should print the value “31”
#> 1 produce unexpected output. By the way: We decided to save all test data below |
Thank you for mentioning Btw. why are you linking to the shell scripts and not the accompanied README.md? How can we improve the README.md? |
Because the code tells you exactly how the tests work (including all the warts).
I assume the best way would be to let someone without any knowledge about the (Markdown) Shell Recorder write a Markdown Shell Recorder test. Afterwards this person could then update and extend the ReadMe, adding information that might not be obvious. Another person with extended knowledge of the MSR (either you or Thomas 😊) could then write a review of the extended ReadMe correcting wrong, unclear and outdated information. |
I found the bug, everything should be working now.
@markus2330 I also do not have the access rights to remove the label.
@sanssecours explanation matches what I understood from the README, so I don't think we need to change anything. |
You should now have the rights! You were added to ElektraDevelopers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, it is nearly finished, most comments are about docu.
doc/METADATA.ini
Outdated
[unit/base] | ||
type = enum dec bin hex any | ||
default = dec | ||
description = allows values to be written in other bases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-v please, what exactly does this metadata do.
Please also say that it is used by the hexnumber plugin
@@ -59,7 +59,8 @@ We added even more functionality, which could not make it to the highlights: | |||
|
|||
## New Plugins | |||
|
|||
- <<TODO>> | |||
- The plugin [hexnumber](https://www.libelektra.org/plugins/hexnumber) has been added. It can be used | |||
to hexadecimal values into decimal when read, and back to hexadecimal when written. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some verb missing. What can it be used for?
src/plugins/hexnumber/README.md
Outdated
|
||
## Introduction | ||
|
||
This plugin can be used to read configuration files that use hexadecimal values. All "hex-values" (see below) will be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
avoid modal verbs "can be" -> "is"
src/plugins/hexnumber/README.md
Outdated
### What are "hex-values"? | ||
There are multiple ways you can signal to the hexnumber plugin, that a value should be converted: | ||
|
||
1. If a Key has the metadata `/unit/base` set to `hex` it will always be interpreted as a hex-value. The plugin will also produce an error, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unit/base should be without slash in the beginning
src/plugins/hexnumber/README.md
Outdated
|
||
1. If a Key has the metadata `/unit/base` set to `hex` it will always be interpreted as a hex-value. The plugin will also produce an error, | ||
if the value contained in such a Key does not start with `0x` (or `0X`). | ||
2. If a Key has the metadata `type` set to an integer-type and its value starts with `0x` (or `0X`) it will be interpreted as a hex-value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a Key has the metadata type
and unit/base is not present...
src/plugins/hexnumber/README.md
Outdated
|
||
# Set up examples | ||
kdb set user/tests/hexnumber/hex 0x1F | ||
kdb setmeta user/tests/hexnumber/hex type int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
int is not a CORBA type
src/plugins/hexnumber/README.md
Outdated
kdb set user/tests/hexnumber/hex2 0xF | ||
kdb setmeta user/tests/hexnumber/hex2 unit/base hex | ||
|
||
# Example 1: read hex value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is easier to read if the get is directly after the respective set.
src/plugins/hexnumber/hexnumber.c
Outdated
++ptr; | ||
} | ||
*array = elektraCalloc ((count + 1) * sizeof (char *)); | ||
char * localString = strdup (values); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use elektraStrDup (for everything that allocates memory, there is an elektra* method)
src/plugins/hexnumber/hexnumber.c
Outdated
const char * type = keyString (typeMeta); | ||
for (int i = 0; types[i] != NULL; ++i) | ||
{ | ||
if (strcmp (types[i], type) == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use elektraStrCmp
src/plugins/hexnumber/hexnumber.c
Outdated
*/ | ||
static bool isHexString (const Key * key) | ||
{ | ||
return strncasecmp (keyString (key), "0x", 2) == 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use elektraStrCaseCmp
@markus2330 everything should be fixed now, please review |
src/plugins/hexnumber/hexnumber.c
Outdated
@@ -251,76 +255,31 @@ static bool hasHexType (const Key * key) | |||
return typeMeta && elektraStrCmp (keyString (typeMeta), "1") == 0; | |||
} | |||
|
|||
/** | |||
* copied from boolean/boolean.c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for getting rid of this! ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I especially like the detailed documentation. The failing Travis build jobs should be fixed after
- you add a newline to the end of
array.c
, and - we apply PR Markdown Shell Recorder: Add Partial Support for Indented Code Blocks #2002
.
src/libs/ease/array.c
Outdated
ksDel (elements); | ||
|
||
return size; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not remove the newline at the end of the file:
src/libs/ease/array.c:300:2: error: no newline at end of file [-Werror,-Wnewline-eof]
}
^
1 error generated.
.
src/plugins/hexnumber/README.md
Outdated
## Introduction | ||
|
||
This plugin is used to read configuration files that use hexadecimal values. All "hex-values" (see below) will be | ||
converted into decimal when the Elektra reads values from the mounted file. When Elektra writes back to the file the converted values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: the Elektra
→ Elektra
src/plugins/hexnumber/hexnumber.c
Outdated
return status; | ||
} | ||
|
||
int elektraHexnumberClose (Plugin * handle ELEKTRA_UNUSED, Key * parentKey ELEKTRA_UNUSED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove ELEKTRA_UNUSED
after the argument handle
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor things to be changed together with @sanssecours requests.
@sanssecours Thank you for the review!
src/plugins/hexnumber/README.md
Outdated
|
||
1. If a Key has the metadata `unit/base` set to `hex` it will always be interpreted as a hex-value. The plugin will also produce an error, | ||
if the value contained in such a Key does not start with `0x` (or `0X`). | ||
2. If a Key has the metadata `type`, `unit/base` is not present, and the `type` value starts with `0x` (or `0X`) it will be interpreted as a hex-value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"type
value" is confusing. Maybe make two subpoints to make clear that two conditions need to be met (type
integer, and the configuration value needs to start with 0x)
src/plugins/hexnumber/README.md
Outdated
2. If a Key has the metadata `type`, `unit/base` is not present, and the `type` value starts with `0x` (or `0X`) it will be interpreted as a hex-value. | ||
The following types are recognized as integer-types per default: `byte`, `short`, `unsigned_short`, `long`, `unsigned_long`, | ||
`long_long`, `unsigned_long_long` | ||
3. If forced conversion mode (`/force`, see below) is enabled all values starting with `0x` (or `0X`) are considered hex-values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make clear that /force is a plugin configuration (and not meta data as the other two above)
src/plugins/hexnumber/README.md
Outdated
|
||
## Configuration | ||
|
||
When mounting a backend with the hexnumber plugin, a few parameters can be configured. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parameter -> setting (see https://www.libelektra.org/manpages/elektra-glossary).
And in the case of plugin configuration, its better to always use "plugin configuration".
src/plugins/hexnumber/README.md
Outdated
starting with `0x` (or `0X`) into decimal before passing the value on to the rest of Elektra. This can be useful for importing a | ||
configuration file that uses hexadecimal values into Elektra without writing a specification for the file. | ||
|
||
NOTE: be careful when using this option, as strings like `0xg` might produce unwanted results. (see `man 3 strtoull` for details) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reference to strtoull unnecessarily exposes implementation details. And I think we should fail in such cases, everything else seems to be too dangerous.
everything should be fixed now |
jenkins build all please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems the plugin is leaking memory. To reproduce these problems locally you can enable the CMake option ENABLE_ASAN
.
@@ -0,0 +1,10 @@ | |||
include (LibAddMacros) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please format this file with the script reformat-cmake
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for noting, we should enforce that with the buildserver. Isn't this just a matter of installing two tools?
Ideally the build server should directly tell that something like that is wrong.
@ingwinlu What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for noting, we should enforce that with the buildserver.
We already do that in the Travis Linux builds.
Isn't this just a matter of installing two tools?
Yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
knock yourself out :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sanssecours It seems you need to manually install libyaml as well as PyYAML for the reformat-cmake
script to work. This should probably be reflected in our documentation. Especially because otherwise the script throws an error for each CMake file and also deletes the contents of all these files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sanssecours It seems you need to manually install libyaml as well as PyYAML for the
reformat-cmake
script to work.
As far as I know cmake-format
has no dependencies:
pip show cmake-format
#> Name: cmake-format
#> Version: 0.3.6
#> Summary: Can format your listfiles so they don't look like crap
#> Home-page: https://github.com/cheshirekow/cmake_format
#> Author: Josh Bialkowski
#> Author-email: josh.bialkowski@gmail.com
#> License: UNKNOWN
#> Location: /Users/rene/.pyenv/versions/3.6.5/lib/python3.6/site-packages
#> Requires:
#> Required-by:
.
This should probably be reflected in our documentation. Especially because otherwise the script throws an error for each CMake file and also deletes the contents of all these files.
While reformat-cmake
is certainly not perfect, it checks for the binaries cmake-format
and sponge
beforehand. If you did not install one of these tools it should just fail printing a (broken) error message. I tried this myself, first by uninstalling cmake-format
and invoking the script. After that I uninstalled moreutils
and ran the command again. At no point did the script change any file in my copy of the repo, at least as far as I can tell. Are you sure that the script deleted you CMake files? If so, can you please describe steps to reproduce the problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure that the script deleted you CMake files?
It did not delete the files themselves, just their contents. I think this is because stdout of cmake-format
is piped back into the CMake file, but stdout is empty because of the error.
As far as I know cmake-format has no dependencies
I also looked at the cmake-format
github page and could not find any mention of the dependency. But here the module yaml
is imported, which is the cause of the error.
The problem could probably be solved by using the json
or python
format for the .cmake-format
file, because these two modules are part of the python standard library.
If you still want me to, I can try to find out how to reproduce the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the detailed description. I opened pull request #2011, which should fix the problem by checking the exit code of cmake-format CMakeLists.txt
before it updates any file.
The problem could probably be solved by using the
json
orpython
format for the.cmake-format
file, because these two modules are part of the python standard library.
That might even be a better solution, than the one I came up with. Anyway, since I prefer the human readability of YAML over JSON or the Python config style, I did not change the format of the config file.
Please add elektraArrayGetStrings to tests/icheck.suppression (It is a newly added API). I am not sure if some kind of iteration wouldn't be a better choice. Freeing the structure you return needs a loop (potential errors..). And it seems it cannot handle NULL-keys (binary Keys from which the value is a NULL pointer) because what you return needs to be NULL-terminated. And empty arrays might also be a problem? What about something like elektraKsNextArrayEntry which simply forwards to the next array entry with the internal cursor? If it can be used within the high-level API, it would be ideal. |
I decided to remove the function again and use a |
jenkins build all please |
Thank you, great job: You could store the old string of the hex number into the internal metadata, then you would not lose the capitalization of hex numbers in the configuration files. |
Purpose
Implementation of hexnumber plugin
Checklist
Check relevant points but please do not remove entries.
For docu fixes, spell checking, and similar nothing
needs to be checked.
@markus2330 please review my pull request