Please use "error and out" logic (Re: [PATCH] Avoid privileged segfault in GetNCChanges and add many more tests)

[Stefan Metzmacher]

Hi Andrew,

> -	sid = pytalloc_get_ptr(py_sid);
> +	if (PyObject_TypeCheck(py_sid, &dom_sid_Type)) {
> +		sid = pytalloc_get_ptr(py_sid);
> +	} else {
> +		PyErr_SetString(PyExc_TypeError,
> +				"expected security.dom_sid "
> +				"for second argument to .from_sddl");
> +		return NULL;
> +
> +	}
>  
>  	secdesc = sddl_decode(NULL, sddl, sid);
>  	if (secdesc == NULL) {
> -- 

Can we please agree on using an "error and out logic" for new code?
There's really no good reason to have the additional indentation
for the "sid = pytalloc_get_ptr(py_sid);" line!
I thought this was part of README.Coding for a long time already,
but I can't find it...

The above example seems harmless, but I notice new examples
almost every time I review patches from you or once you reviewed.
These act as bad examples others will likely copy'n'paste or
at least learn from.

This morning I also noticed

+	if (ret == LDB_SUCCESS) {
+		if (result->count == 0) {
+			ret = LDB_ERR_NO_SUCH_OBJECT;
+		} else {
+			struct ldb_message *match =
+				get_best_match(dn, result);
+			if (match == NULL) {
+				TALLOC_FREE(frame);
+				return LDB_ERR_OPERATIONS_ERROR;
+			}
+			*msg = talloc_move(mem_ctx, &match);
+		}
+	}
=> here
+	TALLOC_FREE(frame);
+	return ret;

Which adds a lot of useless indentation and it makes it really
hard to add new code where I added "here", as you need remember
and understand all the logic be had above or protect
the new code by using another "if (ret == LDB_SUCCESS) {"

It should be:

	if (ret != LDB_SUCCESS) {
		TALLOC_FREE(frame);
		return ret;
	}

	if (result->count == 0) {
		TALLOC_FREE(frame);
		return LDB_ERR_NO_SUCH_OBJECT;
	}

	match = get_best_match(dn, result);
	if (match == NULL) {
		TALLOC_FREE(frame);
		return LDB_ERR_OPERATIONS_ERROR;
	}

	*msg = talloc_move(mem_ctx, &match);
	TALLOC_FREE(frame);
	return LDB_SUCCESS; <= This is important to be not "return ret;"

Untangling these kind of spaghetti was the major pain I had when
working on spnego.c, it took days in order to understand the logic
of some functions.

I know we have to live with our existing code for a long time,
but we should really try hard to avoid it for new code.

I started to write such a mail already a few weeks ago,
but discarded it in order to enjoy my vacation...

(I hope this mail is not too harsh, but reviewing this of stuff
ruins my day)

Thanks and sorry...
metze

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20170809/eef36ee3/signature.sig>