Bug 2308 - [patch] libsmbclient bug fixes and enhancements
Summary: [patch] libsmbclient bug fixes and enhancements
Alias: None
Product: Samba 3.0
Classification: Unclassified
Component: libsmbclient (show other bugs)
Version: 3.0.11
Hardware: All Linux
: P1 enhancement
Target Milestone: none
Assignee: Gerald (Jerry) Carter (dead mail address)
QA Contact: Samba QA Contact
Depends on:
Reported: 2005-02-04 12:14 UTC by Derrell Lipman
Modified: 2005-08-24 10:18 UTC (History)
0 users

See Also:

patch to provide the specified bug fixes and enhancements (90.13 KB, patch)
2005-02-04 12:16 UTC, Derrell Lipman
no flags Details
patch to move "options" back into context, and flags into options (13.50 KB, patch)
2005-03-11 15:21 UTC, Derrell Lipman
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Derrell Lipman 2005-02-04 12:14:15 UTC
Attached is a patch to the latest 3.0 SVN tree which features the following
bug fixes and additional capabilities.  The changes are explained here in
approximately the order in which they are found in the patch.

- libsmb/libsmbclient.c

  * My previous contribution of extended attribute functions applied primarily
    to access control lists.  These changes add the ability to set and get the
    DOS attributes.  This change was needed because simulating the DOS
    attributes with mode bits in chmod() was unreliable; i.e. for directories,
    changes to these bits were ignored entirely (since directories need to have
    the execute bits set).  It is now possible to set and get DOS attributes of
    any file or folder using the new extended attribute functions.

  * URL encoding and decoding was not done symmetrically.  Although
    smbc_opendir() and smbc_open() expected a url-encoded path (e.g. spaces
    converted to "%20", etc.), smbc_readdir() would return non-url-encoded
    paths.  There are many times when this is desired behavior, e.g. when the
    returned paths are to be presented directly to a user.  However, there are
    also many times when it would be better to have the returned strings url-
    encoded, e.g. when they will be appended to the path opened with
    smbc_opendir() and passed back to an smbc_*() function.

    ~ smbc_urldecode() and smbc_urlencode() are now publicly accessible

    ~ added an option in the context structure to specify whether or not to
      url-encode returned readdir entries.

  * In my earlier patch, currently in the 3.0 tree, I had provided a method of
    specifying which master browser to query when browsing for workgroups.
    This was to solve the problem that a master browser which has been running
    for only a short period of time does not yet know about all workgroups on
    the local network.  I had implemented a method where one could pass
    "smb://" or "smb://?mb=.any" to use, as was traditional, the first
    discovered master browser; or "smb://?mb=.all" to request the browse list
    from all discovered master browsers and return a merged list.

    Unfortunately, my implementation violated the URI syntax.  In this patch, I
    have removed the the previous implementation (following discussions a few
    months ago with crh and rsharpe) and replaced it with a choice in a set of
    options within the context structure.

  * Fixed a bug where memory was freed after having been previously freed.

  * Added the capability, again through an options selection in the context, to
    have only one connection per server.  Previously, each share would get its
    own connection.  For long-running applications, it is not necessarily
    desirable to maintain lots and lots of connections -- in fact, XP Home
    appears to stop replying if too many connections are established
    concurrently -- so instead, a single connection may be maintained with a
    tree connect being issued for each change of share use on that server.

- libsmb/cliconnect.c

  * cli_tdis() was not setting the cnum field back to -1, so there was no way
    to determine whether an existing tree connection was active.

- libsmb/libsmb_cache.c

  * more changes here pertaining to whether to reuse a connection to a server
    for multiple shares, or to establish a new connection for each share.

- include/libsmb_internal.h

  * The space for directory entries was not adequate given the new option of
    urlencoding the returned directory entries

- include/libsmbclient.h

  * mappings of DOS mode bits

  * new _smbc_options structure in context.  The capabilities are explained in
    excruciating detail, therein.

Note that all new features default to the original behavior, to prevent
backwards compatibility issues, except for the number of local master browsers
to query for the list of workgroups.  This now defaults to 3 instead of 1,
which is a reasonable compromise between getting an accurate browse list, and
the extra (really, pretty insubstantial) amount of time required to query
additional local master browsers.

I've been running with these changes for the past few months with no problems,
so I believe this to be a reasonably stable patch.
Comment 1 Derrell Lipman 2005-02-04 12:16:21 UTC
Created attachment 945 [details]
patch to provide the specified bug fixes and enhancements
Comment 2 Gerald (Jerry) Carter (dead mail address) 2005-02-09 09:01:21 UTC
assigning to richard.  I think he replied on samba-technical about this.
Comment 3 Derrell Lipman 2005-02-09 14:02:25 UTC
See also bug #2271 as a likely better method of fixing the XP Home bug in
clilist.c than what is included in my patch.
Comment 4 Gerald (Jerry) Carter (dead mail address) 2005-03-09 15:00:19 UTC
richard, if you are pressed for time.  Let me know and I'll work 
on incorporating this patch.  I'd like to see some closure on this
one for 3.0.12.  It's been hanging around for a long time.
Comment 5 Gerald (Jerry) Carter (dead mail address) 2005-03-09 16:33:32 UTC
taking over since richard is pressed for time. Will get this 
in for the next 3.0.12{pre,rc} release.
Comment 6 Gerald (Jerry) Carter (dead mail address) 2005-03-10 12:35:11 UTC
the changes to clidesc.c have already been applied.  Everything 
else applied with minimal fuzz to the current 3_0 svn tree.  
Building and reviewing the rest of the patch now.
Comment 7 Gerald (Jerry) Carter (dead mail address) 2005-03-10 14:02:53 UTC
when I build a new libsmbclient.so with these chanegs and install it
konqueror dies accessing smb://server/share (process died unexpectedly).
Any ideas?  The changes look ok.  I just want to make sure for myself
that certain things don't break.
Comment 8 Derrell Lipman 2005-03-10 14:29:55 UTC
The context structure has new options fields and is a different size.  It likely
requires a simple recompilation of Konqueror with the new libsmbclient.h file to
fix this problem.
Comment 9 Gerald (Jerry) Carter (dead mail address) 2005-03-10 16:43:37 UTC
i've applied the patch bug I moved the options struct 
from _SMBCCTX to the context->internal structure to maintain
binary compatibility.  Tested with Konqueror.  WOrks ok.
derrel, we should probably talk about this some more on the
samba-technical ml.  closing for now.
Comment 10 Derrell Lipman 2005-03-11 15:21:42 UTC
Created attachment 1027 [details]
patch to move "options" back into context, and flags into options
Comment 11 Derrell Lipman 2005-03-11 15:26:57 UTC
re-open bug to handle movement of options back into context from internals, and
flags into options
Comment 12 Gerald (Jerry) Carter (dead mail address) 2005-03-11 16:02:36 UTC
derrell, for now let's leave things as they are (in the current 3.0 tree).
This is identical to your patch but leaves the flags.  We have people using 
that one already.  It was specifically for the gnomevfs people.
Comment 13 Derrell Lipman 2005-03-11 16:28:54 UTC
But the current version in the current 3.0 tree doesn't work since the options
structure is not accessible to applications: options is still inside of
internals which is not available to an application...???  This seems to defeat
most of the purpose of the patch, doesn't it?
Comment 14 Gerald (Jerry) Carter (dead mail address) 2005-08-24 10:18:06 UTC
sorry for the same, cleaning up the database to prevent unecessary reopens of bugs.