A patch to move FSCTL handling into the VFS
Richard Sharpe
realrichardsharpe at gmail.com
Wed Sep 21 22:02:12 MDT 2011
On Wed, Sep 21, 2011 at 5:46 PM, Rusty Russell <rusty at ozlabs.org> wrote:
> On Fri, 16 Sep 2011 11:58:04 -0700, Richard Sharpe <realrichardsharpe at gmail.com> wrote:
>> 2. Is the handling of talloc correct. Ie, will these allocations get
>> cleaned up when the request completes. I used
>> talloc_size(talloc_tos(), ...) mostly.
>
> Yes, that looks wrong (I have no idea about this code, BTW, just that I
> know something about talloc).
>
> You usually want to use talloc_array, which gives you type checking:
>
>> +static NTSTATUS vfswrap_fsctl(struct vfs_handle_struct *handle,
>> + struct files_struct *fsp,
>> + uint32_t function,
>> + uint16_t req_flags, /* Needed for UNICODE ... */
>> + const char *in_data,
>> + uint32_t in_len,
>> + char **out_data,
>> + uint32_t max_out_len,
>> + uint32_t *out_len)
>> +{
> ...
>> + *out_len = (max_out_len >= 64) ? 64 : max_out_len;
>> + /* Hmmm, will this cause problems if less data asked for? */
>> + return_data = talloc_size(talloc_tos(), 64);
>> + if (return_data == NULL) {
>> + return NT_STATUS_NO_MEMORY;
>> + }
>> +
>> + /* For backwards compatibility only store the dev/inode. */
>> + push_file_id_16(return_data, &fsp->file_id);
>> + memcpy(return_data+16,create_volume_objectid(fsp->conn,objid),16);
>> + push_file_id_16(return_data+32, &fsp->file_id);
>> + *out_data = return_data;
>> + return NT_STATUS_OK;
>
> This would be neater as an explicit char array allocation:
> return_data = talloc_array(<parent>, char, 64);
>
> More importantly, the parent is wrong: you can't parent on the top of
> stack then return! Your function should take a MEM_CTX ptr: the caller
> should pass the allocation which it wants the out_data to hang from.
>
>> + shadow_data = talloc_zero(talloc_tos(),
>> + struct shadow_copy_data);
>> + if (shadow_data == NULL) {
>> + DEBUG(0,("TALLOC_ZERO() failed!\n"));
>> + reply NT_STATUS_NO_MEMORY;
>> + }
>
> This is actually OK, since you only use this variable in this function.
>> + cur_pdata = talloc_size(talloc_tos(), *out_len);
>> + if (cur_pdata == NULL) {
>> + TALLOC_FREE(shadow_data);
>> + return NT_STATUS_NO_MEMORY;
>> + }
>> +
>> + *out_data = cur_pdata;
>
> This needs a mem_ctx to hang off...
>> + *out_len = 16;
>> + out_data_tmp = talloc_size(talloc_tos, *out_len);
>
> Again...
Thanks for that additional feedback. I have already added a context
argument at Volker's suggestion (although there is legacy code in
vfs_default.c that passes back something allocated with talloc_tos() I
believe.
However, I will make the additional changes you have suggested and
redo the patch.
Thanks.
--
Regards,
Richard Sharpe
More information about the samba-technical
mailing list