[PATCH] samba-tool ntacl do a proper VFS connect

Jeremy Allison jra at samba.org
Mon Jan 7 12:25:05 MST 2013


On Fri, Dec 28, 2012 at 04:35:00PM +1100, Andrew Bartlett wrote:
> >From bb140b818d4f76fd51d4a72ce67869b4032fcf83 Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <abartlet at samba.org>
> Date: Wed, 10 Oct 2012 13:47:49 +1100
> Subject: [PATCH 1/6] smbd: Split create_conn_struct into a fn that does not
>  change the working dir
> 
> The python bindings do not want the current working directory changed
> during operations, so we provide two functions, one providing the
> original behaviour, and other providing the python bindings with just
> the memory allocation and initilisation stuff.

I'm perfectly fine with the function name changes, just FYI.

Firstly:

Inside the changes to

source3/printing/nt_printing.c
source3/rpc_server/srvsvc/srv_srvsvc_nt.c

you're doing whitespace reformatting of the funtion arguments when
all you're really doing is changing the call name from:

create_conn_struct() -> create_conn_struct_cwd()

I'd rather see a patch that does just that to make it really
clear what you're changing, then an additional patch that does
the whitespace changes (labelled as "whitespace changes only")
if you really want them.

There's also a logic change here that complicates the patch.

Whem splitting out create_conn_struct() into create_conn_struct_cwd()
You added this code:

        if (snum != -1) {
                connpath = talloc_string_sub(conn,
                                             connpath,
                                             "%S",
                                             lp_servicename(talloc_tos(), snum));
                if (!connpath) {
                        TALLOC_FREE(conn);
                        return NT_STATUS_NO_MEMORY;
                }
        }

from the original:

        connpath = talloc_string_sub(conn,
                                connpath,
                                "%S",
                                lp_servicename(talloc_tos(), snum));
        if (!connpath) {
                TALLOC_FREE(conn);
                return NT_STATUS_NO_MEMORY;
        }

(i.e. you added the snum != -1 check).

I can see the point of it, but when splitting a function
in two the cleaner way of doing it is to simply split it
out in one patch, then add the aditional change as a subsequent
patch.

That way it's much clearer to the reviewer what is going on
as you're not mixing logic changes with just moving code.
Small changes like this make it *much* easier to review
changes without missing anything.

The rest of the patchset looks fine to me, if you
can separate out this logic change and resubmit I'll
push to master.

Cheers,

Jeremy.


More information about the samba-technical mailing list