Discussion:
[RFC] To make libsmbclient work for DFS shares for samba3
(too old to reply)
boyang
2009-02-16 19:40:25 UTC
Permalink
Hi, everyone:
Does it make sense to make libsmbclient work for DFS shares? At
present, it works only if DFS target can be accessed anynmously. We
should invoke smbc_set_credentials() or cli_set_cm_credentials() before
the function cli_resolve_path() to set the correct credentials to make
DFS share work with libsmbclient. Please correct me if I am wrong.
If it makes sense, I can take this. :-)
Thanks!
Best
Regards
BoYang
-------------- next part --------------
A non-text attachment was scrubbed...
Name: boyang.vcf
Type: text/x-vcard
Size: 187 bytes
Desc: not available
Url : http://lists.samba.org/archive/samba-technical/attachments/20090216/2a7b1dd6/boyang.vcf
Derrell Lipman
2009-02-16 19:54:01 UTC
Permalink
Post by boyang
Does it make sense to make libsmbclient work for DFS shares? At
present, it works only if DFS target can be accessed anynmously. We
should invoke smbc_set_credentials() or cli_set_cm_credentials() before
the function cli_resolve_path() to set the correct credentials to make
DFS share work with libsmbclient. Please correct me if I am wrong.
If it makes sense, I can take this. :-)
There was a bunch of work done about a year or two ago to make DFS work with
libsmbclient, and I recall adding smbc_set_credentials() fairly recently. I
don't have any kind of test setup for DFS, so I have to rely on reports from
the field in that area. This should be as automated as possible without
affecting non-DFS environments. If you have a test environment for DFS,
please test your changes and then send me a patch to libsmbclient.

Thanks,

Derrell
Jeremy Allison
2009-02-17 07:24:09 UTC
Permalink
Post by boyang
Does it make sense to make libsmbclient work for DFS shares? At
present, it works only if DFS target can be accessed anynmously. We
should invoke smbc_set_credentials() or cli_set_cm_credentials() before
the function cli_resolve_path() to set the correct credentials to make
DFS share work with libsmbclient. Please correct me if I am wrong.
I thought it was already using the cached credentials
to connect to other dfs servers ?
Post by boyang
If it makes sense, I can take this. :-)
If it's not working, I'd like to know :-).

Jeremy.
boyang
2009-02-17 17:07:34 UTC
Permalink
Post by Jeremy Allison
Post by boyang
Does it make sense to make libsmbclient work for DFS shares? At
present, it works only if DFS target can be accessed anynmously. We
should invoke smbc_set_credentials() or cli_set_cm_credentials() before
the function cli_resolve_path() to set the correct credentials to make
DFS share work with libsmbclient. Please correct me if I am wrong.
I thought it was already using the cached credentials
to connect to other dfs servers ?
Post by boyang
If it makes sense, I can take this. :-)
If it's not working, I'd like to know :-).
No, It is not working if the DFS target cannot be accessed anonymously.
Also keep derrell in the loop. :-)
Derrell & Jeremy:
Here is the patch for master, pls review it. If it is OK, I'll
backport it to 3-[023]. Thanks!
I have done some tests using code under directory
examples/libsmbclient/. Before the patch, DFS works with libsmbclient
only when DFS target can be accessed anonymously. After the change, it
works fine for me. :-)
BTW: the Makefile under examples/libsmbclient seems broken,
missing some libraries. I can toggle it to build, __BUT__ I am really
not sure which ones are missed. :-)
Post by Jeremy Allison
Jeremy.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: libsmbclient-dfs-master.diff
Type: text/x-patch
Size: 19051 bytes
Desc: not available
Url : http://lists.samba.org/archive/samba-technical/attachments/20090217/53362533/libsmbclient-dfs-master-0001.bin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: boyang.vcf
Type: text/x-vcard
Size: 187 bytes
Desc: not available
Url : http://lists.samba.org/archive/samba-technical/attachments/20090217/53362533/boyang-0001.vcf
Derrell Lipman
2009-02-17 19:40:02 UTC
Permalink
Post by boyang
Post by Jeremy Allison
If it's not working, I'd like to know :-).
No, It is not working if the DFS target cannot be accessed anonymously.
Also keep derrell in the loop. :-)
Here is the patch for master, pls review it. If it is OK, I'll
backport it to 3-[023]. Thanks!
Hi. I've looked only briefly at your patch. Thanks for working on this!

There is one significant problem with the patch which makes it difficult to
include as is: you've modified the API/ABI with the signature change of
smbc_set_credentials(). It looks like the change you made possibly should
have been there all along, but I'd like to look into alternatives to
changing the signature.

Also, there are numerous calls to your new wrapper function. When I get some
time to look into this, I'll see if maybe that functionality can be merged
into the generic code in libsmb_server.c instead of spread throughout the
library.

I may not get to this right away. If you'd like to look into the above two
issues, that'd be great. It looks like the changes to the core
(non-libsmbclient) code are fairly unobtrusive, but I suspect Jeremy will
comment on those if there are any issues.


BTW: the Makefile under examples/libsmbclient seems broken,
Post by boyang
missing some libraries. I can toggle it to build, __BUT__ I am really
not sure which ones are missed. :-)
Please provide details. I use that Makefile all the time for my testing and
have no issues, but I'm testing only under Linux. What OS are you using, and
what issues are you seeing?

Thanks again,

Derrell
boyang
2009-02-17 20:21:06 UTC
Permalink
Post by Derrell Lipman
Hi. I've looked only briefly at your patch. Thanks for working on this!
There is one significant problem with the patch which makes it
difficult to include as is: you've modified the API/ABI with the
signature change of smbc_set_credentials(). It looks like the change
you made possibly should have been there all along, but I'd like to
look into alternatives to changing the signature.
If we want to keep the API untact, we can use another function to set
fallback_after_kerberos. The question here is that I don't know if any
applications have used smbc_set_credentials(), it there is no, we can
safely change it. But if there is, we might have to use another function
to set fallback_after_kerberos. :-) But this have to be called in order,
smbc_set_credentials() first, and then set fallback_after_kerberos to
avoid surprise.
Post by Derrell Lipman
Also, there are numerous calls to your new wrapper function. When I
get some time to look into this, I'll see if maybe that functionality
can be merged into the generic code in libsmb_server.c instead of
spread throughout the library.
It cannot be merged into SMBC_server alone, if this is what you mean.
Because some SMBC_XXX_ctx function don't call SMBC_server() before call
cli_resolve_path(). Maybe it is possible to merge it in
SMBC_parse_path() and SMBC_server().
Post by Derrell Lipman
I may not get to this right away. If you'd like to look into the above
two issues, that'd be great. It looks like the changes to the core
(non-libsmbclient) code are fairly unobtrusive, but I suspect Jeremy
will comment on those if there are any issues.
BTW: the Makefile under examples/libsmbclient seems broken,
missing some libraries. I can toggle it to build, __BUT__ I am really
not sure which ones are missed. :-)
Please provide details. I use that Makefile all the time for my
testing and have no issues, but I'm testing only under Linux. What OS
are you using, and what issues are you seeing?
I am using Linux too. It is linking error, some functions that is used
by libsmbclient are not found.(I cannot remember them exactly, one looks
like related to dictionary parse, and one looks like something in zlib,
several others...). I added "-lrt -liniparser -lnsl -lz -lcap
-lcom_err" to LDFLAGS to make it work.
Post by Derrell Lipman
Thanks again,
Derrell
-------------- next part --------------
A non-text attachment was scrubbed...
Name: boyang.vcf
Type: text/x-vcard
Size: 187 bytes
Desc: not available
Url : http://lists.samba.org/archive/samba-technical/attachments/20090217/256bc67f/boyang.vcf
Derrell Lipman
2009-02-17 22:52:26 UTC
Permalink
Post by boyang
Post by boyang
BTW: the Makefile under examples/libsmbclient seems broken,
missing some libraries. I can toggle it to build, __BUT__ I am really
not sure which ones are missed. :-)
Please provide details. I use that Makefile all the time for my
testing and have no issues, but I'm testing only under Linux. What OS
are you using, and what issues are you seeing?
I am using Linux too. It is linking error, some functions that is used
by libsmbclient are not found.(I cannot remember them exactly, one looks
like related to dictionary parse, and one looks like something in zlib,
several others...). I added "-lrt -liniparser -lnsl -lz -lcap
-lcom_err" to LDFLAGS to make it work.
Hmmm... Are you passing some non-default arguments to the configure script?
If you run configure with no arguments, or use configure.developer and
optional --enable-debug, you should find that the examples link with no
errors. Please let me know if that's not the case.

Someone has created a Makefile.internal that is built from
Makefile.internal.in. I'm not sure when that was created (I actually didn't
even realize until just now that it existed). You might try updating it with
all of the recent examples, and see if it links properly. Maybe it's time to
obsolete the manually-generated Makefile and move over to this
auto-generated one...

Derrell
boyang
2009-02-18 06:21:17 UTC
Permalink
Post by Derrell Lipman
Hmmm... Are you passing some non-default arguments to the configure script?
yes.
Post by Derrell Lipman
If you run configure with no arguments, or use configure.developer and
optional --enable-debug, you should find that the examples link with
no errors. Please let me know if that's not the case.
Someone has created a Makefile.internal that is built from
Makefile.internal.in <http://Makefile.internal.in>. I'm not sure when
that was created (I actually didn't even realize until just now that
it existed). You might try updating it with all of the recent
examples, and see if it links properly. Maybe it's time to obsolete
the manually-generated Makefile and move over to this auto-generated
one...
I didn't use the Makefile.internal. I'll try to build with default
configuration to see if they work.
Post by Derrell Lipman
Derrell
-------------- next part --------------
A non-text attachment was scrubbed...
Name: boyang.vcf
Type: text/x-vcard
Size: 187 bytes
Desc: not available
Url : http://lists.samba.org/archive/samba-technical/attachments/20090218/92358747/boyang.vcf
boyang
2009-02-19 11:02:30 UTC
Permalink
Post by Derrell Lipman
Hmmm... Are you passing some non-default arguments to the configure
script? If you run configure with no arguments, or use
configure.developer and optional --enable-debug, you should find that
the examples link with no errors. Please let me know if that's not the
case.
Hi, Derrell & Jeremy:
This is the updated version of the patch. There are two
improvements according to Derrell's comment:
* Keep the smbc_set_credentials() API untact.
* Merge smbc_set_crendentials_wrapper() into libsmb_path.c and
libsmb_server.c
Tested and worked fine for me. :-)
Pls review it, Thanks!

-------------- next part --------------
A non-text attachment was scrubbed...
Name: v1-libsmbclient-dfs-master.diff
Type: text/x-patch
Size: 7638 bytes
Desc: not available
Url : http://lists.samba.org/archive/samba-technical/attachments/20090219/9caefa29/v1-libsmbclient-dfs-master.bin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: boyang.vcf
Type: text/x-vcard
Size: 187 bytes
Desc: not available
Url : http://lists.samba.org/archive/samba-technical/attachments/20090219/9caefa29/boyang.vcf
Derrell Lipman
2009-02-19 21:42:19 UTC
Permalink
Post by boyang
This is the updated version of the patch. There are two
* Keep the smbc_set_credentials() API untact.
* Merge smbc_set_crendentials_wrapper() into libsmb_path.c and
libsmb_server.c
Tested and worked fine for me. :-)
Pls review it, Thanks!
Ok, I'm much happier with this patch. :-) I have one change I think we
should make, for consistency, and a naming change request.

1. The function smbc_set_credentials_wrapper() takes an SMBCCTX * as a
parameter. The convention in the library is that functions that take a
SMBCCTX * parameter have it as the first parameter, so please move it from
the end of the parameter list to the beginning.

2. If smbc_set_credentials_wrapper() really should be an exposed function in
the the interface (and it looks like it probably should be), we can probably
come up with a better name for it. How about either
smbc_set_credentials_auto() or smbc_set_credentials_with_fallback() ?

If you'd like to make these couple of changes and then submit a patch
generated with git-format-patch (so you are attributed), I'll commit this
for you.

Thanks!

Derrell
boyang
2009-02-20 11:22:59 UTC
Permalink
Post by Derrell Lipman
Ok, I'm much happier with this patch. :-) I have one change I think we
should make, for consistency, and a naming change request.
1. The function smbc_set_credentials_wrapper() takes an SMBCCTX * as a
parameter. The convention in the library is that functions that take a
SMBCCTX * parameter have it as the first parameter, so please move it from
the end of the parameter list to the beginning.
2. If smbc_set_credentials_wrapper() really should be an exposed function in
the the interface (and it looks like it probably should be), we can probably
come up with a better name for it. How about either
smbc_set_credentials_auto() or smbc_set_credentials_with_fallback() ?
If you'd like to make these couple of changes and then submit a patch
generated with git-format-patch (so you are attributed), I'll commit this
for you.
Hi, Derrell:
I have improved the patch as you suggested. :-) At present, I think
we can site comfortably with the patch. Once other applications rely on
libsmbclient aware of calling smbc_set_credntials() or
smbc_set_credentials_with_fallback() and call it in the authentication
callback function, we can remove smbc_set_credentials_with_fallback()
from SMBC_parse_path() and SMBC_server(), I think.
I have backported it to v3-[023], please review them. Thanks!

BoYang

-------------- next part --------------
A non-text attachment was scrubbed...
Name: v2-libsmbclient-dfs-master.diff
Type: text/x-patch
Size: 7725 bytes
Desc: not available
Url : http://lists.samba.org/archive/samba-technical/attachments/20090220/40ca0aa3/v2-libsmbclient-dfs-master.bin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: v2-libsmbclient-dfs-v3-0-test.diff
Type: text/x-patch
Size: 8727 bytes
Desc: not available
Url : http://lists.samba.org/archive/samba-technical/attachments/20090220/40ca0aa3/v2-libsmbclient-dfs-v3-0-test.bin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: v2-libsmbclient-dfs-v3-2-test.diff
Type: text/x-patch
Size: 9609 bytes
Desc: not available
Url : http://lists.samba.org/archive/samba-technical/attachments/20090220/40ca0aa3/v2-libsmbclient-dfs-v3-2-test.bin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: v2-libsmbclient-dfs-v3-3-test.diff
Type: text/x-patch
Size: 7739 bytes
Desc: not available
Url : http://lists.samba.org/archive/samba-technical/attachments/20090220/40ca0aa3/v2-libsmbclient-dfs-v3-3-test.bin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: boyang.vcf
Type: text/x-vcard
Size: 187 bytes
Desc: not available
Url : http://lists.samba.org/archive/samba-technical/attachments/20090220/40ca0aa3/boyang.vcf
Derrell Lipman
2009-02-20 21:07:17 UTC
Permalink
Post by boyang
I have improved the patch as you suggested. :-) At present, I think
we can site comfortably with the patch. Once other applications rely on
libsmbclient aware of calling smbc_set_credntials() or
smbc_set_credentials_with_fallback() and call it in the authentication
callback function, we can remove smbc_set_credentials_with_fallback()
from SMBC_parse_path() and SMBC_server(), I think.
I have backported it to v3-[023], please review them. Thanks!
I've applied your patch to master and v3-3. I think I'd like to leave it at
that to shake out any unexpected issues that might arise.

Thanks for your work on this!

Derrell
Jeremy Allison
2009-02-21 06:08:31 UTC
Permalink
Post by boyang
This is the updated version of the patch. There are two
* Keep the smbc_set_credentials() API untact.
* Merge smbc_set_crendentials_wrapper() into libsmb_path.c and
libsmb_server.c
Tested and worked fine for me. :-)
Pls review it, Thanks!
Ok, I'm much happier with this patch. :-) I have one change I think we should
make, for consistency, and a naming change request.
1. The function smbc_set_credentials_wrapper() takes an SMBCCTX * as a
parameter. The convention in the library is that functions that take a SMBCCTX
* parameter have it as the first parameter, so please move it from the end of
the parameter list to the beginning.
2. If smbc_set_credentials_wrapper() really should be an exposed function in
the the interface (and it looks like it probably should be), we can probably
come up with a better name for it. How about either smbc_set_credentials_auto()
or smbc_set_credentials_with_fallback() ?
Sorry for not getting to this sooner...

We really should make smbc_set_credentials_with_fallback()
take all const values for workgroup, user, password. We
don't want to be adding new non-const function signatures
here.

smbc_set_credentials should also have all const values
as well, but that would be an ABI change which we're
trying to avoid.

But for new functions this should be const-only.

Jeremy.
Derrell Lipman
2009-02-21 08:41:08 UTC
Permalink
Post by boyang
Post by boyang
This is the updated version of the patch. There are two
* Keep the smbc_set_credentials() API untact.
* Merge smbc_set_crendentials_wrapper() into libsmb_path.c and
libsmb_server.c
Tested and worked fine for me. :-)
Pls review it, Thanks!
Ok, I'm much happier with this patch. :-) I have one change I think we
should
Post by boyang
make, for consistency, and a naming change request.
1. The function smbc_set_credentials_wrapper() takes an SMBCCTX * as a
parameter. The convention in the library is that functions that take a
SMBCCTX
Post by boyang
* parameter have it as the first parameter, so please move it from the
end of
Post by boyang
the parameter list to the beginning.
2. If smbc_set_credentials_wrapper() really should be an exposed function
in
Post by boyang
the the interface (and it looks like it probably should be), we can
probably
Post by boyang
come up with a better name for it. How about either
smbc_set_credentials_auto()
Post by boyang
or smbc_set_credentials_with_fallback() ?
Sorry for not getting to this sooner...
We really should make smbc_set_credentials_with_fallback()
take all const values for workgroup, user, password. We
don't want to be adding new non-const function signatures
here.
smbc_set_credentials should also have all const values
as well, but that would be an ABI change which we're
trying to avoid.
But for new functions this should be const-only.
Hi Jeremy,

I have no problem making those parameters const, but it shouldn't make any
difference. In fact, we can change the parameters in smbc_set_credentials()
to const too without changing the ABI. Adding const to the parameters is
simply a debugging tool to ensure that we don't inadvertently write to the
user's buffers (which may not be writable). It's allowable to pass a
non-const parameter to a function that expects a const. It's just not
allowable to pass a const parameter to a function that does not specify
const since the function then has the option of writing to that buffer.
Therefore if we had declared it originally const and then considered
removing the const in the function definition and having our function write
to the pointed-to buffer, we'd have a problem, because then we couldn't be
sure that applications were passing writable buffers.

In short, adding const to these parameters only prevents the future
developer of these functions from messing with callers' buffers. It has no
effect now (as long as we're not already scribbling on our callers' buffers)
and it has no effect on the callers themselves going in the direction of no
const to specifying const.

Derrell

Continue reading on narkive:
Loading...