[PATCH] s3: vfs: time_audit: fix handling of token_blob in smb_time_audit_offload_read_recv()

[Christof Schmitt]

On Fri, Aug 10, 2018 at 09:50:36AM +0200, Ralph Wuerthner wrote:
> On 09.08.2018 21:46, Christof Schmitt wrote:
> >Good catch! I think the intention of the code was to first use a local
> >variable and then assign the result to the pointer. That pattern is
> >common in the Samba code.
> 
> The original intention regarding variable token_blob in
> smb_time_audit_offload_read_recv() is obvious. But from a
> performance perspective this is suboptimal. In my point of view it
> doesn't make sense to allocate a new blob, fill the new blob with
> the contents of the old blob and destroy the old one. Keeping the
> old blob is much more straight forward.
> BTW: the corresponding VFS call in vfs_fruit is using the
> talloc_move() pattern too:
> 
> static NTSTATUS fruit_offload_read_recv(struct tevent_req *req,
>                                         struct vfs_handle_struct *handle,
>                                         TALLOC_CTX *mem_ctx,
>                                         DATA_BLOB *token)
> {
>         struct fruit_offload_read_state *state = tevent_req_data(
>                 req, struct fruit_offload_read_state);
>         NTSTATUS status;
> 
>         if (tevent_req_is_nterror(req, &status)) {
>                 tevent_req_received(req);
>                 return status;
>         }
> 
>         token->length = state->token.length;
>         token->data = talloc_move(mem_ctx, &state->token.data);
> 
>         tevent_req_received(req);
>         return NT_STATUS_OK;
> }

Fare enough, we can go with your version to avoid the talloc call.

> You only added smb2.ioctl to the simpleserver environmet. What about
> other smbtorture tests? Do we have a full coverage now for
> vfs_time_audit? Are there any reasons why we shouldn't add
> vfs_time_audit to the other test environments too?

The initial goal was to simply load the modules somewhere to trigger the
smb_vfs_assert_all_fns check. We can probably just load them everywhere.
I am running the attached patches through a gitlab pipeline now to see
if anything else shows up:
https://gitlab.com/samba-team/devel/samba/pipelines/27670843

Christof