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

2016 04 stze result api call #57

Merged
merged 24 commits into from
Oct 5, 2016
Merged

2016 04 stze result api call #57

merged 24 commits into from
Oct 5, 2016

Conversation

stze
Copy link

@stze stze commented Aug 26, 2016

This PR introduces the result api call. You can test this by using 2016-05-bontric-implement-result-call python client branch.

  • Fixed handle run callid bug
  • Implemented result api call + functional test
  • Fixed coverage
  • Introduce travis gcc build (used for coverage)

@stze
Copy link
Author

stze commented Aug 27, 2016

@mkind i'll hand over this PR and branch to you. If you'll find some problems, would be great if you could fix them, otherwise please merge into master.

Copy link
Author

@stze stze left a 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 @mkind. Only a few comments.

@@ -327,13 +329,13 @@ endif()

if(CLANG_ADDRESS_SANITIZER)
set_property(TARGET sb APPEND_STRING PROPERTY COMPILE_FLAGS "-DEXITFREE ")
set_property(TARGET sb APPEND_STRING PROPERTY COMPILE_FLAGS "-fno-sanitize-recover -fno-omit-frame-pointer -fno-optimize-sibling-calls -fsanitize=address -fsanitize=undefined ")
set_property(TARGET sb APPEND_STRING PROPERTY COMPILE_FLAGS "-fno-sanitize-recover=undefined,integer -fno-omit-frame-pointer -fno-optimize-sibling-calls -fsanitize=address -fsanitize=undefined ")
Copy link
Author

@stze stze Sep 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change seems reasonable, but it reverts an old commit where this option was disabled. travis clang is old and doesn't support undefined,integer yet.

@@ -68,9 +68,12 @@ int api_result(char *targetpluginkey, uint64_t callid,
cinfo = connection_send_request(targetpluginkey, result, result_params,
api_error);

if (true == api_error->isset)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not:

if(api_error->isset) 

and

if(!api_error->isset) 

instead of

if(true == api_error->isset) 

and

if(false == api_error->isset) 

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no problem with your proposal

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed this in 0e6e4bb.

if (!p || !f)
LOG_ERROR("[test] Failed to alloc mem for example plugin.\n");

p->key = cstring_copy_string("0123456789ABCDEF");
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be nice to allocate all these strings on the stack. maybe we can change it to:

p->key = (string) {.str = "0123456789ABCDEF", .length = sizeof("0123456789ABCDEF") - 1}};

then we can remove the free_string calls in the helper_free_plugin function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be a general approach to save some heap allocs in other contexts too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in a30446b

@stze stze merged commit 83ecd4d into master Oct 5, 2016
@mkind mkind deleted the 2016-04-stze-result-api-call branch December 5, 2018 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants