Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

memory value fixed conflicts #2744

Merged
merged 29 commits into from Jun 17, 2019

Conversation

Projects
None yet
4 participants
@hesirui
Copy link
Contributor

commented Jun 3, 2019

This is the follow up pull request to #2699
there were conflicts with my local branch, which I could not resolve with rebasing or merging, thus I created a new pull request.

Basics

Check relevant points but please do not remove entries.
Do not describe the purpose of this PR in the PR description but:

  • Short descriptions should be in the release notes (added as entry in
    doc/news/_preparation_next_release.md which contains _(my name)_)
    Please always add something to the the release notes.
  • Longer descriptions should be in documentation or in design decisions.
  • Describe details of how you changed the code in commit messages
    (first line should have module: short statement syntax)
  • References to issues, e.g. close #X, should be in the commit messages.

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
  • I ran all tests locally and everything went fine
  • affected documentation is fixed
  • I added code comments, logging, and assertions (see doc/CODING.md)
  • meta data is updated (e.g. README.md of plugins and doc/METADATA.md)

Review

Remove the line below and add the "work in progress" label if you do
not want the PR to be reviewed:

@markus2330 please review my pull request

Merging

Please add the "ready to merge" label when the build server and you say
that everything is ready to be merged.

hesirui added some commits Jun 3, 2019

@hesirui hesirui referenced this pull request Jun 3, 2019

Closed

Added MemoryValue Plugin #2699

2 of 9 tasks complete

hesirui added some commits Jun 3, 2019

@hesirui

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

It seems odd, that the following error occurs in the mac build only.
../src/plugins/memoryvalue/memoryvalue.c:107:31: error: format specifies type 'unsigned long' but the argument has type 'kdb_unsigned_long_long_t' (aka 'unsigned long long') [-Werror,-Wformat]
snprintf (buf, n + 1, "%lu", normalizedMemVal);

when I change to long long %llu, the BSD build breaks.

@sanssecours

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

It seems odd, that the following error occurs in the mac build only.
../src/plugins/memoryvalue/memoryvalue.c:107:31: error: format specifies type 'unsigned long' but the argument has type 'kdb_unsigned_long_long_t' (aka 'unsigned long long') [-Werror,-Wformat]
snprintf (buf, n + 1, "%lu", normalizedMemVal);

when I change to long long %llu, the BSD build breaks.

You can fix this problem using the macro ELEKTRA_UNSIGNED_LONG_LONG_F from kdbtypes.h.

hesirui added some commits Jun 4, 2019

Merge branch 'memoryvalrebased' of https://github.com/hesirui/libelektra
 into memoryvalrebased

# Conflicts:
#	src/plugins/memoryvalue/README.md
@hesirui

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

ready to merge

@markus2330 markus2330 requested a review from kodebach Jun 5, 2019

@markus2330 markus2330 referenced this pull request Jun 5, 2019

Open

WIP: Units plugin #2614

2 of 9 tasks complete
@markus2330
Copy link
Contributor

left a comment

Looks very nice.

@@ -1211,6 +1211,13 @@ description= check if value is a valid rgbcolor in hexformat
# Old stuff inherited from filesys plugin
#

[check/memoryvalue]

This comment has been minimized.

Copy link
@markus2330

markus2330 Jun 5, 2019

Contributor

"memoryvalue" is not really a well known term. Ideally you would rename it to check/unit and also allow other units, and not only "B".

This comment has been minimized.

Copy link
@kodebach

kodebach Jun 5, 2019

Contributor

In that case this PR should be merged with #2614

This comment has been minimized.

Copy link
@markus2330

markus2330 Jun 14, 2019

Contributor

In #2614 the author stopped with CM.

So this PR could use check/unit.

This comment has been minimized.

Copy link
@hesirui

hesirui Jun 14, 2019

Author Contributor

should I rename it to, check/unit?

Show resolved Hide resolved doc/METADATA.ini Outdated
Show resolved Hide resolved src/plugins/memoryvalue/memoryvalue.c Outdated
#include <stdlib.h>


static kdb_unsigned_long_long_t is_valid_key (Key * key)

This comment has been minimized.

Copy link
@markus2330

markus2330 Jun 5, 2019

Contributor

Coding convention: isValidKey.

@kodebach
Copy link
Contributor

left a comment

The plugin itself is quite well made. However, it is very limited in functionality and it might make sense to combine this PR's efforts with #2614. But if we really want a separate smaller plugin for memory values (byte values might be a better term), we should at least also support Bits (small b) and the binary prefixes Ki, Mi, etc. These are less used for other units, but would be very useful for byte values.

Your tests do need a bit of work, as does the README. Some code comments would be nice too, although everything is very straight forward so just a few short doxygen comments would suffice, e.g. what formatFactor in elektraMemoryvalueConvertToByteString is used for.

Edit: The only major issue is the problem in kdbSet, which probably becomes obvious if you extend your tests the way I suggested.

Show resolved Hide resolved src/plugins/memoryvalue/memoryvalue.c Outdated
Show resolved Hide resolved src/plugins/memoryvalue/memoryvalue.c Outdated
Show resolved Hide resolved src/plugins/memoryvalue/memoryvalue.c Outdated
Show resolved Hide resolved src/plugins/memoryvalue/memoryvalue.c Outdated
Show resolved Hide resolved src/plugins/memoryvalue/testmod_memoryvalue.c Outdated
Show resolved Hide resolved src/plugins/memoryvalue/testmod_memoryvalue.c Outdated
Show resolved Hide resolved src/plugins/memoryvalue/testmod_memoryvalue.c Outdated
Show resolved Hide resolved src/plugins/memoryvalue/memoryvalue.c Outdated
Show resolved Hide resolved src/plugins/memoryvalue/testmod_memoryvalue.c Outdated

hesirui added some commits Jun 13, 2019

Merge branch 'master' into memoryvalrebased
# Conflicts:
#	doc/news/_preparation_next_release.md
#	src/plugins/README.md
@hesirui

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

i fixed the problems above

@hesirui

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

Unfortunately the tests break on the build server with the free BSD setup, on my local machine (Mac) all tests pass.

@markus2330 markus2330 requested a review from kodebach Jun 14, 2019


if (format == 0)
{
ELEKTRA_SET_ERRORF (171, parentKey, "%s is not formatted properly!", keyString (cur));

This comment has been minimized.

Copy link
@markus2330

markus2330 Jun 14, 2019

Contributor

The error message could be improved. Please tell how it should be formatted and which key is affected.

@markus2330
Copy link
Contributor

left a comment

Thank you for your improvements! Please make sure that the build server is happy, otherwise we cannot merge it.


char * endPtr;
// convert to long, if valid key, pointer should point to spaces or memory suffix like MB, GB
ret = ELEKTRA_UNSIGNED_LONG_LONG_S (tempval, &endPtr, 10);

This comment has been minimized.

Copy link
@markus2330

markus2330 Jun 14, 2019

Contributor

Check if this was successful? (Any number found?) Please also add a testcase with simply "B".

This comment has been minimized.

Copy link
@hesirui

hesirui Jun 14, 2019

Author Contributor

The problem with ELEKTRA_UNSIGNED_LONG_LONG_S, as I have understood it is, that the behavior is not defined in case of an error.

This comment has been minimized.

Copy link
@kodebach

kodebach Jun 14, 2019

Contributor

ELEKTRA_UNSIGNED_LONG_LONG_S is just strtoull(3) or strtoul(3), IIRC. Their behaviour in case of error is quite well defined.

This comment has been minimized.

Copy link
@hesirui

hesirui Jun 14, 2019

Author Contributor

I know, but I thought it is not if the string specified is not valid.
http://www.cplusplus.com/reference/cstdlib/strtoull/
Exceptions (C++)
No-throw guarantee: this function never throws exceptions.

If str does not point to a valid C-string, or if endptr does not point to a valid pointer object, it causes undefined behavior.

This comment has been minimized.

Copy link
@hesirui

hesirui Jun 14, 2019

Author Contributor

Or should I just expect the string to be valid but simply not contain a number?
Then I can verfiy the conversion with 0ULL


## Usage

Add the metakey `check/memoryvalue` and set a memory value e.g. 20MB to the key.

This comment has been minimized.

Copy link
@markus2330

markus2330 Jun 14, 2019

Contributor

Please improve the docu and describe how to use it (with examples, see other plugins).

*
*/

#define _XOPEN_SOURCE

This comment has been minimized.

Copy link
@markus2330

markus2330 Jun 14, 2019

Contributor

Why is this needed? Please add a comment.

#include <stdio.h>
#include <stdlib.h>

static char * deblank (char * input)

This comment has been minimized.

Copy link
@markus2330

markus2330 Jun 14, 2019

Contributor

Why are you returning the pointer if you modify the content of input itself? Please add a short docu what this function is doing.

}
}

// formatFactor is used to determine by which factor the value has to multiplied to normalize to bytes

This comment has been minimized.

Copy link
@markus2330
@kodebach
Copy link
Contributor

left a comment

Apart from the things @markus2330 noted, this looks to good me.

}
const char * value = keyString (key);
// else we manipulate the original
char * tempval = elektraStrDup (value);

This comment has been minimized.

Copy link
@kodebach

kodebach Jun 14, 2019

Contributor

You forgot to elektraFree(tempval). If you don't modify the value (i.e. deblank just skips all spaces and returns the new position in the string) you won't have to elektraStrDup at all.

@hesirui

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

I fixed the remarked problems and renamed the plugin to check/unit.

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

Thank you! Please make sure that the build server accepts it.

@hesirui

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

I really don't know why the tests always fail here with the stroull approach, with regex it worked flawlessly. Also on my machine I dont get any error.

hesirui added some commits Jun 15, 2019

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2019

@kodebach can you help here please?

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2019

These error messages seem pretty explicit to me. You initialized an unsigned integer to 0 and tried to decrement it.

hesirui added some commits Jun 15, 2019

@hesirui

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2019

finally the build server accepts it

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2019

Good to see this success! 👍

@kodebach is everything from your review now done?

@hesirui: the main Readme could still be improved, in particular with shell recorder tests (to have examples how your plugin can be used with the kdb tool). Maybe @sanssecours can help you there? As starting point you can take a look at https://master.libelektra.org/tests/shell/shell_recorder/tutorial_wrapper

@markus2330 markus2330 merged commit 52a1530 into ElektraInitiative:master Jun 17, 2019

10 of 12 checks passed

LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
LGTM analysis: C/C++ No new or fixed alerts
Details
bsd FreeBSD:freebsd-11-2-release-amd64 Task Summary
Details
bsd FreeBSD:freebsd-12-0-release-amd64 Task Summary
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
linux Task Summary
Details
mac Task Summary
Details
mac ASAN_OPTIONS:detect_leaks=1 BINDINGS:cpp ENABLE_ASAN:ON TOOLS:kdb Task Summary
Details
mac BUILD_FULL:ON BUILD_SHARED:OFF Task Summary
Details
mac KDB_DB_FILE:default.mmap KDB_DB_INIT:elektra.mmap KDB_DEFAULT_STORAGE:mmapstorage Task Summary
Details
@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2019

Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.