Augeas add lenspath to errormessage

[ghost]

Basics

These points need to be fulfilled for every PR:

• Short descriptions of your changes are in the release notes
  (added as entry in doc/news/_preparation_next_release.md which
  contains _(my name)_)
  Please always add something to the release notes.
• Details of what you changed are in commit messages
  (first line should have module: short statement syntax)
• References to issues, e.g. close #X, are in the commit messages.
• The buildservers are happy.
• The PR is rebased with current master.

If you have any troubles fulfilling these criteria, please write
about the trouble as comment in the PR. We will help you.
But we cannot accept PRs that do not fulfill the basics.

Checklist

Check relevant points but please do not remove entries.
For docu fixes, spell checking, and similar none of these points below
need to be checked.

• I added unit tests for my code
• I fully described what my PR does in the documentation
  (not in the PR description)
• I fixed all affected documentation
• I added code comments, logging, and assertions as appropriate (see Coding Guidelines)
• I updated all meta data (e.g. README.md of plugins and METADATA.ini)
• I mentioned every code not directly written by me in THIRD-PARTY-LICENSES

Review

Reviewers will usually check the following:

• Documentation is introductory, concise, good to read and describes everything what the PR does
• Examples are well chosen and understandable
• Code is conforming to our Coding Guidelines
• APIs are conforming to our Design Guidelines
• Code is consistent to our Design Decisions

Labels

If you are already Elektra developer:

• Add the "work in progress" label if you do not want the PR to be reviewed yet.
• Add the "ready to merge" label if the basics are fulfilled and you also
  say that everything is ready to be merged.

[ghost]

New message:

kdb mount /etc/security/limits.conf system/tests augeas lens=NonExistingLens
kdb ls system/tests

#> Sorry, module augeas issued the error C01200:
#> Installation: Lens not found
#> 	lensPath: NonExistingLens

[ghost]

jenkins build libelektra please

[markus2330]

Thank you, looks good to me.

[sanssecours]

The commands:

kdb mount /etc/security/limits.conf system/tests augeas lens=NonExistingLens
kdb ls system/tests

do not print an error message on my machine. Is this expected behavior?

[ghost]

On my system it actually prints an error message:

[sanssecours]

On my system it actually prints an error message:

Thank you for the fast response. Maybe I still use an old version of Elektra. Guess I need to try again with a clean build directory.

[sanssecours]

I think I found a small off-by-one error. Other than that everything looks good as far as I can tell.

[sanssecours]

I think you can also make the pointer const, if you want to.

Suggested change

	static const char * getAugeasError (augeas * augeasHandle, const char * lensPath)
	static const char * getAugeasError (augeas * augeasHandle, const char * const lensPath)

[sanssecours]

As far as I know strlen only returns the number of characters of a string.

The strlen() function returns the number of characters that precede the terminating NUL character.

I think you need an additional byte for the terminating \0 character, otherwise buffer will not be able to hold the entire error message.

Suggested change

	size_t messageSize = strlen (reason) + strlen (lensPath) + strlen (format);
	size_t messageSize = strlen (reason) + strlen (lensPath) + strlen (format) + 1;

[ghost]

I have copied this from here:

libelektra/src/plugins/augeas/augeas.c

Lines 143 to 150 in e747336

	const char * format = "%s\n\tposition: %s:%s\n\tmessage: %s\n\tlens: %s";
	size_t messageSize = (augeasError ? strlen (augeasError) : 0) + (line ? strlen (line) : 0) +
	(character ? strlen (character) : 0) + (message ? strlen (message) : 0) +
	(lens ? strlen (lens) : 0) + strlen (format);
	char * buffer = elektraMalloc (messageSize);
	snprintf (buffer, messageSize, format, augeasError ? augeasError : "", line ? line : "", character ? character : "",
	message ? message : "", lens ? lens : "");
	reason = buffer;

This bug should then also appear there

[sanssecours]

This bug should then also appear there

I think you are right. The off-by-one error is also one of the reasons why elektraStrLen also counts the terminating NULL character.

libelektra/src/libs/elektra/internal.c

Lines 369 to 371 in f3460ca

	* This function differs from strlen() because it is Unicode and multibyte
	* chars safe. While strlen() counts characters and ignores the final NULL,
	* elektraStrLen() count bytes including the ending NULL.

[markus2330]

@kodebach can you take a look here?

[kodebach]

What exactly should I look at?

@sanssecours is right in general, but actually the + 1 is not needed here. The string format is a printf-string in the resulting buffer the %s will not be present, but they are counted by strlen. So actually the allocated buffer is 3 bytes bigger than it needs to be.

I didn't check the second one. It might be more problematic, because in the NULL cases, strlen is replaced by 0, but the NULL string is replaced by " ". I think it still works, but adding 1 to the buffer size certainly doesn't hurt.

PS If someone is working on augeas, there are code paths where it sets an error, but continues to execute and in the end returns SUCCESS.

[sanssecours]

The string format is a printf-string in the resulting buffer the %s will not be present, but they are counted by strlen.

I did not think about that 😊. Thank you for the detailed explanation.

[sanssecours]

Maybe I still use an old version of Elektra. Guess I need to try again with a clean build directory.

I still can not reproduce the error message. Neither on macOS nor on Ubuntu Bionic the commands:

kdb mount /etc/security/limits.conf system/tests augeas lens=NonExistingLens
kdb ls system/tests

print an error message. I also tried to reproduce the error message unsuccessfully on Cirrus using FreeBSD. Of course there is still the possibility that I did something wrong. As far as I can tell the branch I used for testing should include the updates of this PR though.

[ghost]

I just pulled from origin/master, cleared the build directory, freshly installed and tried it with success.

does the behavior change when you add sudo to the commands?

[markus2330]

@sanssecours I think you have a different problem than this PR tries to solve. This PR is about adding additional information about an (in this PR unchanged) error from Augeas.

Of course it would be nice to have shell recorder tests for the augeas plugin but I think other tasks have higher priority.

For me it works:

kdb mount /etc/security/limits.conf system/tests augeas lens=NonExistingLens
kdb ls system/tests
#> Sorry, module augeas issued the error C01200:
#> Installation: Lens not found
#>         lensPath: NonExistingLens
#>

I agree with @sanssecours though that these arbitrary whitespaces everywhere only create problems. It is also inconsistent to other errors.

[sanssecours]

@sanssecours I think you have a different problem than this PR tries to solve.

That might be the case. As I write below I was finally able to reproduce the error using Docker. For my usual build configuration on macOS, the commands still do not produce an error message though. Anyway, from my point of view, this PR does what it is supposed to do 👍.

does the behavior change when you add sudo to the commands?

Nope. The commands:

sudo kdb mount /etc/security/limits.conf system/tests augeas lens=NonExistingLens
sudo kdb ls system/tests

do not print an error message. There might be some differences between my system and your system that influence the behavior. For example:

• the file /etc/security/limits.conf does not exist on my system,
• I changed KDB_DB_SPEC, KDB_DB_HOME and KDB_DB_SYSTEM to user writeable subdirectories of my build directory,
• usually my Elektra configuration is empty.

Anyway, after some time I was finally able to reproduce the error message using the steps below.

1. Switch to the master branch

   git checkout master

2. Pull the latest changes

   git pull
   git log --pretty=oneline --since=1.day
   #> f3460ca7bf4371fbde2eb358e5217c9c658c8314 (HEAD -> master, elektra/master) Merge pull request #3286 from Piankero/augeas-add-lenspath-to-errormessage
   #> f03bb14dc736f80107b0deebb3383c617b27ab50 news: add notes about finished thesis
   #> c88292f23f5f9cbd7be25ab13a36e51b3307c55c Merge pull request #3288 from ElektraInitiative/release-notes

3. Build the Dockerfile for Ubuntu Bionic

   # fish syntax!
   docker build -t elektra-bionic \
     --build-arg JENKINS_USERID=(id -u) \
     --build-arg JENKINS_GROUPID=(id -g) \
     -f scripts/docker/ubuntu/bionic/Dockerfile \
     scripts/docker/ubuntu/bionic

4. Run the Docker image

   docker run \
           -it --rm --cap-add SYS_PTRACE \
           -v "$PWD:/home/jenkins/workspace" \
           -w /home/jenkins/workspace \
           elektra-bionic

5. Build Elektra

   mkdir build
   cd build
   mkdir system
   cmake -GNinja -DKDB_DB_SYSTEM="$PWD/system" ..
   ninja

6. Extend PATH and LD_LIBRARY_PATH

   export PATH="$PWD/bin:$PATH"
   export LD_LIBRARY_PATH="$PWD/lib"  

7. Execute kdb commands

   kdb mount /etc/security/limits.conf system/tests augeas lens=NonExistingLens
   kdb ls system/tests