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

[Sean Finney]

Hi Andrew,

On Fri, May 27, 2011 at 07:48:19AM +1000, Andrew Bartlett wrote:
> 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.

Happy to refine the patch :)

> 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()

itym data_blob_string_const, but not sure how it fits in above with the
informational string printing.  but definitely other places, like here:

> > +	blob.data = (char*)string;
> > +	blob.length = strlen(string);

make sense and I'll modify them in the meantime.  Let me know if there's
other contexts in which I should use it.

> > +	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.

will take a look.

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

Yeah, that's why I included the "desc" field in that struct
ndr_string_test_data, so that there would be something descriptive
in the assert failure (i.e. "UTF8/STR_ASCII").  But if you need/want
me to change how that's done it's not a prob.  We could for example
use individually defined structs (each with a descriptive variable name)
instead of an array, which would give better assert output i guess.
And if the asserts are moved into the test functions then it wouldn't
be *that* ugly...  So just let me know.

> > +		{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

ah, didn't know about the macro.  went for the NULL terminated approach for
purely cosmetic/aesthetic reasons (using a sizeof(foo) / sizeof(bar) condition
pushed the for(...) line over 80 chars, that was all).  simple fix, will
do while waiting to hear back about whether we should have the loop at
all.

> > +	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.

yeah, i'll push the asserts deeper into the test functions themselves.


	sean