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.

Sign up for GitHub

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

Jump to bottom

Flux request decode raw fix #1144

Merged
merged 4 commits into from Aug 10, 2017
Merged

Flux request decode raw fix #1144

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1ee56fd
libflux/request: Fix "data" type flux_request_decode_raw()
morrone Aug 8, 2017
20b505f
man3: Fix prototype of flux_request_encode_raw()
morrone Aug 8, 2017
c360963
libflux/message: Change void * to void ** in flux_msg_get_payload
morrone Aug 8, 2017
45f5341
libflux: Convert void * to void ** in flux_rpc_get_raw and friends
morrone Aug 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion doc/man3/flux_request_decode.adoc
View file
Open in desktop
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@chu11 chu11 Aug 8, 2017 •
edited

Choose a reason for hiding this comment

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

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 **.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 forgot about those.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can work on those others.

Copy link
Member

Choose a reason for hiding this comment

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

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


DESCRIPTION
-----------
Expand Down
3 changes: 2 additions & 1 deletion src/broker/content-cache.c
View file
Open in desktop
Expand Up @@ -359,7 +359,8 @@ void content_load_request (flux_t *h, flux_msg_handler_t *w,
int saved_errno = 0;
int rc = -1;

if (flux_request_decode_raw (msg, NULL, &blobref, &blobref_size) < 0) {
if (flux_request_decode_raw (msg, NULL, (void **)&blobref,
&blobref_size) < 0) {
saved_errno = errno;
goto done;
}
Expand Down
2 changes: 1 addition & 1 deletion src/broker/log.c
View file
Open in desktop
Expand Up @@ -532,7 +532,7 @@ static void append_request_cb (flux_t *h, flux_msg_handler_t *w,
log_msg ("%s: malformed log request", __FUNCTION__);
return;
}
if (flux_request_decode_raw (msg, NULL, &buf, &len) < 0)
if (flux_request_decode_raw (msg, NULL, (void **)&buf, &len) < 0)
goto error;
if (logbuf_append (logbuf, buf, len) < 0)
goto error;
Expand Down
4 changes: 2 additions & 2 deletions src/common/libflux/request.c
View file
Open in desktop
Expand Up @@ -75,7 +75,7 @@ int flux_request_decode (const flux_msg_t *msg, const char **topic,
}

int flux_request_decode_raw (const flux_msg_t *msg, const char **topic,
void *data, int *len)
void **data, int *len)
{
const char *ts;
void *d = NULL;
Expand All @@ -95,7 +95,7 @@ int flux_request_decode_raw (const flux_msg_t *msg, const char **topic,
}
if (topic)
*topic = ts;
*(void **)data = d;
*data = d;
*len = l;
rc = 0;
done:
Expand Down
2 changes: 1 addition & 1 deletion src/common/libflux/request.h
View file
Open in desktop
Expand Up @@ -28,7 +28,7 @@ int flux_request_unpack (const flux_msg_t *msg, const char **topic,
* Returns 0 on success, or -1 on failure with errno set.
*/
int flux_request_decode_raw (const flux_msg_t *msg, const char **topic,
void *data, int *len);
void **data, int *len);

/* Encode a request message.
* If json_str is non-NULL, assign the json payload.
Expand Down
3 changes: 2 additions & 1 deletion src/common/libflux/test/request.c
View file
Open in desktop
Expand Up @@ -11,7 +11,8 @@ int main (int argc, char *argv[])
flux_msg_t *msg;
const char *topic, *s;
const char *json_str = "{\"a\":42}";
char *d, data[] = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";
void *d;
char data[] = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";
int i, l, len = strlen (data);

plan (NO_PLAN);
Expand Down
3 changes: 2 additions & 1 deletion src/modules/content-sqlite/content-sqlite.c
View file
Open in desktop
Expand Up @@ -255,7 +255,8 @@ void load_cb (flux_t *h, flux_msg_handler_t *w,
int uncompressed_size;
int rc = -1;

if (flux_request_decode_raw (msg, NULL, &blobref, &blobref_size) < 0) {
if (flux_request_decode_raw (msg, NULL, (void **)&blobref,
&blobref_size) < 0) {
flux_log_error (h, "load: request decode failed");
goto done;
}
Expand Down