cifs: remove "sockopt" mount option

[Steve French-2]

Looking at patch
http://git.samba.org/?p=jlayton/cifs.git;a=commit;h=1c1e39d7cd344ef662c958cfad5e1a0df804ac74

which would remove the "sockopt" mount option (which is currently
unimplemented), I spent some time looking at the equivalent smb.conf
mount option ("socket options") which smbfs and samba client and
server use.  The explanation and examples of times it is useful to
override socket options (e.g. for setting low delay) seem reasonable.
It looks like it would be better to add parsing to allow setting these
to allow those who need to do advanced tuning.

--
Thanks,

Steve
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Jeff Layton-2]

On Thu, 25 Jun 2009 17:42:43 -0500
Steve French <smfrench@...> wrote:

> Looking at patch
> http://git.samba.org/?p=jlayton/cifs.git;a=commit;h=1c1e39d7cd344ef662c958cfad5e1a0df804ac74
>
> which would remove the "sockopt" mount option (which is currently
> unimplemented), I spent some time looking at the equivalent smb.conf
> mount option ("socket options") which smbfs and samba client and
> server use.  The explanation and examples of times it is useful to
> override socket options (e.g. for setting low delay) seem reasonable.
> It looks like it would be better to add parsing to allow setting these
> to allow those who need to do advanced tuning.
>

Ok, but let's not just leave this stub option laying around. The time
to add a new option to the parser is when there's some code to back it
up and make it actually do something useful.

A) it's confusing since it doesn't actually work

B) it just bloats out the code for no good purpose

For the record, I'm also disappointed that the commented out asn1
function got merged today too. I'd have preferred to see that go in
when it actually becomes useful (and usable).

--
Jeff Layton <jlayton@...>
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Steve French-2]

On Thu, Jun 25, 2009 at 6:28 PM, Jeff Layton<jlayton@...> wrote:
> On Thu, 25 Jun 2009 17:42:43 -0500
> For the record, I'm also disappointed that the commented out asn1
> function got merged today too. I'd have preferred to see that go in
> when it actually becomes useful (and usable).

I understand your point.  I thought the timing made sense because
1) Shirish had just added the missing ASN define for this (so the
timing of the change to add the code to support parsing a sequence
made sense)
2) I reviewed it and it looked fine
3) testing it it worked fine
4) I already reviewed it so didn't want to bloat his future patch
and make it larger with an ASN support function that
although needed for the kerberos session setup parsing, really
has nothing to do with kerberos otherwise.



--
Thanks,

Steve
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Jeff Layton-2]

On Thu, 25 Jun 2009 18:36:16 -0500
Steve French <smfrench@...> wrote:

> On Thu, Jun 25, 2009 at 6:28 PM, Jeff Layton<jlayton@...> wrote:
> > On Thu, 25 Jun 2009 17:42:43 -0500
> > For the record, I'm also disappointed that the commented out asn1
> > function got merged today too. I'd have preferred to see that go in
> > when it actually becomes useful (and usable).
>
> I understand your point.  I thought the timing made sense because
> 1) Shirish had just added the missing ASN define for this (so the
> timing of the change to add the code to support parsing a sequence
> made sense)
> 2) I reviewed it and it looked fine
> 3) testing it it worked fine
> 4) I already reviewed it so didn't want to bloat his future patch
> and make it larger with an ASN support function that
> although needed for the kerberos session setup parsing, really
> has nothing to do with kerberos otherwise.
>

I see no harm in making the second patch that will actually use this
function a little larger. The problem with this logic is of course:
"What if the second patch never materializes?". We're all busy and
things get postponed.

Then code changes happen that make the original function not work
right. Then someone has to go in and fix it (or review the as of yet
unused code). Or worse yet, they end up reimplementing the function and
just ripping out the unused one anyway. Commented out stuff like this
is extra-susceptible to bitrot.

CIFS is rife with examples of this sort of thing. The only sane way to
manage this is not to merge code that isn't actually being used. That's
what the mainline kernel tree usually does. CIFS should follow suit.

--
Jeff Layton <jlayton@...>
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[simo-7]

On Thu, 2009-06-25 at 19:52 -0400, Jeff Layton wrote:
>
> CIFS is rife with examples of this sort of thing. The only sane way to
> manage this is not to merge code that isn't actually being used.
> That's
> what the mainline kernel tree usually does. CIFS should follow suit.

+1

--
Simo Sorce
Samba Team GPL Compliance Officer <simo@...>
Principal Software Engineer at Red Hat, Inc. <simo@...>

_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Christoph Hellwig]

On Thu, Jun 25, 2009 at 06:36:16PM -0500, Steve French wrote:
> On Thu, Jun 25, 2009 at 6:28 PM, Jeff Layton<jlayton@...> wrote:
> > On Thu, 25 Jun 2009 17:42:43 -0500
> > For the record, I'm also disappointed that the commented out asn1
> > function got merged today too. I'd have preferred to see that go in
> > when it actually becomes useful (and usable).
>
> I understand your point.  I thought the timing made sense because


 a) unused code should never go in
 b) patches should go out to a mailinglist and reviewed.

Can we please get these basic rules of linux development discipline
working again in cifs-land?

_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Steve French-2]

On Fri, Jun 26, 2009 at 12:04 PM, Christoph Hellwig<hch@...> wrote:
>  a) unused code should never go in
After discussion with me and Jeff, Shirish did not think the next
patch in the series was ready yet.  He told me he is working on it
now.  The next patch in the series uses the decode ASN_SEQ function.
The rest of the patch though was needed for the case of NTLMSSP mounts
to Windows Vista and Windows 2008 (not needed for mounts to XP and
earlier).

>  b) patches should go out to a mailinglist and reviewed.

I reminded Shirish to post his ASN patch (and all future patches) more
broadly, but he had sent out various versions of his patch via email.



--
Thanks,

Steve
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client