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