[PATCH] libcli: Overwrite destination in cli_smb2_rename

[ronnie sahlberg]

On Tue, Aug 5, 2014 at 2:55 PM, Jeremy Allison <jra at samba.org> wrote:
> On Tue, Aug 05, 2014 at 02:37:10PM -0700, Jeremy Allison wrote:
>>
>> However.... (email thinking in real-time here :-).
>> SMB1 trans2 SMB_FILE_RENAME_INFORMATION *does*
>> have a client supplied replace_if_exists flag,
>> the same as SMB2. So we could move to that
>> instead.
>
> So looks like we *can* do this. Now the
> question is *how* should we do this.
>
> We could create a cli_rename2() or
> cli_rename_overwrite() which has the
> desired semantics for both SMB1 and
> SMB2.
>
> Currently the code inside libsmbclient
> does:
>
>        if (!NT_STATUS_IS_OK(cli_rename(targetcli1, targetpath1, targetpath2))) {
>                 int eno = SMBC_errno(ocontext, targetcli1);
>
>                 if (eno != EEXIST ||
>                     !NT_STATUS_IS_OK(cli_unlink(targetcli1, targetpath2, FILE_ATTRIBUTE_SYSTEM | FILE_ATTRIBUTE_HIDDEN)) ||
>                     !NT_STATUS_IS_OK(cli_rename(targetcli1, targetpath1, targetpath2))) {
>
>
> so if it gets an EEXIST it falls back to
> a non-atomic unlink and rename. So
> chjanging this to remove the unlink
> and just call cli_rename_overwrite()
> directly would be a distinct improvement
> without changing the underlying
> semantics inside libsmbclient.
>
> Thoughts ?

+1 for using trans2.

It makes the rename much more atomic and  less prone to leave the
filesystem in an undeterministic state if there is a failure.
For example, if the command fails and returns an error back to the
client. Does targetpath2 still exist or not?
(APIs where on error a client can not deterministically know what the
current state is is generally sub-optimal.)


Unless you know of many users that depend on the current behavior,  I
would suggest changing it to become more like posix rename() and just
atomically overwrite.