[PATCH] s3: vfs: time_audit: fix handling of token_blob in smb_time_audit_offload_read_recv()
Ralph Wuerthner
ralphw at de.ibm.com
Fri Aug 10 07:50:36 UTC 2018
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;
}
>> Dump question: wouldn't it make sense to run all self tests with
>> vfs_time_audit enabled as this module is by definition a no-op?
>
> Good question. At least in master, the "simpleserver" environment
> already includes time_audit. The gap is that smb2.ioctl is not running
> against the simpleserver.
>
> Attached are two patches, one is a proposed shorter fix, the other one is
> for running smb2.ioctl also against simpleserver. If you only apply the
> second patch, you can easily see the failing test:
>
> ./configure --picky-developer --enable-selftest
> make -j
> make test TESTS=smb2.ioctl
>
> If you agree with the fix, then we should open a bugzilla and also
> backport the fix to 4.8 (i have not check 4.7 yet).
>
> Christof
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?
Regards
Ralph
More information about the samba-technical
mailing list