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

Flux request decode raw fix #1144

Merged
merged 4 commits into from Aug 10, 2017

Conversation

Projects
None yet
6 participants
@morrone
Copy link
Contributor

morrone commented Aug 8, 2017

No description provided.

morrone added some commits Aug 8, 2017

libflux/request: Fix "data" type flux_request_decode_raw()
Fix the type of the "data" parameter in the flux_request_decode_raw().
In all uses, "data" is a the address of a pointer, so "void **data" is
the correct type.  This makes it fairly clear that the buffer will be
provided by flux_request_decode_raw() rather than by the caller.
man3: Fix prototype of flux_request_encode_raw()
Fix the documentation of the "len" paramter in the flux_request_encode_raw()
function.  It is an int, not a pointer.

Also expand the explanation a little bit.

@morrone morrone requested review from chu11 and garlick Aug 8, 2017

@@ -16,7 +16,7 @@ SYNOPSIS
const char *json_str);

flux_msg_t *flux_request_encode_raw (const char *topic,
void *data, int *len);
void *data, int len);

This comment has been minimized.

@chu11

chu11 Aug 8, 2017

Contributor

I don't think you meant to change flux_request_encode_raw?

This comment has been minimized.

@chu11

chu11 Aug 8, 2017

Contributor

oh whoops, nevermind. This is a typo fix, not a change related to the other stuff.

This comment has been minimized.

@morrone

morrone Aug 8, 2017

Author Contributor

Yeah, just along for the ride, sorry. :)

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Aug 8, 2017

LGTM, nice catch.

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Aug 8, 2017

I brought up this issue with @garlick awhile back. Looking through old e-mails we never came to a conclusion.

I believe it was initially programmed this way to avoid void ** casting. But I think this is overall more correct/less confusing and worth the void ** casting. But I'll let others chime in.

@@ -22,7 +22,7 @@ SYNOPSIS

int flux_request_decode_raw (const flux_msg_t *msg,
const char **topic,
void *data, int *len);
void **data, int *len);

This comment has been minimized.

@garlick

garlick Aug 8, 2017

Member

I may be so very wrong about wanting to do this, but it was quite on purpose, and the reason the result parameter is defined as a void * instead of a void ** is so that the user doesn't have to do something like

const char *data;
int len;

flux_request_decode_raw (msg, NULL, (void **)&data, &len);

in virtually every place this function is used.

This comment has been minimized.

@garlick

garlick Aug 8, 2017

Member

I should temper that comment with the observation that now it doesn't seem all that wonderful an idea. What do others think?

This comment has been minimized.

@chu11

chu11 Aug 8, 2017

Contributor

For me, when I first looked at:

int flux_request_decode_raw (const flux_msg_t *msg, const char **topic,
                             void *data, int *len);

and compared it to

flux_msg_t *flux_request_encode_raw (const char *topic,
                                     const void *data, int len);

My thought was "how come topic & len are in & out parameters, but not data?" You would think it should be a void **.

This comment has been minimized.

@grondo

grondo Aug 8, 2017

Contributor

I think the cast to (void **) is not so bad for increased clarity in the prototype.

If it becomes too onerous a user can create a wrapper function so at most we'd see the case once per file/module.

This comment has been minimized.

@garlick

garlick Aug 8, 2017

Member

OK, sounds like @grondo already acked so let's fix this. Want to go ahead and also fix:

int flux_msg_get_payload (const flux_msg_t *msg, int *flags,
                          void *buf, int *size);
int flux_mrpc_get_raw (flux_mrpc_t *mrpc, void *data, int *len);
int flux_rpc_get_raw (flux_future_t *f, void *data, int *len);
int flux_response_decode_raw (const flux_msg_t *msg, const char **topic,
                              void *data, int *len);

for consistency?

This comment has been minimized.

@grondo

grondo Aug 8, 2017

Contributor

👍 forgot about those.

This comment has been minimized.

@morrone

morrone Aug 8, 2017

Author Contributor

Sure, I can work on those others.

This comment has been minimized.

@garlick

garlick Aug 8, 2017

Member

@chu11: just changed prototypes for treeobj_decode_val() and flux_kvs_lookup_get_raw() in the branch we're working on and pushed.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Aug 8, 2017

Some checks in Travis failed and I'm restarting them. I saw another case of "write error" during the distcheck packaging phase, and a hang due to failure in the faketime tests. All known issues obviously not related to this PR.

garlick added a commit to garlick/flux-core that referenced this pull request Aug 8, 2017

libkvs/treeobj: treeobj_decode_val use void **
As discussed in flux-framework#1144, function result parameters like
the one in treeobj_decode_val() should probably be
void ** not void *.

garlick added a commit to garlick/flux-core that referenced this pull request Aug 8, 2017

libkvs/lookup: lookup_get_raw use use void **
As discussed in flux-framework#1144, function result parameters like
the one in flux_kvs_lookup_get_raw() should probably be
void ** not void *.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 8, 2017

Coverage Status

Coverage decreased (-0.02%) to 77.963% when pulling 20b505f on morrone:flux_request_decode_raw_fix into 0abcd67 on flux-framework:master.

libflux/message: Change void * to void ** in flux_msg_get_payload
The function want to return a pointer to a buffer in one of its
parameters, so that parameter should be a void ** rather than a void *.
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 8, 2017

Codecov Report

Merging #1144 into master will increase coverage by 0.02%.
The diff coverage is 92.85%.

@@            Coverage Diff             @@
##           master    #1144      +/-   ##
==========================================
+ Coverage   77.61%   77.63%   +0.02%     
==========================================
  Files         157      157              
  Lines       28679    28680       +1     
==========================================
+ Hits        22260    22267       +7     
+ Misses       6419     6413       -6
Impacted Files Coverage Δ
src/common/libflux/mrpc.c 86.32% <0%> (ø) ⬆️
src/modules/kvs/kvs.c 63.45% <100%> (+0.78%) ⬆️
src/cmd/builtin/content.c 73.87% <100%> (ø) ⬆️
src/common/libflux/content.c 86.66% <100%> (ø) ⬆️
src/broker/content-cache.c 73.27% <100%> (-1.3%) ⬇️
src/common/libflux/request.c 89.74% <100%> (+1.28%) ⬆️
src/broker/log.c 69.64% <100%> (ø) ⬆️
src/common/libflux/response.c 83.73% <100%> (ø) ⬆️
src/common/libflux/rpc.c 94.21% <100%> (+0.82%) ⬆️
src/common/libflux/message.c 81.76% <100%> (+0.58%) ⬆️
... and 16 more
@morrone

This comment has been minimized.

Copy link
Contributor Author

morrone commented Aug 8, 2017

OK, I added a couple of commits to this pull requires to address these five functions:

flux_msg_get_payload
flux_rpc_get_raw
flux_response_decode_raw
flux_mrpc_get_raw
flux_content_load_get

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 8, 2017

Coverage Status

Coverage decreased (-0.04%) to 77.945% when pulling 3e8cb3b on morrone:flux_request_decode_raw_fix into 0abcd67 on flux-framework:master.

@@ -0,0 +1,228 @@
#! /bin/bash

This comment has been minimized.

@chu11

chu11 Aug 8, 2017

Contributor

Did you accidentally git add this file?

This comment has been minimized.

@morrone

morrone Aug 9, 2017

Author Contributor

Doh. Fixed.

libflux: Convert void * to void ** in flux_rpc_get_raw and friends
The functions want to return a pointer to a buffer in one of its
parameters, so that parameter should be a void ** rather than a void *.

The following functions are converted:

flux_rpc_get_raw
flux_response_decode_raw
flux_content_load_get
flux_mrpc_get_raw
@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 9, 2017

Coverage Status

Coverage decreased (-0.05%) to 77.931% when pulling 45f5341 on morrone:flux_request_decode_raw_fix into 0abcd67 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Aug 10, 2017

restarted travis since it failed to update status of the PR and GitHub wouldn't let me press the button.

@grondo grondo merged commit 1665912 into flux-framework:master Aug 10, 2017

4 checks passed

codecov/patch 92.85% of diff hit (target 77.61%)
Details
codecov/project 77.63% (+0.02%) compared to 0abcd67
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.05%) to 77.931%
Details

@morrone morrone deleted the morrone:flux_request_decode_raw_fix branch Aug 10, 2017

chu11 added a commit to chu11/flux-core that referenced this pull request Aug 15, 2017

libkvs/treeobj: treeobj_decode_val use void **
As discussed in flux-framework#1144, function result parameters like
the one in treeobj_decode_val() should probably be
void ** not void *.

chu11 added a commit to chu11/flux-core that referenced this pull request Aug 15, 2017

libkvs/lookup: lookup_get_raw use use void **
As discussed in flux-framework#1144, function result parameters like
the one in flux_kvs_lookup_get_raw() should probably be
void ** not void *.

@grondo grondo referenced this pull request Aug 23, 2017

Closed

0.8.0 Release #1160

chu11 added a commit to chu11/flux-core that referenced this pull request Jul 27, 2018

libflux/future: convert void * to const void **
In flux_future_get, convert void * parameter to void ** parameter,
consistent to fixes done in flux-framework#1144 and flux-framework#1212.

Fixes flux-framework#1602

chu11 added a commit to chu11/flux-core that referenced this pull request Jul 27, 2018

libflux/future: convert void * to const void **
In flux_future_get, convert void * parameter to void ** parameter,
consistent to fixes done in flux-framework#1144 and flux-framework#1212.

Fixes flux-framework#1602
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.