Hi Noel, thanks for fixing clang warnings, but I fear the following changes are wrong.
Can we have SMB_ASSERT(hdr != NULL); after the if statements with a
comment explaining why it's there.
> diff --git a/source3/lib/ctdbd_conn.c b/source3/lib/ctdbd_conn.c
> index d5fac572d3c..8a8fbec4552 100644
> --- a/source3/lib/ctdbd_conn.c
> +++ b/source3/lib/ctdbd_conn.c
> @@ -398,15 +398,16 @@ static int ctdb_read_packet(int fd, int timeout,
> TALLOC_CTX *mem_ctx,
> static int ctdb_read_req(struct ctdbd_connection *conn, uint32_t reqid,
> TALLOC_CTX *mem_ctx, struct ctdb_req_header **result)
> {
> - struct ctdb_req_header *hdr;
> + struct ctdb_req_header *hdr = NULL;
> int ret;
>
> next_pkt:
>
> ret = ctdb_read_packet(conn->fd, conn->timeout, mem_ctx, &hdr);
> - if (ret != 0) {
> + if (hdr == NULL || ret != 0) {
> DBG_ERR("ctdb_read_packet failed: %s\n", strerror(ret));
> cluster_fatal("failed to read data from ctdbd\n");
> + return -1;
> }
This is not strictly a bug, but it's makes the code really unclear.
> DEBUG(11, ("Received ctdb packet\n"));
> @@ -1038,7 +1039,7 @@ int ctdbd_traverse(struct ctdbd_connection *conn,
> uint32_t db_id,
> int ret;
> TDB_DATA key, data;
> struct ctdb_traverse_start t;
> - int32_t cstatus;
> + int32_t cstatus = 0;
>
> if (ctdbd_conn_has_async_reqs(conn)) {
> /*
> @@ -1945,7 +1946,7 @@ static void ctdbd_parse_done(struct tevent_req *subreq)
>
> ret = ctdb_pkt_recv_recv(subreq, state, &hdr);
> TALLOC_FREE(subreq);
> - if (tevent_req_error(req, ret)) {
> + if ((hdr == NULL) || tevent_req_error(req, ret)) {
> DBG_ERR("ctdb_pkt_recv_recv returned %s\n", strerror(ret));
> return;
> }
This is actually really a bug, in case ctdb_pkt_recv_recv fails
we'll never run tevent_req_error() and the callers callback function,
so it will just hang as hdr is always NULL if ret is not 0.
Can you revert this and add SMB_ASSERT().
Thanks!
metze
signature.asc
Description: OpenPGP digital signature
