[PATCH 2/2] librpc/ndr: add new LIBNDR_FLAG_STR_RAW8 for ndr_pull_string

[Andrew Bartlett]

On Thu, 2011-05-26 at 15:45 +0200, Sean Finney wrote:
> This commit also includes some (basic) tests of each of the flags'
> respective behaviors with the ndr push/pull string functions, in a new
> source4 torture test suite ndr.ndr_string.

I've made a few comments, particularly because you look like the kind of
bloke who will keep producing great patches for us, and it will help us
all if you know some more of the tricks/standard practices.

See the macros used elsewhere in the file, such as
tortore_assert_string_equal(), torture_assert_data_blob_equal()

> +	fprintf (stderr, "test_ndr_push_string %s flags 0x%x expecting err 0x%x and strcmp %s: ", string, flags, exp_ndr_err, strcmp_pass?"pass":"fail");
> +	if (exp_ndr_err != NDR_ERR_SUCCESS) {
> +		fprintf (stderr, "(ignore any Conversion error) ");
> +	}

See data_blob_const_string()

> +	blob.data = (char*)string;
> +	blob.length = strlen(string);
> +	ndr = ndr_pull_init_blob(&blob, mem_ctx);
> +	ndr_set_flags (&ndr->flags, flags);
> +
> +	err = ndr_pull_string (ndr, NDR_SCALARS, &result);
> +	if (err != exp_ndr_err) {
> +		fprintf (stderr, "FAIL: returned 0x%x but expected 0x%x\n",
> +		         err, exp_ndr_err);
> +	} else if (exp_ndr_err == NDR_ERR_SUCCESS) {
> +		if (result == NULL) {
> +			fprintf (stderr, "FAIL: succeeded but result NULL\n");
> +		} else {
> +			strcmp_res = strcmp(string, result);
> +			if (strcmp_res != 0 && strcmp_pass
> +			    || strcmp_res == 0 && !strcmp_pass) {
> +				fprintf (stderr, "FAIL: unexpected result for strcmp('%s','%s')\n", string, result);
> +			} else {
> +				ret = true;
> +			}
> +		}
> +	} else {
> +		ret = true;
> +	}
> +	if (ret) {
> +		fprintf (stderr, "PASS\n");

I think torture_ok() is the right thing here, but jelmer (CC'ed) may be
able to help get this right.  We may need to find another 'loop' test to
emulate.

When I last did string conversion tests I unrolled the loops, because it
gave better torture assert output, but it's ugly.

> +	}
> +
> +	talloc_free(ndr);
> +	return ret;
> +}
> +
> +static bool torture_ndr_string(struct torture_context *torture) {
> +	int i;
> +	struct ndr_string_test_data pushdata[] = {
> +		{ascii, fl_ascii_null, NDR_ERR_SUCCESS, true,
> +		 "test_ndr_push_string data: ASCII,STR_ASCII"},
> +		{utf8, fl_ascii_null, NDR_ERR_CHARCNV, false,
> +		 "test_ndr_push_string data: UTF8,STR_ASCII"},
> +		{utf8, fl_utf8_null, NDR_ERR_SUCCESS, true,
> +		 "test_ndr_push_string data: UTF8,STR_UTF8"},
> +		{latin1, fl_ascii_null, NDR_ERR_CHARCNV, false,
> +		 "test_ndr_push_string data: LATIN1,STR_ASCII"},
> +		{latin1, fl_raw8_null, NDR_ERR_SUCCESS, true,
> +		 "test_ndr_push_string data: LATIN1,STR_RAW8"},
> +		{utf8, fl_raw8_null, NDR_ERR_SUCCESS, true,
> +		 "test_ndr_push_string data: UTF8,STR_RAW8"},
> +		{NULL, 0, 0, false, NULL}
> +	};
> +	struct ndr_string_test_data pulldata[] = {
> +		{ascii, fl_ascii_null, NDR_ERR_SUCCESS, true,
> +		 "test_ndr_pull_string data: ASCII,STR_ASCII"},
> +		{utf8, fl_ascii_null, NDR_ERR_SUCCESS, false,
> +		 "test_ndr_pull_string data: UTF8,STR_ASCII"},
> +		{utf8, fl_utf8_null, NDR_ERR_SUCCESS, true,
> +		 "test_ndr_pull_string data: UTF8,STR_UTF8"},
> +		{latin1, fl_ascii_null, NDR_ERR_SUCCESS, false,
> +		 "test_ndr_pull_string data: LATIN1,STR_ASCII"},
> +		{latin1, fl_raw8_null, NDR_ERR_SUCCESS, true,
> +		 "test_ndr_pull_string data: LATIN1,STR_RAW8"},
> +		{utf8, fl_raw8_null, NDR_ERR_SUCCESS, true,
> +		 "test_ndr_pull_string data: UTF8,STR_RAW8"},
> +		{NULL, 0, 0, false, NULL}
> +	};

Typical in torture tests is to use

for (i=0; i < ARRAY_SIZE(pushdata); i++) {

and to then not null terminate the array

> +	for (i = 0; pushdata[i].data != NULL; ++i) {
> +		struct ndr_string_test_data *nstd = &pushdata[i];
> +		bool ok = test_ndr_push_string (nstd->data, nstd->flags,
> +		                                nstd->expected_code,
> +		                                nstd->should_strcmp_pass);
> +		torture_assert(torture, ok, nstd->desc);

We may need to regig this a little so we assert/ok at the right spots.

> +	}
> +
> +	for (i = 0; pulldata[i].data != NULL; ++i) {
> +		struct ndr_string_test_data *nstd = &pulldata[i];
> +		bool ok = test_ndr_pull_string (nstd->data, nstd->flags,
> +		                                nstd->expected_code,
> +		                                nstd->should_strcmp_pass);
> +		torture_assert(torture, ok, nstd->desc);
> +	}
> +
> +	return true;
> +}
> +
> +struct torture_suite *ndr_string_suite(TALLOC_CTX *ctx)
> +{
> +	struct torture_suite *suite = torture_suite_create(ctx, "ndr_string");
> +
> +	torture_suite_add_simple_test(suite, "ndr_string", torture_ndr_string);
> +	suite->description = talloc_strdup(suite, "NDR - string-conversion focused push/pull tests");
> +
> +	return suite;
> +}
> diff --git a/source4/torture/wscript_build b/source4/torture/wscript_build
> index 68ec4e6..106cc64 100644
> --- a/source4/torture/wscript_build
> +++ b/source4/torture/wscript_build
> @@ -33,7 +33,7 @@ bld.RECURSE('libnetapi')
>  bld.RECURSE('libsmbclient')
>  
>  bld.SAMBA_SUBSYSTEM('TORTURE_NDR',
> -	source='ndr/ndr.c ndr/winreg.c ndr/atsvc.c ndr/lsa.c ndr/epmap.c ndr/dfs.c ndr/netlogon.c ndr/drsuapi.c ndr/spoolss.c ndr/samr.c ndr/dfsblob.c ndr/drsblobs.c ndr/nbt.c ndr/ntlmssp.c ndr/backupkey.c',
> +	source='ndr/ndr.c ndr/winreg.c ndr/atsvc.c ndr/lsa.c ndr/epmap.c ndr/dfs.c ndr/netlogon.c ndr/drsuapi.c ndr/spoolss.c ndr/samr.c ndr/dfsblob.c ndr/drsblobs.c ndr/nbt.c ndr/ntlmssp.c ndr/backupkey.c ndr/string.c',
>  	autoproto='ndr/proto.h',
>  	deps='torture'
>  	)

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org