[PATCH] Minor cleanup of libnet_LookupName_recv

[Christof Schmitt]

On Fri, Jan 26, 2018 at 01:38:14PM +0100, Swen Schillig via samba-technical wrote:
> Hi
> 
> Another minor cleanup.
> 
> Please review.
> 
> Thanks.
> 
> Cheers Swen.

> From 04c66afd2fa8a38877ffac0e2774f91e59607003 Mon Sep 17 00:00:00 2001
> From: Swen Schillig <swen at vnet.ibm.com>
> Date: Fri, 26 Jan 2018 13:28:58 +0100
> Subject: [PATCH] Minor cleanup of libnet_LookupName_recv
> 
> Zero libnet_LookupName out struct before setting results,
> preventing false result interpretation.
> Reduce indentation level.
> Comply with 80 column rule.
> 
> Signed-off-by: Swen Schillig <swen at vnet.ibm.com>

Would it make sense to split this in two patches? One for the zero
initialization, and the other one for the logic change to avoid an
indentation level?

> ---
>  source4/libnet/libnet_lookup.c | 59 ++++++++++++++++++++++--------------------
>  1 file changed, 31 insertions(+), 28 deletions(-)
> 
> diff --git a/source4/libnet/libnet_lookup.c b/source4/libnet/libnet_lookup.c
> index bdb99950493..3b91379087f 100644
> --- a/source4/libnet/libnet_lookup.c
> +++ b/source4/libnet/libnet_lookup.c
> @@ -383,43 +383,46 @@ NTSTATUS libnet_LookupName_recv(struct composite_context *c, TALLOC_CTX *mem_ctx
>  	NTSTATUS status;
>  	struct lookup_name_state *s;
>  
> +	memset(&io->out, 0, sizeof(io->out));
> +

Samba has the ZERO_STRUCT macro, that should be preferred over a direct
call to memset.

>  	status = composite_wait(c);
>  
> -	if (NT_STATUS_IS_OK(status)) {
> -		s = talloc_get_type(c->private_data, struct lookup_name_state);
> +	if (!NT_STATUS_IS_OK(status)) {
> +		io->out.error_string = talloc_asprintf(mem_ctx, "Error: %s",
> +						       nt_errstr(status));
> +		goto done;
> +	}
>  
> -		io->out.rid = 0;
> -		io->out.sid = NULL;
> -		io->out.sidstr = NULL;
> +	s = talloc_get_type(c->private_data, struct lookup_name_state);
>  
> -		if (*s->lookup.out.count > 0) {
> -			struct lsa_RefDomainList *domains = *s->lookup.out.domains;
> -			struct lsa_TransSidArray *sids = s->lookup.out.sids;
> +	if (*s->lookup.out.count > 0) {
> +		struct lsa_RefDomainList *domains = *s->lookup.out.domains;
> +		struct lsa_TransSidArray *sids = s->lookup.out.sids;
>  
> -			if (domains == NULL || sids == NULL) {
> -				status = NT_STATUS_UNSUCCESSFUL;
> -				io->out.error_string = talloc_asprintf(mem_ctx, "Error: %s", nt_errstr(status));
> -				goto done;
> -			}
> +		if (domains == NULL || sids == NULL) {
> +			status = NT_STATUS_UNSUCCESSFUL;
> +			io->out.error_string =
> +				talloc_asprintf(mem_ctx, "Error: %s",
> +						nt_errstr(status));
> +			goto done;
> +		}
>  
> -			if (sids->count > 0) {
> -				io->out.rid        = sids->sids[0].rid;
> -				io->out.sid_type   = sids->sids[0].sid_type;
> -				if (domains->count > 0) {
> -					io->out.sid = dom_sid_add_rid(mem_ctx, domains->domains[0].sid, io->out.rid);
> -					NT_STATUS_HAVE_NO_MEMORY(io->out.sid);
> -					io->out.sidstr = dom_sid_string(mem_ctx, io->out.sid);
> -					NT_STATUS_HAVE_NO_MEMORY(io->out.sidstr);

libcli/util/ntstatus.h
has a comment that these macros should also be avoided. Maybe that could
become a third patch to replace these with an explicit if() and "goto
done" in the error case. That would actually fix a memory leak in the
error path, as currently talloc_free(c) is missing for that case.

> -				}
> +		if (sids->count > 0) {
> +			io->out.rid        = sids->sids[0].rid;
> +			io->out.sid_type   = sids->sids[0].sid_type;
> +			if (domains->count > 0) {
> +				io->out.sid =
> +					dom_sid_add_rid(mem_ctx,
> +							domains->domains[0].sid,
> +							io->out.rid);
> +				NT_STATUS_HAVE_NO_MEMORY(io->out.sid);
> +				io->out.sidstr = dom_sid_string(mem_ctx,
> +								io->out.sid);
> +				NT_STATUS_HAVE_NO_MEMORY(io->out.sidstr);
>  			}
>  		}
> -
> -		io->out.error_string = talloc_strdup(mem_ctx, "Success");
> -
> -	} else if (!NT_STATUS_IS_OK(status)) {
> -		io->out.error_string = talloc_asprintf(mem_ctx, "Error: %s", nt_errstr(status));
>  	}
> -
> +	io->out.error_string = talloc_strdup(mem_ctx, "Success");
>  done:
>  	talloc_free(c);
>  	return status;
> -- 
> 2.14.3
> 

Christof