On (04/11/15 20:39), Jakub Hrozek wrote:
On Wed, Nov 04, 2015 at 03:22:20PM +0100, Lukas Slebodnik wrote:
> >Do you think it would be safer to initialize state->sd to -1 after the
> >state is created and only close state->sd if it's > 1? I think if the
> >socket() call would fail, then state->sd was 0 (because tevent uses
> >talloc_zero to create the state) and then we might attempt to close
> >stdin..
> The state->sd would be initialized either by socket or in
> part if HAVE_LDAP_INIT_FD is not defined to -1.
>
> It could be 0 only in case if function socket returned 0.
> which is very unlikely. The file descriptor 0 would be already
> occupied by other file. But we shoudl not rely on this.
>
> and on the other hand it's nicer to test for -1.
Thanks. I still think it would be better to initialize the socket FD to
-1 after the state is created.
static analyzers would comply that state->sd it
was set but not used.
334 req = tevent_req_create(mem_ctx, &state, struct sss_ldap_init_state);
335 if (req == NULL) {
336 DEBUG(SSSDBG_CRIT_FAILURE, "tevent_req_create failed.\n");
337 return NULL;
338 }
339
340 talloc_set_destructor((TALLOC_CTX *)state, sss_ldap_init_state_destructor);
341
342 state->ldap = NULL;
343 state->uri = uri;
344
345 #ifdef HAVE_LDAP_INIT_FD
346 struct tevent_req *subreq;
347 struct timeval tv;
348
349 state->sd = socket(addr->ss_family, SOCK_STREAM, 0);
^^^^^^^^^
//initialized here
350 if (state->sd == -1) {
^^^^^^^^^
// in case of error it will be -1
351 ret = errno;
352 DEBUG(SSSDBG_CRIT_FAILURE,
353 "socket failed [%d][%s].\n", ret, strerror(ret));
354 goto fail;
355 }
//snip
390 fail:
391 tevent_req_error(req, ret);
392 #else
393 DEBUG(SSSDBG_MINOR_FAILURE, "ldap_init_fd not available, "
394 "will use ldap_initialize with uri [%s].\n", uri);
395 state->sd = -1;
^^^^^^^^^
//initialized here to -1
396 ret = ldap_initialize(&state->ldap, uri);
397 if (ret == LDAP_SUCCESS) {
398 tevent_req_done(req);
399 } else {
400 DEBUG(SSSDBG_CRIT_FAILURE,
401 "ldap_initialize failed [%s].\n",
sss_ldap_err2string(ret));
402 if (ret == LDAP_SERVER_DOWN) {
403 tevent_req_error(req, ETIMEDOUT);
404 } else {
405 tevent_req_error(req, EIO);
406 }
407 }
408 #endif
409
410 tevent_req_post(req, ev);
But if you feel too strongly, I won't oppose. I just think if the
code
was confusing to me now it might be confusing for other developers
trying to find bugs in the future...
So do we want to add new warnig from static analyzers?
LS