[PATCH] Replace CHECK_STATUS with torture macros

[Michael Adam]

Anoop,

Thanks for your patience! :-)

Reviewed-by: me

Cheers - Michael

On 2015-10-29 at 23:07 +0530, Anoop C S wrote:
> On Tue, 2015-10-27 at 17:06 +0100, Michael Adam wrote:
> > On 2015-10-27 at 18:43 +0530, Anoop C S wrote:
> > > On Tue, 2015-10-27 at 12:00 +0100, Michael Adam wrote:
> > > > On 2015-10-27 at 11:54 +0530, Anoop C S wrote:
> > > > > On Mon, 2015-10-26 at 11:26 +0100, Michael Adam wrote:
> > > > > > Hi Anoop,
> > > > > > 
> > > > > > The patch looks clean now.
> > > > > > But I have one more request:
> > > > > > 
> > > > > > Just as you split the addition of torture_asserts for
> > > > > > the smb2_unlink calls (that had no assert with them
> > > > > > before) into a patch of their own, could you do the
> > > > > > same with the smb2_util_close calls?
> > > > > > I just would like the patch that is called
> > > > > > "Replace CHECK_STATUS with torture_assert macros"
> > > > > > to do that and not more (adding checks where there
> > > > > > were none before). You could do a third patch or
> > > > > > add add these hunks to the patch with the unlink part.
> > > > > > It is up to you.
> > > > > > 
> > > > > 
> > > > > Attaching 3 patches as per Michael's comments.
> > > > 
> > > > Thanks!
> > > > 
> > > > This looks much cleaner now.
> > > > 
> > > > Just one more remark (sorry..):
> > > > 
> > > 
> > > No problem.
> > > 
> > > > In the first patch (replacing CHECK_STATUS by torture asserts),
> > > > when you are not checking for STATUS_OK, then your messages
> > > > 'smb2_foobar failed' are misleading. In those few cases, you
> > > > should rather utter a message like 'smb2_foobar gave unexpected
> > > > result' or similar..
> > > > 
> > > > 
> > > > Example:
> > > > 
> > > > > diff --git a/source4/torture/smb2/session.c
> > > > > b/source4/torture/smb2/session.c
> > > > > index 798230b..92f404c 100644
> > > > > --- a/source4/torture/smb2/session.c
> > > > > +++ b/source4/torture/smb2/session.c
> > > > > @@ -105,7 +98,10 @@ bool test_session_reconnect1(struct
> > > > > torture_context *tctx, struct smb2_tree *tre
> > > > >  	qfinfo.generic.level =
> > > > > RAW_FILEINFO_POSITION_INFORMATION;
> > > > >  	qfinfo.generic.in.file.handle = _h1;
> > > > >  	status = smb2_getinfo_file(tree, mem_ctx, &qfinfo);
> > > > > -	CHECK_STATUS(status, NT_STATUS_USER_SESSION_DELETED);
> > > > > +	torture_assert_ntstatus_equal_goto(tctx, status,
> > > > > +					  
> > > > >  NT_STATUS_USER_SESSION_DELETED,
> > > > > +					   ret, done,
> > > > > +					   "smb2_getinfo_file
> > > > > failed");
> > > > 
> > > > Here failure of smb2_getinfo_file is expected and succes
> > > > of the function call would actually be a failure of the test
> > > > just any status code other than NT_STATUS_USER_SESSION_DELETED
> > > > would be.
> > > > 
> > > 
> > > I have made necessary changes. Please let me know if I am missing
> > > some
> > > parts.
> > 
> > This looks good.
> > 
> > patch #1: reviewed-by me
> > patch #3: reviewed-by me
> > 
> > patch #2: there is one concern:
> >   Most of the unlink calls are at the beginning of the test case.
> >   The purpose of these calls was to make sure that the file does
> >   not exist in the share for the remainder of the test, so we can
> >   start from scratch. The file could in theory exist in the share.
> > 
> >   So I think the omission of asserts/checks was intentional
> >   in those cases. So we should possibly better omit them?
> > 
> >   I spotted at least one unlink in the middle of a test case
> >   that validly gets an assert.
> 
> Removed all other torture asserts except the one mentioned above.
> Reattaching all 3 patches again.
> 
> > Cheers - Michael
> > 
> > > > Cheers - Michael
> > 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20151029/10bbf110/signature.sig>