PATCH: context based libsmbclient

[Andrew Bartlett]

Tom Jansen wrote:
> 
> > > > > OK, I hope to look at it today ... Thanks ...
> >
> > Firstly, this patch looks good, thankyou *very* much for your work on
> > it.
> 
> You're _very_ welcome. I'm glad I can help the samba team.

Always CC the list - others may also have comments.  (I've CCed it for
this reply)

> > I do however have some comments:
> >
> > I don't like the seperation between SMBCCTX * smbc_new_context(void)
> > and
> > smbc_init_context().  I think that smbc_init_context() should do the
> > new_context implicitly - it should not be exposed as part of the
> > interface.
> 
> This will bring in a nasty problem. There a quite a few options that can be set
> via the contents of the SMBCCTX struct. Init can do some checking checking on
> those set by the user and correct some if required. However, if there is no
> distinction between new() and init() the user has to set the values after the
> init() call and we will need many extra checks in the rest of the code for NULL
> pointers etc.
> IMHO we should keep the two functions separated. I do not like introducting many
> extra checks in the functions that provide the actual functionality. If SMBCCTX
> * context->_initialized == 1, then we should be able to assert that all
> callbacks are set, workgroup != NULL, user != NULL, etc.
> 
> Another way to make sure everything is correct is passing all the
> user-configurable values as arguments to the init() functions. This greatly
> reduces the expandability (if that's a word). I'm planning to add some more
> configurable parameters like the timeout to wait for a connection and the domain
> you suggested below to the context soon. And this will break all programs
> immediatly if passed as a function argument.

Yes, I normally prefer the 'make this with these properties' style - but
for an external library I can certainly see your point.  

> > + * @note            Do not forget to smbc_init_context() the returned
> > SMBCCTX pointer !
> > + */
> > +int smbc_free_context(SMBCCTX * context);
> > +
> >
> > Firstly, that @note is a cut-n-paste bug.
> 
> Oops!
> 
> > But while I like this (don't free if still used) I think we need a
> > 'shutdown' interface, so programs have somthing to call on their way out
> > (becouse otherwise programs with just exit() and let the OS clean up
> > - not the best way...).
> 
> I'll change the interface: smbc_free_context(SMBCCTX * context, int shutdown)
> if (shutdown) {
>    close_all_files
>    shutdown_all_server_connections
>    delete_cache
> }
> 
> This way you can do polite cleanups and aggressive ones. I think this is a nice
> feature. The aggressive ones will render all SMBCFILE and SMBCDIR pointer
> useless, but it should not cause any segfaults because of the DLIST_CONTAINS()
> checks.
> 
> > Are we really allowed to twiddle with errno?  If malloc failed, didn't
> > the OS fill in the correct error number?  (Which might not be ENOMEM
> > for
> > some very werid circumstance).
> 
> I do not have much reference material, but on Linux (glibc) errno is not set on
> malloc failure. I do not know about other platforms though.
> I always fiddle with errno in my io-library work.

OK, but if you could check I would appriciate it.  

> >
> > -       pstrcpy(workgroup, lp_workgroup());
> > +       /* HELP !!! Which workgroup should I use ? Or are they always
> > the same -- Tom */
> > +       pstrcpy(workgroup, ocontext->workgroup);
> >
> > This needs work.  The workgroup here actually needs to be specified by
> > the user - becouse its used to figure out what domain the user belongs
> > to, and is used by the server in authenticaion.  If it doesn't match
> > the
> > server's domain or one of its trusted domains, it is *assumed* to be
> > the
> > server's domain (so that works fine for most cases) but if the user is
> > in a trusted domain, it breaks.
> 
> This is actually specified by the user. The workgroup in ocontext->workgroup is
> the default value set by the programmer. You cut just a little to much in the
> patch. The next line contains a smbc_server() call which calls
> context->callbacks.auth_fn() on authorization need. Auth_fn() is allowed to
> change the workgroup too.
> 
> > The 'get username/password' stuff needs to take a 'domain' argument.
> > We might also allow that domain to be in the username (as
> > domain\username), but I don't like these hacks.
> 
> I'll add it to the auth_fn. This adds some compatibilty issues. If an old
> program has the auth_fn type used as a variable somewhere it breaks. Should we
> break compatibility or just add auth_fn2 ? This also needs some changes in the
> cache-lookup. If the domain is set we need to match on that too.
> Shouldn't be too much of a hassle.

hmm - considering the compatibility issues, I think we might just use
the current (implicit) case, where domain\user is actually handled by
the cli_session_setup() code internally.  But I don't really like it. 
Hmm...

> > +off_t smbc_lseek(int fd, off_t offset, int whence)
> > +{
> > +       SMBCFILE * file = (SMBCFILE *) fd;
> > +       return statcont->lseek(statcont, file, offset, whence);
> > +}
> > +
> >
> > While I like the idea, I think a linear serch of the files in the
> > linked list would be better.  sizeof(int) != sizeof(void *) on some
> > platforms.
> 
> I do not think a linear search is possible because of the variable size(think
> close()). We should add "magic" fd table for this. Your favourite :  )

I honestly didn't expect yout to get both the context basis and the file
struct in the same swoop.  Well done - for a hack this is very neat. 
However I sill don't see why we can't use the linked list of open
files.  Just create an atomicly incresing counter and make that the
'file number', then use that when searching the linked list.  

> 2>
> >
> > +int smbc_stat(const char *url, struct stat *st)
> > +{
> > +               return statcont->stat(statcont, url, st);
> > +}
> >
> > This might just be diff/space/tab interaction, but watch that indent.
> 
> Just two tabs in emacs. I'll indent-region all the code before the next patch.
> >
> > Again, thanks for the patch, it looks pretty good.
> Thanks for the comments. I'll fix what needs to be fixed.

Thanks!

Andrew Bartlett

-- 
Andrew Bartlett                                 abartlet at pcug.org.au
Manager, Authentication Subsystems, Samba Team  abartlet at samba.org
Student Network Administrator, Hawker College   abartlet at hawkerc.net
http://samba.org     http://build.samba.org     http://hawkerc.net