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

Warning arrayization #2688

Open
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@Piankero
Copy link
Contributor

commented May 9, 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

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

@Piankero Piankero force-pushed the Piankero:warning_arrayization branch from 5615af0 to 6f00751 May 9, 2019

@Piankero Piankero force-pushed the Piankero:warning_arrayization branch 3 times, most recently from 31d2058 to ed25d46 May 9, 2019

@Piankero Piankero force-pushed the Piankero:warning_arrayization branch 2 times, most recently from d8bcc5d to 29f934e May 11, 2019

@Piankero Piankero force-pushed the Piankero:warning_arrayization branch from 29f934e to 9256d4c May 11, 2019

@Piankero Piankero force-pushed the Piankero:warning_arrayization branch from 1c1b5d9 to 7d22241 May 11, 2019

@Piankero

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2019

@sanssecours I am not able to fix the memory leaks. Do you know why it complains about a memleak in the following code?

static inline void elektraAddWarning1(Key *warningKey, const char *reason,
	const char *file, const char *line)
{
	if (!warningKey) return;

	char * nextNumber = elektraFormat("%lu", getNextArrayNumber(warningKey));
	char * buffer = elektraFormat ("warnings/#%s", nextNumber);

>>line 211	keySetMeta(warningKey, buffer, "number description module file line function reason");
	char * number = elektraFormat ("%s/%s", buffer, "number");
	char * description = elektraFormat ("%s/%s", buffer, "description");
	char * module = elektraFormat ("%s/%s", buffer, "module");
	char * fileKey = elektraFormat ("%s/%s", buffer, "file");
	char * lineKey = elektraFormat ("%s/%s", buffer, "line");
	char * mountpoint = elektraFormat ("%s/%s", buffer, "mountpoint");
	char * configfile = elektraFormat ("%s/%s", buffer, "configfile");
	char * reasonKey = elektraFormat ("%s/%s", buffer, "reason");

	keySetMeta(warningKey, "warnings", nextNumber);
	keySetMeta(warningKey, number, "1");
	keySetMeta(warningKey, description, "could not load module, dlopen failed");
	keySetMeta(warningKey, module, "dl");
	keySetMeta(warningKey, fileKey, file);
	keySetMeta(warningKey, lineKey, line);
	keySetMeta(warningKey, mountpoint, keyName(warningKey));
	keySetMeta(warningKey, configfile, keyString(warningKey));
	keySetMeta(warningKey, reasonKey, reason);
	elektraFree(nextNumber);
	elektraFree(buffer);
	elektraFree(number);
	elektraFree(description);
	elektraFree(module);
	elektraFree(fileKey);
	elektraFree(lineKey);
	elektraFree(mountpoint);
	elektraFree(configfile);
	elektraFree(reasonKey);

Valgrind:

==7738== 1,277,467 (64 direct, 1,277,403 indirect) bytes in 1 blocks are definitely lost in loss record 35 of 35
==7738==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7738==    by 0x4E422A3: ksVNew (keyset.c:238)
==7738==    by 0x4E42428: ksNew (keyset.c:220)
==7738==    by 0x4E3F6FA: keySetMeta (keymeta.c:522)
==7738==    by 0x4065D7: elektraAddWarning1.constprop.80 (kdberrors.h:211)
==7738==    by 0x401CA9: elektraTriggerWarnings (kdberrors.h:20943)
==7738==    by 0x401CA9: test_warning_array (test_warning_array.c:15)
==7738==    by 0x401CA9: main (test_warning_array.c:40)
==7738== 
@sanssecours

This comment has been minimized.

Copy link
Member

commented May 11, 2019

I am not able to fix the memory leaks. Do you know why it complains about a memleak in the following code?

Not really. I would recommend you make sure the code of the PR compiles on macOS, before you try to fix memory leaks.

@Piankero Piankero force-pushed the Piankero:warning_arrayization branch from 1c64e36 to aba8a1d May 12, 2019

@Piankero

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2019

Not really. I would recommend you make sure the code of the PR compiles on macOS, before you try to fix memory leaks.

Thank you for this hint. It is very interesting how mac and linux differ in seeing kdb_long_long_t when trying to use it in elektraFormat functions. Linux sees it as long int whereas mac sees it as long long int. When I try to cast it to an int using (int), the build server complains about the "old style cast" (because it is in a C++ file)? I cannot use static_cast<int>(..) because it is not known to C.

Do you have an idea there?

@sanssecours

This comment has been minimized.

Copy link
Member

commented May 13, 2019

Do you have an idea there?

As far as I can tell, you can use the format specifier ELEKTRA_LONG_LONG_F:

#define ELEKTRA_LONG_LONG_F "%" PRIi64

to fix this issue.

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.