Cascaded VFS patch
Andrew Bartlett
abartlet at samba.org
Tue Jun 18 01:34:01 GMT 2002
Simo Sorce wrote:
>
> On Tue, 2002-06-18 at 09:05, Andrew Bartlett wrote:
> > Simo Sorce wrote:
> > > + /* Handles on dlopen() call */
> > > + smb_vfs_dl_handle *dl_handle;
> > > + void **vfs_private;
> >
> > Why a void** ?
>
> Because you really do not know what the module wants to put there, can
> be anything.
I would have a void* thats all.
> > > +typedef enum _vfs_op_layer {
> > > + SMB_VFS_LAYER_NOOP = -1, /* - For using in VFS module to indicate end of array */
> > > + /* of operations description */
> > > + SMB_VFS_LAYER_OPAQUE = 0, /* - Final level, does not call anything beyond itself */
> > > + SMB_VFS_LAYER_TRANSPARENT, /* - Normal operation, calls underlying layer after */
> > > + /* possibly changing passed data */
> > > + SMB_VFS_LAYER_LOGGER, /* - Logs data, calls underlying layer, logging does not */
> > > + /* use Samba VFS */
> > > + SMB_VFS_LAYER_SPLITTER, /* - Splits operation, calls underlying layer _and_ own facility, */
> > > + /* then combines result */
> > > + SMB_VFS_LAYER_SCANNER /* - Checks data and possibly initiates additional */
> > > + /* file activity like logging to files _inside_ samba VFS */
> > > +} vfs_op_layer;
> >
> > I'm still not conviced on this stuff.
>
> I am and this is required for a relly functional cascading mode.
> Trivial schemes will not address the needs of modules makers.
>
> >
> > SAFE_FREE()
> >
>
> ok, ok
>
> > > {"vfs object", P_STRING, P_LOCAL, &sDefault.szVfsObjectFile, handle_vfs_object, NULL, FLAG_SHARE},
> > > {"vfs options", P_STRING, P_LOCAL, &sDefault.szVfsOptions, NULL, NULL, FLAG_SHARE},
> > > + {"vfs path", P_STRING, P_LOCAL, &sDefault.szVfsPath, NULL, NULL, FLAG_SHARE},
> >
> > Why do we need a new smb.conf option? Why can't we just have full paths
> > on objects?
>
> simple, generally parameters are 1024 bytes limited, leaving out the
> path, make it both more readable and less space consuming and make u
> easy to move modules to another path, or make it parametrical.
>
> >
> > If this option does what I think it does, then it really should be
> > global.
>
> no because you can have a different path for any different share, each
> with it's own set of modules.
I'll defer on this one - its not a major issue.
> > > + for(dl_handle = conn->dl_handle; dl_handle->handle; dl_handle++) {
> > > + /* Close dlopen() handle */
> > > + done_fptr = (void (*)(connection_struct *))sys_dlsym(dl_handle->handle, dl_handle->done_method);
> > > +
> > > + if (done_fptr == NULL) {
> > > + DEBUG(3, ("No %s() symbol found in module with handle %p, ignoring\n", dl_handle->done_method, dl_handle->handle));
> > > + } else {
> > > + done_fptr(conn);
> > > + }
> > > + sys_dlclose(dl_handle->handle);
> > > + string_free(&dl_handle->done_method);
> >
> > Why a string_free()?
>
> nice catch, need to look more closely to this point.
>
> > > -static BOOL vfs_init_default(connection_struct *conn)
> > > +static void vfs_init_default(connection_struct *conn)
> > > {
> > > DEBUG(3, ("Initialising default vfs hooks\n"));
> > >
> > > memcpy(&conn->vfs_ops, &default_vfs_ops, sizeof(struct vfs_ops));
> > > - return True;
> > > + conn->dl_handle = NULL;
> > > }
> >
> > Why? I think we should return errors on failure...
>
> if the dl_handle is null we have an error.
I don't like signaling errors/status by changing a paratmer - I think it
is better to actually look at the return value. Thats the point I was
trying to make.
> > > /****************************************************************************
> > > initialise custom vfs hooks
> > > ****************************************************************************/
> > >
> > > -static BOOL vfs_init_custom(connection_struct *conn)
> > > +static BOOL vfs_init_custom(connection_struct *conn, int vfs_number, const char *vfs_object)
> > > {
> > > int vfs_version = -1;
> > > - struct vfs_ops *ops, *(*init_fptr)(int *, struct vfs_ops *);
> > > + vfs_op_tuple *ops, *(*init_fptr)(int *, const struct vfs_ops *, int);
> > > + fstring method_init_name;
> > > + fstring vfs_object_name;
> >
> > fstrings are BAD. We might use a lot of them, but *all* new code needs
> > to use
> > asprintf() etc.
>
> it really depends on where they are, but I agree in this case, we have
> to change these bits, before a commit.
>
> > I'm really not conviced that we want to maintain 'backward
> > compatibility'. It sets a precedent, and I'm not comfortable with it.
>
> I think it is important.
> we already are backward compatible for lot of things, why not modules
> that will likely be built outside the samba team and need a bit more
> stable interface OR backwards compatibility?
I just don't want to be maintaining backward compatibilty forever.
> > Watch that indenting - 8 space tabs please.
>
> oh andrew ...
I'm here to nitpick, its my job :-)
> > > + if (string_set(&vfsobj, lp_vfsobj(SNUM(conn)))) {
> > > + /* Parse passed modules specification to array of modules */
> > > + set_first_token(vfsobj);
> > > + /* We are using default separators: ' \t\r\n' */
> > > + vfs_objects = toktocliplist(&nobj, NULL);
> >
> > What are you trying here? Why can't this be a normal lp_list?
>
> yes can be, but it was not so spread when the patch has been made, and
> the right name now is str_list_ and is located in util_str.c
Thanks, I had forgotten the name change :-)
> >
> > As you can see, I'm not happy with a few things in this patch.
>
> I see :)
:-)
> >
> > idra: Can you hold off on your commit? While I don't mind (to much)
> > the vfsclient issue, these things need addressing. Yes they are simple,
> > and yes, nobody will fix them later...
>
> yes I will hold, until this problems get addressed.
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
More information about the samba-technical
mailing list