[PATCH] Additional fix and test for bug #12899 - smbclient "setmode" no longer works to clear attribute bits due to dialect upgrade

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] Additional fix and test for bug #12899 - smbclient "setmode" no longer works to clear attribute bits due to dialect upgrade

Samba - samba-technical mailing list
Steve's fix for the smbclient setmode command
exposed a bug in our SMB1 implementation once I
wrote the test for it (our SMB2 code was already
correct).

An SMB1 SMBsetatr command should ignore the
set attribute request if a value of *zero* is
sent, we were ignoring it if a value of
FILE_ATTRIBUTE_NORMAL was sent.

FILE_ATTRIBUTE_NORMAL is already correctly
filtered out inside the set_dosmode() function.

Our SMB2 code already did this correctly.

Fix contains the SMB1 fix and then the
torture test that exposed the bug.

Please review and push if happy !

Thanks,

Jeremy.

bug-12899.master (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] Version 2 - Additional fix and test for bug #12899 - smbclient "setmode" no longer works to clear attribute bits due to dialect upgrade

Samba - samba-technical mailing list
On Fri, Jul 14, 2017 at 04:17:56PM -0700, Jeremy Allison via samba-technical wrote:

> Steve's fix for the smbclient setmode command
> exposed a bug in our SMB1 implementation once I
> wrote the test for it (our SMB2 code was already
> correct).
>
> An SMB1 SMBsetatr command should ignore the
> set attribute request if a value of *zero* is
> sent, we were ignoring it if a value of
> FILE_ATTRIBUTE_NORMAL was sent.
>
> FILE_ATTRIBUTE_NORMAL is already correctly
> filtered out inside the set_dosmode() function.
>
> Our SMB2 code already did this correctly.
>
> Fix contains the SMB1 fix and then the
> torture test that exposed the bug.
Bah. I missed this fix meant a change in
SMB1 server behavior (SMBsetatr 0 means
clear all attributes, the torture code
depends on that).

Here's the updated version that passes
all the existing samba3.smbtorture_s3
tests. Sorry for the mistake.

Please review,

Jeremy.

bug-12899.master (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Version 2 - Additional fix and test for bug #12899 - smbclient "setmode" no longer works to clear attribute bits due to dialect upgrade

Samba - samba-technical mailing list
On Fri, Jul 14, 2017 at 04:47:16PM -0700, Jeremy Allison wrote:

> On Fri, Jul 14, 2017 at 04:17:56PM -0700, Jeremy Allison via samba-technical wrote:
> > Steve's fix for the smbclient setmode command
> > exposed a bug in our SMB1 implementation once I
> > wrote the test for it (our SMB2 code was already
> > correct).
> >
> > An SMB1 SMBsetatr command should ignore the
> > set attribute request if a value of *zero* is
> > sent, we were ignoring it if a value of
> > FILE_ATTRIBUTE_NORMAL was sent.
> >
> > FILE_ATTRIBUTE_NORMAL is already correctly
> > filtered out inside the set_dosmode() function.
> >
> > Our SMB2 code already did this correctly.
> >
> > Fix contains the SMB1 fix and then the
> > torture test that exposed the bug.
>
> Bah. I missed this fix meant a change in
> SMB1 server behavior (SMBsetatr 0 means
> clear all attributes, the torture code
> depends on that).
>
> Here's the updated version that passes
> all the existing samba3.smbtorture_s3
> tests. Sorry for the mistake.

ARGGGHHHH. It may pass samba3.smbtorture_s3,
but this isn't what Windows does (tm) - as
tested against Win2KR12 SMB1 server.

Please ignore this patchset until I've
gotten everything sorted out against
"what Windows does (tm)".

Sorry for the noise.

Jeremy.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Version 2 - Additional fix and test for bug #12899 - smbclient "setmode" no longer works to clear attribute bits due to dialect upgrade

Samba - samba-technical mailing list
On Fri, Jul 14, 2017 at 05:09:29PM -0700, Jeremy Allison via samba-technical wrote:

> On Fri, Jul 14, 2017 at 04:47:16PM -0700, Jeremy Allison wrote:
> > On Fri, Jul 14, 2017 at 04:17:56PM -0700, Jeremy Allison via samba-technical wrote:
> > > Steve's fix for the smbclient setmode command
> > > exposed a bug in our SMB1 implementation once I
> > > wrote the test for it (our SMB2 code was already
> > > correct).
> > >
> > > An SMB1 SMBsetatr command should ignore the
> > > set attribute request if a value of *zero* is
> > > sent, we were ignoring it if a value of
> > > FILE_ATTRIBUTE_NORMAL was sent.
> > >
> > > FILE_ATTRIBUTE_NORMAL is already correctly
> > > filtered out inside the set_dosmode() function.
> > >
> > > Our SMB2 code already did this correctly.
> > >
> > > Fix contains the SMB1 fix and then the
> > > torture test that exposed the bug.
> >
> > Bah. I missed this fix meant a change in
> > SMB1 server behavior (SMBsetatr 0 means
> > clear all attributes, the torture code
> > depends on that).
> >
> > Here's the updated version that passes
> > all the existing samba3.smbtorture_s3
> > tests. Sorry for the mistake.
>
> ARGGGHHHH. It may pass samba3.smbtorture_s3,
> but this isn't what Windows does (tm) - as
> tested against Win2KR12 SMB1 server.
>
> Please ignore this patchset until I've
> gotten everything sorted out against
> "what Windows does (tm)".

OK - figured it out.

Against Windows

-------------------------------------------------------------------
Over SMB2 - doing a SMB2_CREATE/SET_FILE_BASIC_INFORMATION/CLOSE

if attr == 0 - all attributes are left alone.
if attr == 0x80 (FILE_ATTRIBUTE_NORMAL) - all attributes are cleared (server returns FILE_ATTRIBUTE_NORMAL on query).

Over SMB1 - using SMB1setatr (0x9):

if attr == 0 - all attributes are cleared (server returns FILE_ATTRIBUTE_NORMAL on query).
if attr == 0x80 (FILE_ATTRIBUTE_NORMAL) - all attributes are left alone.
-------------------------------------------------------------------

Completely and utterly insane, but there you go and I
have the wireshark traces to prove it :-).

What that means is Steve's original patch is incorrect
(sorry for the +1 review, all I can say is it looked
correct at the time but further testing proved otherwise :-).

Yay for more tests :-).

Most of our existing code uses the API:

cli_setatr(cli1, fname, 0, 0);

to mean clear all attributes. So what I think I'm going to
do is revert Steve's patch and then change cli_smb2_setatr()
to change attr == 0 to map attr to FILE_ATTRIBUTE_NORMAL
internally for SMB2. That's actually following the intent.

It does mean there's no way to use '0' in SMB2 to mean
'leave this alone', but I know of no use cases of that
in our existing code.

The most important thing is that cli_setatr() exposed
to libsmbclient via SMBC_setatr(), so doing the SMB2
mapping will keep existing external users working correctly
with the move to SMB2 by default.

Thoughts ?

Jeremy.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Version 2 - Additional fix and test for bug #12899 - smbclient "setmode" no longer works to clear attribute bits due to dialect upgrade

Samba - samba-technical mailing list
On Fri, Jul 14, 2017 at 05:39:41PM -0700, Jeremy Allison via samba-technical wrote:

> OK - figured it out.
>
> Against Windows
>
> -------------------------------------------------------------------
> Over SMB2 - doing a SMB2_CREATE/SET_FILE_BASIC_INFORMATION/CLOSE
>
> if attr == 0 - all attributes are left alone.
> if attr == 0x80 (FILE_ATTRIBUTE_NORMAL) - all attributes are cleared (server returns FILE_ATTRIBUTE_NORMAL on query).
>
> Over SMB1 - using SMB1setatr (0x9):
>
> if attr == 0 - all attributes are cleared (server returns FILE_ATTRIBUTE_NORMAL on query).
> if attr == 0x80 (FILE_ATTRIBUTE_NORMAL) - all attributes are left alone.
> -------------------------------------------------------------------
>
> Completely and utterly insane, but there you go and I
> have the wireshark traces to prove it :-).
>
> What that means is Steve's original patch is incorrect
> (sorry for the +1 review, all I can say is it looked
> correct at the time but further testing proved otherwise :-).
>
> Yay for more tests :-).
>
> Most of our existing code uses the API:
>
> cli_setatr(cli1, fname, 0, 0);
>
> to mean clear all attributes. So what I think I'm going to
> do is revert Steve's patch and then change cli_smb2_setatr()
> to change attr == 0 to map attr to FILE_ATTRIBUTE_NORMAL
> internally for SMB2. That's actually following the intent.
>
> It does mean there's no way to use '0' in SMB2 to mean
> 'leave this alone', but I know of no use cases of that
> in our existing code.

Thinking about this on the drive home, it means
that we just change the SMB2 code to match SMB1.

This means:

cli_setatr(cli, fname, FILE_ATTRIBUTE_NORMAL, 0);

specifies "no change" for both SMB1 and SMB2, and

cli_setatr(cli, fname, FILE_ATTRIBUTE_NORMAL, 0);

means "clear all attributes" for both SMB1 and SMB2.

That's the way the current SMB1-specific cli_setatr()
code works, so this fix swaps the semantics for
SMB2 to keep the existing libsmbclient ABI for callers.

Essentially inside cli_smb2_setatr() I do:

if (attr == 0) {
        attr = FILE_ATTRIBUTE_NORMAL;
} else if (attr == FILE_ATTRIBUTE_NORMAL;
        attr = 0;
}

to swap the meaning to the SMB1 semantics (with
appropriate comments explaining this of course).

That keeps the existing ABI and gives no change
to any cli_setatr() or libsmbclient callers. After
reverting Steve's patch and putting this in place
the new test patch will excercise this perfectly
as a regression test.

I'll also look into adding a source4/torture test
for the insane SMB1/SMB2 semantics to make sure
we test the server correctly without going through
the cli_setatr() interface.

Sounds like a plan - should get this done on Monday !

Jeremy.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Version 2 - Additional fix and test for bug #12899 - smbclient "setmode" no longer works to clear attribute bits due to dialect upgrade

Samba - samba-technical mailing list
On Fri, Jul 14, 2017 at 10:46:15PM -0700, Jeremy Allison wrote:

>
> Thinking about this on the drive home, it means
> that we just change the SMB2 code to match SMB1.
>
> This means:
>
> cli_setatr(cli, fname, FILE_ATTRIBUTE_NORMAL, 0);
>
> specifies "no change" for both SMB1 and SMB2, and
>
> cli_setatr(cli, fname, FILE_ATTRIBUTE_NORMAL, 0);
>
> means "clear all attributes" for both SMB1 and SMB2.

I meant:

  cli_setatr(cli, fname, 0, 0);

for "clear all attributes" of course, that's what
the existing code uses.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Version 2 - Additional fix and test for bug #12899 - smbclient "setmode" no longer works to clear attribute bits due to dialect upgrade

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
Hi Jeremy,

just replying so you're not just talking to self. ;)

On Fri, Jul 14, 2017 at 10:46:15PM -0700, Jeremy Allison wrote:
> Sounds like a plan - should get this done on Monday !

Yes, sounds good.

Cheerio!
-slow

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Version 2 - Additional fix and test for bug #12899 - smbclient "setmode" no longer works to clear attribute bits due to dialect upgrade

Samba - samba-technical mailing list
On Sat, Jul 15, 2017 at 08:15:26AM +0200, Ralph Böhme wrote:
> Hi Jeremy,
>
> just replying so you're not just talking to self. ;)
>
> On Fri, Jul 14, 2017 at 10:46:15PM -0700, Jeremy Allison wrote:
> > Sounds like a plan - should get this done on Monday !
>
> Yes, sounds good.

Thanks, was getting lonely :-).

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Version 2 - Additional fix and test for bug #12899 - smbclient "setmode" no longer works to clear attribute bits due to dialect upgrade

Samba - samba-technical mailing list
Am 15.07.2017 um 08:27 schrieb Jeremy Allison:

> On Sat, Jul 15, 2017 at 08:15:26AM +0200, Ralph Böhme wrote:
>> Hi Jeremy,
>>
>> just replying so you're not just talking to self. ;)
>>
>> On Fri, Jul 14, 2017 at 10:46:15PM -0700, Jeremy Allison wrote:
>>> Sounds like a plan - should get this done on Monday !
>>
>> Yes, sounds good.
>
> Thanks, was getting lonely :-).
I just found:

commit ef4475adc4b8da0e3acfe6d70b2afa94a1e500cd
Author:     Stefan Metzmacher <[hidden email]>
AuthorDate: Fri Jun 30 08:16:59 2006 +0000
Commit:     Gerald (Jerry) Carter <[hidden email]>
CommitDate: Wed Oct 10 14:09:39 2007 -0500

    r16706: for RAW_SFILEINFO_SETATTR attrib == 0 means set it to
FILE_ATTRIB_NORMAL
    and attrib == FILE_ATTRIB_NORMAL means no change...

    but for RAW_SFILEINFO_BASIC_INFORMATION attrib == 0 means no change

    metze
    (This used to be commit e1945feda09a56b6f55bd0f7ab591f3bd069be67)
---
 source4/ntvfs/posix/pvfs_setfileinfo.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/source4/ntvfs/posix/pvfs_setfileinfo.c
b/source4/ntvfs/posix/pvfs_setfileinfo.c
index 2665b9e..6c7c6dc 100644
--- a/source4/ntvfs/posix/pvfs_setfileinfo.c
+++ b/source4/ntvfs/posix/pvfs_setfileinfo.c
@@ -493,7 +493,9 @@ NTSTATUS pvfs_setpathinfo(struct
ntvfs_module_context *ntvfs,
                if (!null_time(info->setattr.in.write_time)) {
                        unix_to_nt_time(&newstats.dos.write_time,
info->setattr.in.write_time);
                }
-               if (info->setattr.in.attrib != FILE_ATTRIBUTE_NORMAL) {
+               if (info->setattr.in.attrib == 0) {
+                       newstats.dos.attrib = FILE_ATTRIBUTE_NORMAL;
+               } else if (info->setattr.in.attrib !=
FILE_ATTRIBUTE_NORMAL) {
                        newstats.dos.attrib = info->setattr.in.attrib;
                }
                break;

I guess this implemented this difference.

Maybe we should unify some more code from smb_set_file_dosmode() and
reply_setatr(), I think they should only handle the
FILE_ATTRIBUTE_NORMAL difference and otherwise let file_set_dosmode()
the common logic.

I guess we should be able to move the FILE_ATTRIBUTE_DIRECTORY
logic to file_set_dosmode().

And also the /* check the mode isn't different, before changing it */
logic would simply some callers of file_set_dosmode() and also
doesn't hurt all other callers.

metze






signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Version 2 - Additional fix and test for bug #12899 - smbclient "setmode" no longer works to clear attribute bits due to dialect upgrade

Samba - samba-technical mailing list
On Sat, Jul 15, 2017 at 09:40:35AM +0200, Stefan Metzmacher wrote:

>
> I just found:
>
> commit ef4475adc4b8da0e3acfe6d70b2afa94a1e500cd
> Author:     Stefan Metzmacher <[hidden email]>
> AuthorDate: Fri Jun 30 08:16:59 2006 +0000
> Commit:     Gerald (Jerry) Carter <[hidden email]>
> CommitDate: Wed Oct 10 14:09:39 2007 -0500
>
>     r16706: for RAW_SFILEINFO_SETATTR attrib == 0 means set it to
> FILE_ATTRIB_NORMAL
>     and attrib == FILE_ATTRIB_NORMAL means no change...
>
>     but for RAW_SFILEINFO_BASIC_INFORMATION attrib == 0 means no change
>
>     metze
>     (This used to be commit e1945feda09a56b6f55bd0f7ab591f3bd069be67)
> ---
>  source4/ntvfs/posix/pvfs_setfileinfo.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/source4/ntvfs/posix/pvfs_setfileinfo.c
> b/source4/ntvfs/posix/pvfs_setfileinfo.c
> index 2665b9e..6c7c6dc 100644
> --- a/source4/ntvfs/posix/pvfs_setfileinfo.c
> +++ b/source4/ntvfs/posix/pvfs_setfileinfo.c
> @@ -493,7 +493,9 @@ NTSTATUS pvfs_setpathinfo(struct
> ntvfs_module_context *ntvfs,
>                 if (!null_time(info->setattr.in.write_time)) {
>                         unix_to_nt_time(&newstats.dos.write_time,
> info->setattr.in.write_time);
>                 }
> -               if (info->setattr.in.attrib != FILE_ATTRIBUTE_NORMAL) {
> +               if (info->setattr.in.attrib == 0) {
> +                       newstats.dos.attrib = FILE_ATTRIBUTE_NORMAL;
> +               } else if (info->setattr.in.attrib !=
> FILE_ATTRIBUTE_NORMAL) {
>                         newstats.dos.attrib = info->setattr.in.attrib;
>                 }
>                 break;
>
> I guess this implemented this difference.

Yeah, that's in the ntvfs server, I don't think it
ever implemented the SMB2 semantics here. If it
fails make test I'm planning to just mark it knownfail.

Don't have the resources to support 2 servers :-(.

> Maybe we should unify some more code from smb_set_file_dosmode() and
> reply_setatr(), I think they should only handle the
> FILE_ATTRIBUTE_NORMAL difference and otherwise let file_set_dosmode()
> the common logic.
>
> I guess we should be able to move the FILE_ATTRIBUTE_DIRECTORY
> logic to file_set_dosmode().
>
> And also the /* check the mode isn't different, before changing it */
> logic would simply some callers of file_set_dosmode() and also
> doesn't hurt all other callers.

Yeah, but that's code for a later fix :-). In the meantime
smbd actually implements the correct semantics for both
SMB1 and SMB2, I just need to fix the source3 client code
to make the cli_setat() ABI consistent across SMB1 and SMB2.

More on Monday.

Jeremy.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Version 2 - Additional fix and test for bug #12899 - smbclient "setmode" no longer works to clear attribute bits due to dialect upgrade

Samba - samba-technical mailing list
Am 15.07.2017 um 19:05 schrieb Jeremy Allison:

> On Sat, Jul 15, 2017 at 09:40:35AM +0200, Stefan Metzmacher wrote:
>>
>> I just found:
>>
>> commit ef4475adc4b8da0e3acfe6d70b2afa94a1e500cd
>> Author:     Stefan Metzmacher <[hidden email]>
>> AuthorDate: Fri Jun 30 08:16:59 2006 +0000
>> Commit:     Gerald (Jerry) Carter <[hidden email]>
>> CommitDate: Wed Oct 10 14:09:39 2007 -0500
>>
>>     r16706: for RAW_SFILEINFO_SETATTR attrib == 0 means set it to
>> FILE_ATTRIB_NORMAL
>>     and attrib == FILE_ATTRIB_NORMAL means no change...
>>
>>     but for RAW_SFILEINFO_BASIC_INFORMATION attrib == 0 means no change
>>
>>     metze
>>     (This used to be commit e1945feda09a56b6f55bd0f7ab591f3bd069be67)
>> ---
>>  source4/ntvfs/posix/pvfs_setfileinfo.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/source4/ntvfs/posix/pvfs_setfileinfo.c
>> b/source4/ntvfs/posix/pvfs_setfileinfo.c
>> index 2665b9e..6c7c6dc 100644
>> --- a/source4/ntvfs/posix/pvfs_setfileinfo.c
>> +++ b/source4/ntvfs/posix/pvfs_setfileinfo.c
>> @@ -493,7 +493,9 @@ NTSTATUS pvfs_setpathinfo(struct
>> ntvfs_module_context *ntvfs,
>>                 if (!null_time(info->setattr.in.write_time)) {
>>                         unix_to_nt_time(&newstats.dos.write_time,
>> info->setattr.in.write_time);
>>                 }
>> -               if (info->setattr.in.attrib != FILE_ATTRIBUTE_NORMAL) {
>> +               if (info->setattr.in.attrib == 0) {
>> +                       newstats.dos.attrib = FILE_ATTRIBUTE_NORMAL;
>> +               } else if (info->setattr.in.attrib !=
>> FILE_ATTRIBUTE_NORMAL) {
>>                         newstats.dos.attrib = info->setattr.in.attrib;
>>                 }
>>                 break;
>>
>> I guess this implemented this difference.
>
> Yeah, that's in the ntvfs server, I don't think it
> ever implemented the SMB2 semantics here. If it
> fails make test I'm planning to just mark it knownfail.
>
> Don't have the resources to support 2 servers :-(.
That was the patch to fix the ntvfs server.

>> Maybe we should unify some more code from smb_set_file_dosmode() and
>> reply_setatr(), I think they should only handle the
>> FILE_ATTRIBUTE_NORMAL difference and otherwise let file_set_dosmode()
>> the common logic.
>>
>> I guess we should be able to move the FILE_ATTRIBUTE_DIRECTORY
>> logic to file_set_dosmode().
>>
>> And also the /* check the mode isn't different, before changing it */
>> logic would simply some callers of file_set_dosmode() and also
>> doesn't hurt all other callers.
>
> Yeah, but that's code for a later fix :-). In the meantime
> smbd actually implements the correct semantics for both
> SMB1 and SMB2, I just need to fix the source3 client code
> to make the cli_setat() ABI consistent across SMB1 and SMB2.
>
> More on Monday.
Ok.

metze



signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Version 3 - Additional fix and test for bug #12899 - smbclient "setmode" no longer works to clear attribute bits due to dialect

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
OK, here's the version I think we need for
master.

#1 - revert the incorrect patch.
#2 - Do the actual fix by changing the SMB2 client
        code to provide the same ABI as the SMB1
        cli_setatr().
#3 - Test this code works. This code is excercised
        over both SMB1 and SMB2 so ensures the server
        provies the correct semantics for both SMB1
        and SMB2.

Fro 4.7.x and earlier we only need patches #2 and #3
for the backport, as those branches never got the
incorrect patch reverted by #1.

Please review and push if happy. Sorry for the original
issue (caused by not having a proper regression
test initially).

Cheers,

        Jeremy.

bug-12899.master (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Version 3 - Additional fix and test for bug #12899 - smbclient "setmode" no longer works to clear attribute bits due to dialect

Samba - samba-technical mailing list
On Mon, Jul 17, 2017 at 12:32 PM, Jeremy Allison <[hidden email]> wrote:

> OK, here's the version I think we need for
> master.
>
> #1 - revert the incorrect patch.
> #2 - Do the actual fix by changing the SMB2 client
>         code to provide the same ABI as the SMB1
>         cli_setatr().
> #3 - Test this code works. This code is excercised
>         over both SMB1 and SMB2 so ensures the server
>         provies the correct semantics for both SMB1
>         and SMB2.
>
> Fro 4.7.x and earlier we only need patches #2 and #3
> for the backport, as those branches never got the
> incorrect patch reverted by #1.
>
> Please review and push if happy. Sorry for the original
> issue (caused by not having a proper regression
> test initially).

I hate to be a pain in the ass, but since smbclients's setmode command
appears to accept a list of +mode and -mode commands, perhaps your
test should also test that the code work by having something like +XY
-ZA or whatever.

Otherwise: RB+


--
Regards,
Richard Sharpe
(何以解憂?唯有杜康。--曹操)

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Version 3 - Additional fix and test for bug #12899 - smbclient "setmode" no longer works to clear attribute bits due to dialect

Samba - samba-technical mailing list
On Mon, Jul 17, 2017 at 04:10:40PM -0700, Richard Sharpe wrote:

> On Mon, Jul 17, 2017 at 12:32 PM, Jeremy Allison <[hidden email]> wrote:
> > OK, here's the version I think we need for
> > master.
> >
> > #1 - revert the incorrect patch.
> > #2 - Do the actual fix by changing the SMB2 client
> >         code to provide the same ABI as the SMB1
> >         cli_setatr().
> > #3 - Test this code works. This code is excercised
> >         over both SMB1 and SMB2 so ensures the server
> >         provies the correct semantics for both SMB1
> >         and SMB2.
> >
> > Fro 4.7.x and earlier we only need patches #2 and #3
> > for the backport, as those branches never got the
> > incorrect patch reverted by #1.
> >
> > Please review and push if happy. Sorry for the original
> > issue (caused by not having a proper regression
> > test initially).
>
> I hate to be a pain in the ass, but since smbclients's setmode command
> appears to accept a list of +mode and -mode commands, perhaps your
> test should also test that the code work by having something like +XY
> -ZA or whatever.

OK, split the attrs into a list of set's/unset's rather than
doing as one arg.

> Otherwise: RB+

Thanks - pushed !

Loading...