[PATCH] Final removal of lp_posix_pathnames() from the smbd server main code paths.

[Uri Simchoni]

Done!

To summarize:
- I have a couple of cosmetic comments on #5, but it's otherwise RB+ me.
- Cannot ack #13 - seems like a "product decision" because it changes
behavior no to support old buggy clients, plus if we do change behavior
it seems we can delete the "buggy client support code" rather than
change it. (see separate message on thread)

Otherwise RB+ me.

On 03/24/2016 12:02 AM, Uri Simchoni wrote:
> I started reviewing it, here's where I am (note the one comment):
> 
> On 03/23/2016 06:13 PM, Jeremy Allison wrote:
>> On Tue, Mar 22, 2016 at 08:47:15AM -0700, Jeremy Allison wrote:
>>> On Sun, Mar 20, 2016 at 09:31:40PM -0700, Jeremy Allison wrote:
>>>> This is the final removal of all the
>>>> lp_posix_pathnames() globals from the
>>>> SMB1/2/3 server code paths (except for
>>>> the per-request lookup in the SMB1 server).
>>>>
>>>> After this patchset we should be able
>>>> to add per-handle based unix extensions
>>>> to the SMB2 server without running into
>>>> any issues with the lp_posix_pathnames() global.
>>>>
>>>> Passes local make test.
>>>>
>>>> Patset is:
>>>>
>>>> 1). Fix vfs_afsacl.c module build (missing struct
>>>> smb_filename conversion).
>>>>
> RB+ me
> 
>>>> 2). vfs_afsacl.c - move to using STAT directly
>>>> (if it was a symlink path, we already refused it).
>>>>
> RB+ me
> 
>>>> 3). Move lp_posix_pathnames() out of ea_list_has_invalid_name()
>>>> utility function.
>>>>
> RB+ me
> 
>>>> 4). Add uint32_t flags field to struct smb_filename. Only
>>>> defined field currently is SMB_FILENAME_POSIX_PATH.
>>>>
> still reviewing....
> 

RB+ me

>>>> 5). Add uint32_t flags parameter to synthetic_smb_fname().
>>>> Touches a lot of files but is mostly boilerplate, copying
>>>> the 'flags' field from an available smb_filename struct.
> still reviewing.... (count is right, now need to go through each one)
>>>>

I have some comments on this patch, none of them seem to actually point
to a bug so I leave it to you whether to fix or not.

In general reviewing this one proved harder than originally anticipated,
because if the flags are not copied, we should justify why they are 0. I
used the heuristic that if the user of the smb_fname knows what it's
doing with it at the posix level (i.e. it's doing a stat or an lstat or
mkdir etc) then it's safe to pass 0, because "below the VFS" only
readdir_fn cares whether it's a POSIX or NT name (to make the proper
stat) - as can be seen in patch #6.

This means that we may encounter bugs down the road if a future VFS
module implementing unlink or opendir or mkdir cares whether the
smb_fname is POSIX or NT, and relies on the flags.

A couple of cosmetic comments:

> @@ -3816,7 +3825,8 @@ static void fruit_copy_chunk_done(struct tevent_req *subreq)
>                         req,
>                         state->dst_fsp->fsp_name->base_name,
>                         streams[i].name,
> -                       NULL);
> +                       NULL,
> +                       state->src_fsp->fsp_name->flags);
>                 if (tevent_req_nomem(dst_fname_tmp, req)) {
>                         TALLOC_FREE(src_fname_tmp);
>                         return;

Should this be state->dst_fsp->fsp_name->flags? (yeah I know they are
probably the same but it looks cleaner)

> @@ -299,7 +299,8 @@ static bool recycle_create_dir(vfs_handle_struct *handle, const char *dname)
>                         smb_fname = synthetic_smb_fname(talloc_tos(),
>                                                 new_dir,
>                                                 NULL,
> -                                               NULL);
> +                                               NULL,
> +                                               0);
>                         if (smb_fname == NULL) {
>                                 goto done;
>                         }
Maybe the flags here better come from the unlink smb_fname?


> 
>>>> 6). Remove use of lp_posix_pathnames() below the VFS.
>>>> Removes an optimization if it's done on a symlink.
>>>>
> RB+ me
> 
>>>> 7). posix_acls.c - move to using STAT directly
>>>> (if it was a symlink path, we already refused it).
>>>>
> RB+ me
> 
>>>> 8). Remove unneeded lp_posix_pathnames() check in
>>>>  SMB2 create.
>>>>
> RB+ me
> 
>>>> 9). Remove many common uses of lp_posix_pathnames().
>>>> We can check smb_fname->flags now.
>>>>
> Still reviewing (one hunk to go)
RB+ me

There's a recurring theme of "if posix do lstat else do stat" which we
probably want to tuck into it's own function in a later patch.
> 
>>>> 10). vfs_recycle.c - Remove use of vfs_stat_smb_basename().
> RB+ me
> 
>>>>
>>>> 11). vfs_acl_tdb.c - Remove use of vfs_stat_smb_basename().
> RB+ me
> 
>>>>
>>>> 12). Modify vfs_stat_smb_basename() to take a
>>>>  const struct smb_filename *. Allows last use of
>>>> lp_posix_pathnames() below the VFS to be removed.
> RB+ me
> 
>>>>
>>>> 13). Remove lp_posix_pathnames() from msdfs code.
>>>> Last lp_posix_pathnames() in the SMB1/2/3 code paths.
> Not sure I quite get this one - lp_posix_pathnames() is derived from a
> protocol handshake and we're replacing it by a heuristic?
Can't ack this one - see thread.

>>>>
>>>> 14). torture. Remove spurious lp_posix_pathnames().
>>>> Cut-and-paste error from long ago.
> RB+ me
> 
>>>>
>>>> Please review and push if happy !
>>>
>>> Updated version containing the flag definition
>>> change requested by Ralph.
>>>
>>> The flags really are different things though,
>>> as they apply to different objects within the
>>> core server - so the names should be different.
>>> But at least the values are now consistent.
>>
>> I know people are really busy with other More.Important.Things. :-(
>> right now.
>>
>> But for those who aren't, it'd be really great to get this in
>> as then I can start prototyping the SMB2 unix extensions !
>>
>> Cheers,
>>
>> Jeremy.
>>
> 
>