Bug 10687 - 'RW2' smbtorture test fails when -N <numprocs> is set to 2 due to the invalid status check in the second client.
Summary: 'RW2' smbtorture test fails when -N <numprocs> is set to 2 due to the invalid...
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.6
Classification: Unclassified
Component: Client Tools (show other bugs)
Version: 3.6.12
Hardware: All All
: P5 normal
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-07-02 06:31 UTC by Warren
Modified: 2014-07-17 18:40 UTC (History)
0 users

See Also:


Attachments
patch (763 bytes, patch)
2014-07-02 14:30 UTC, Volker Lendecke
jra: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Warren 2014-07-02 06:31:43 UTC
Samba 3.6.12

While running smbtorture RW2 test, it FAILED with the following error.

# smbtorture //path_to_test_share -U user%password -W workgroup -N2 --samba RW2
using seed 1397624068
...
2 clients started
run_readwritemulti: fname \XX752ed930
run_readwritemulti: fname \XX752ed930
unlink failed (NT_STATUS_OBJECT_NAME_NOT_FOUND) (normal, this file should not exist)
second open read-only of \XX752ed930 failed (NT_STATUS_OBJECT_NAME_NOT_FOUND)
20   131069
TEST RW2 FAILED!
RW2 took 1.07827 secs

###############################################################################

"RW2 -N = 2" uses (forks) two clients. The first one will open() with O_CREAT | O_RDWR to create/write a file. The second one will open() with O_RDONLY to read the same file.

It looks to me there is a bug in torture.c:rw_torture3(). The second client can fail to open the file if it races and comes before the first client creates the file. The status check on the second client open probably not right e.g. "break if status != NT_STATUS_NOT_FOUND" rather than ""break if !NT_STATUS_IS_OK(status)". Otherwise, the loop of try open doesn't make sense to me.

The samba 4.1.7 code has the similar issue.

        if (procnum == 0)
        {
                if (!NT_STATUS_IS_OK(cli_unlink(c, lockfname, FILE_ATTRIBUTE_SY$
                        printf("unlink failed (%s) (normal, this file should no$
                }

                if (!NT_STATUS_IS_OK(cli_open(c, lockfname, O_RDWR | O_CREAT | $
                                 DENY_NONE, &fnum))) {
                        printf("first open read/write of %s failed (%s)\n",
                                        lockfname, cli_errstr(c));
                        return False;
                }
        }
        else
        {
                for (i = 0; i < 500 && fnum == (uint16_t)-1; i++)
                {
                        status = cli_open(c, lockfname, O_RDONLY,
                                         DENY_NONE, &fnum);
                        if (!NT_STATUS_IS_OK(status)) {  <===== Should not have "!"?
                                break;
                        }
                        smb_msleep(10);
                }
                if (!NT_STATUS_IS_OK(status)) {
                        printf("second open read-only of %s failed (%s)\n",
                                        lockfname, cli_errstr(c));
                        return False;
                }
        }
Comment 1 Volker Lendecke 2014-07-02 14:30:09 UTC
Created attachment 10068 [details]
patch

This is ancient... 2009ish. Entirely right!

These days we'd code this soo much differently :-)

Volker
Comment 2 Jeremy Allison 2014-07-04 01:01:04 UTC
Comment on attachment 10068 [details]
patch

LGTM
Comment 3 Jeremy Allison 2014-07-04 01:01:40 UTC
Re-assigning to Karolin for inclusion in 4.1.next. 4.0.next.
Jeremy.
Comment 4 Karolin Seeger 2014-07-11 07:53:51 UTC
(In reply to comment #3)
> Re-assigning to Karolin for inclusion in 4.1.next. 4.0.next.
> Jeremy.

Pushed to autobuild-v4-[0|1]-test.
Comment 5 Karolin Seeger 2014-07-17 18:40:54 UTC
(In reply to comment #4)
> (In reply to comment #3)
> > Re-assigning to Karolin for inclusion in 4.1.next. 4.0.next.
> > Jeremy.
> 
> Pushed to autobuild-v4-[0|1]-test.

Pushed to both branches.
Closing out bug report.

Thanks!