{"action":"created","issue":{"url":"https://api.github.com/repos/flux-framework/flux-core/issues/1539","repository_url":"https://api.github.com/repos/flux-framework/flux-core","labels_url":"https://api.github.com/repos/flux-framework/flux-core/issues/1539/labels{/name}","comments_url":"https://api.github.com/repos/flux-framework/flux-core/issues/1539/comments","events_url":"https://api.github.com/repos/flux-framework/flux-core/issues/1539/events","html_url":"https://github.com/flux-framework/flux-core/issues/1539","id":327058035,"node_id":"MDU6SXNzdWUzMjcwNTgwMzU=","number":1539,"title":"drop errnum parameter from flux_respond()","user":{"login":"garlick","id":169947,"node_id":"MDQ6VXNlcjE2OTk0Nw==","avatar_url":"https://avatars1.githubusercontent.com/u/169947?v=4","gravatar_id":"","url":"https://api.github.com/users/garlick","html_url":"https://github.com/garlick","followers_url":"https://api.github.com/users/garlick/followers","following_url":"https://api.github.com/users/garlick/following{/other_user}","gists_url":"https://api.github.com/users/garlick/gists{/gist_id}","starred_url":"https://api.github.com/users/garlick/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/garlick/subscriptions","organizations_url":"https://api.github.com/users/garlick/orgs","repos_url":"https://api.github.com/users/garlick/repos","events_url":"https://api.github.com/users/garlick/events{/privacy}","received_events_url":"https://api.github.com/users/garlick/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":0,"created_at":"2018-05-28T15:15:29Z","updated_at":"2018-06-13T20:01:53Z","closed_at":null,"author_association":"MEMBER","body":"`flux_respond()` is used in RPC message handlers to construct a response message using the request message as a template:\r\n```c\r\nint flux_respond (flux_t *h, const flux_msg_t *request, int errnum, const char *s);\r\n```\r\nIf _errnum_ is zero, the response includes a payload built from _s_.\r\n\r\nIf _errnum_ is non-zero, the response is constructed with no payload, ignoring _s_.\r\n\r\nSome RPC handlers have been coded with the following pattern:\r\n```c\r\n  char *payload = NULL;\r\n  int rc = -1;\r\n  if (flux_request_decode (msg, ...) < 0)\r\n    goto done;\r\n  if (service_specific_function (..., &payload) < 0)\r\n    goto done;\r\n  rc = 0;\r\ndone:\r\n  if (flux_respond (h, msg, rc < 0 ? errno : 0, payload) < 0)\r\n    flux_log_error (\"flux_respond\");\r\n  free (payload);\r\n```\r\nWith the addition of `flux_respond_error()` in #1538, we can have a richer error response.  The `flux_respond_error()` call also, by its name, makes it more explicit that an error is occurring, and that whatever action was intended to be set in motion by the request ought to be halted and unwound.\r\n\r\nRPC handlers could use the following pattern\r\n```c\r\n  char *payload = NULL;\r\n  if (flux_request_decode (msg, ...) < 0\r\n    goto error;\r\n  if (service_specific_function (..., &payload) < 0)\r\n    goto error;\r\n  if (flux_respond (h, msg, 0, payload) < 0)\r\n    flux_log_error (\"flux_respond\");\r\n  free (payload);\r\n  return;\r\nerror:\r\n  if (flux_respond_error (h, msg, errno, NULL) < 0)\r\n    flux_log_error (\"flux_respond_error\");\r\n  free (payload);\r\n```\r\n\r\nI think I prefer the latter pattern even though it requires a duplication of cleanup code like the `free()` above.  My thought is that one would only make self-contained RPC handlers for very simple operations, and that more complex ones would use helpers like `service_specific_function()` above, which could minimize the amount of cleanup code that is duplicated.\r\n\r\nWhat do people think?\r\n\r\nIf there is agreement we could drop the _errnum_ parameter from `flux_respond()` so that all error responses must use `flux_respond_error()`."},"comment":{"url":"https://api.github.com/repos/flux-framework/flux-core/issues/comments/397066901","html_url":"https://github.com/flux-framework/flux-core/issues/1539#issuecomment-397066901","issue_url":"https://api.github.com/repos/flux-framework/flux-core/issues/1539","id":397066901,"node_id":"MDEyOklzc3VlQ29tbWVudDM5NzA2NjkwMQ==","user":{"login":"chu11","id":274859,"node_id":"MDQ6VXNlcjI3NDg1OQ==","avatar_url":"https://avatars0.githubusercontent.com/u/274859?v=4","gravatar_id":"","url":"https://api.github.com/users/chu11","html_url":"https://github.com/chu11","followers_url":"https://api.github.com/users/chu11/followers","following_url":"https://api.github.com/users/chu11/following{/other_user}","gists_url":"https://api.github.com/users/chu11/gists{/gist_id}","starred_url":"https://api.github.com/users/chu11/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/chu11/subscriptions","organizations_url":"https://api.github.com/users/chu11/orgs","repos_url":"https://api.github.com/users/chu11/repos","events_url":"https://api.github.com/users/chu11/events{/privacy}","received_events_url":"https://api.github.com/users/chu11/received_events","type":"User","site_admin":false},"created_at":"2018-06-13T20:01:53Z","updated_at":"2018-06-13T20:01:53Z","author_association":"CONTRIBUTOR","body":"I think it's a good idea, more so just b/c of consistency.  ```flux_respond()``` doesn't allow the user to specify an error string.  So if the caller really does not want to send an error string, it should perhaps be explicit?\r\n\r\nBut skimming code, it appears no code currently takes advantage of sending the error string.  Is this something that we would anticipate being used more often in the future?  It does seem useful for debugging.  It'd be a pain to stick in error strings everywhere right now, but perhaps its a good refactoring step."}}
{"actor":{"display_login":"chu11"}}