URL: https://github.com/SSSD/sssd/pull/197
Title: #197: KCM responder
mzidek-rh commented:
"""
Some nitpicks mostly regarding the lack of DEBUG messages.
src/responder/kcm/kcmsrv_cmd.c
164 /* First 16 bits are 8 bit major and 8bit major protocol version */
^^^^^
Should be minor
-----------------------------------
src/responder/kcm/kcmsrv_cmd.c - this module could use more debug messages.
First because it is tevent code and second because the debug messages server as
"natural" comments. For example:
311 static void kcm_send_reply(struct cli_ctx *cctx,
312 struct kcm_op_io *op_io,
313 struct kcm_repbuf *repbuf)
314 {
315 errno_t ret;
316
317 ret = kcm_output_construct(op_io, repbuf);
318 if (ret != EOK) {
It would be good to print DEBUG message here with the error code.
319 kcm_reply_error(cctx, ret, repbuf);
320 return;
321 }
-----------------------------------
This function:
509 static int kcm_send_data(struct cli_ctx *cctx)
Could have a debug a message after each kcm_write_iovec operation saying what
it failed to write.
-----------------------------------
369 static void kcm_cmd_request_done(struct tevent_req *req)
389 static errno_t kcm_recv_data(int fd, struct kcm_reqbuf *reqbuf)
IMO the above functions should also print DEBUG messages on errors.
-----------------------------------
The function
bool kcm_cc_access(struct kcm_ccache *cc
Should IMO include TRACE_FUNC level debug messages at least for all cases when
the result is false. The root cause is lost in the above calling functions and
it ends up later in some async code.
-----------------------------------
1166 static errno_t sec_list_recv(struct tevent_req *req,
1167 TALLOC_CTX *mem_ctx,
1168 const char ***_sec_key_list,
1169 size_t *_sec_key_list_len)
1170
1171 {
1172 struct sec_list_state *state = tevent_req_data(req,
1173 struct sec_list_state);
1174
1175 TEVENT_REQ_RETURN_ON_ERROR(req);
1176
1177 if (_sec_key_list != NULL) {
1178 *_sec_key_list = talloc_steal(mem_ctx, state->sec_key_list);
1179 }
1180 if (_sec_key_list_len) {
^^^^^^ use != NULL here too
1181 *_sec_key_list_len = state->sec_key_list_len;
1182 }
1183 return EOK;
1184 }
-----------------------------------
1247 static void sec_get_done(struct tevent_req *subreq)
1248 {
1249 errno_t ret;
1250 struct tevent_req *req = tevent_req_callback_data(subreq, struct
tevent_req);
1251 struct sec_get_state *state = tevent_req_data(req,
1252 struct sec_get_state);
1253 struct sss_iobuf *outbuf;
1254 const char *sec_value;
1255 int http_code;
1256
1257 ret = tcurl_http_recv(state, subreq, &http_code, &outbuf);
1258 talloc_zfree(subreq);
1259 if (ret != EOK) {
1260 DEBUG(SSSDBG_OP_FAILURE,
1261 "GET HTTP request failed [%d]: %s\n", ret,
sss_strerror(ret));
1262 tevent_req_error(req, ret);
1263 return;
1264 }
1265
1266 if (http_code != 200) {
1267 ret = http2errno(http_code);
Add DEBUG message here
1268 tevent_req_error(req, ret);
1269 return;
1270 }
"""
See the full comment at
https://github.com/SSSD/sssd/pull/197#issuecomment-288409105
_______________________________________________
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
