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

Added MemoryValue Plugin #2699

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@hesirui
Copy link
Contributor

commented May 14, 2019

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

@markus2330

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

Thank you for creating this PR!

@kodebach
Copy link
Contributor

left a comment

Good work, nice plugin.

There are a few problems with the normalization though. Also this plugin kind of covers the same use case as #2614. It might still be useful though, if only byte values are needed, because it is easier to use.

MEMORY_PETABYTE,
} MemoryFormat;

static MemoryFormat is_valid_key(Key * key, Key * parentKey){

This comment has been minimized.

Copy link
@kodebach

kodebach May 14, 2019

Contributor

If you let this function return an integer (0 for invalid, 1 for byte, 1000 for kilobyte, etc) you can simplify the call site (see below).

normalizedMemVal=ret;

//normalize to byte string
switch(format){

This comment has been minimized.

Copy link
@kodebach

kodebach May 14, 2019

Contributor

... if format was an integer (see above), you could just multiply ret by this integer here.

normalizedMemVal=ret*1000*1000;
break;
case MEMORY_GIGABYTE:
normalizedMemVal=ret*1000*1000;

This comment has been minimized.

Copy link
@kodebach

kodebach May 14, 2019

Contributor

There is a *1000 missing here (and in all cases below).

keySetMeta (key, "unprocessedvalue", str);
char *ptr;
long ret;
kdb_unsigned_long_t normalizedMemVal;

This comment has been minimized.

Copy link
@kodebach

kodebach May 14, 2019

Contributor

Use kdb_unsigned_long_long_t for 64 bit integers.

long ret;
kdb_unsigned_long_t normalizedMemVal;

ret = strtol(str, &ptr, 10);

This comment has been minimized.

Copy link
@kodebach

kodebach May 14, 2019

Contributor

Use strtoll here, also error handling is missing.

Actually it would be better to use elektraKeyToUnsignedLongLong here. But before that the conversion functions need to be separate from the highlevel API. If this is done before this PR is merged, I will add another comment.

//convert back to string
const int n = snprintf(NULL, 0, "%lu", normalizedMemVal);
char buf[n+1];
int c = snprintf(buf, n+1, "%lu", normalizedMemVal);

This comment has been minimized.

Copy link
@kodebach

kodebach May 14, 2019

Contributor

use "%llu"

static void elektraMemoryvalueConvertToByteString (Key * key, MemoryFormat format){

const char * str = keyString (key);
keySetMeta (key, "unprocessedvalue", str);

This comment has been minimized.

Copy link
@kodebach

kodebach May 14, 2019

Contributor

Store the original value in origvalue, as this metakey is specially handled to avoid some unexpected behaviour.

ret = strtol(str, &ptr, 10);
normalizedMemVal=ret;

//normalize to byte string

This comment has been minimized.

Copy link
@kodebach

kodebach May 14, 2019

Contributor

You should do an overflow check before the multiplication.

e.g. for Megabyte do:

if (ret > UINT64_MAX / (1000 * 1000))
{
      // TODO: error value to big
}

Ideally you wouldn't use UINT64_MAX, but add max/min constants to kdbtypes.h and use those.

test_memoryvalue ("10B", 10);
test_memoryvalue ("10MB", 1000000);
test_memoryvalue ("10 MB", 1000000);
test_memoryvalue ("10GB", 1000000000);

This comment has been minimized.

Copy link
@kodebach

kodebach May 14, 2019

Contributor

Please add more tests e.g. for negative values, strings that don't resemble memory values at all, digits mixed with letters etc.

Maybe also add a test in which multiple keys are tested with one kdbSet call.

@@ -0,0 +1,16 @@
**Memory Value Plugin**

This comment has been minimized.

Copy link
@kodebach
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.