Re: [PATCH v2 6/7] rust: file: add `DeferredFdCloser`

[Benno Lossin]

On 12/12/23 10:35, Alice Ryhl wrote:
> On Mon, Dec 11, 2023 at 6:23 PM Benno Lossin <benno.lossin@xxxxxxxxx> wrote:
>>
>>>>> +        // We update the file pointer that the task work is supposed to fput.
>>>>> +        //
>>>>> +        // SAFETY: Task works are executed on the current thread once we return to userspace, so
>>>>> +        // this write is guaranteed to happen before `do_close_fd` is called, which means that a
>>>>> +        // race is not possible here.
>>>>> +        //
>>>>> +        // It's okay to pass this pointer to the task work, since we just acquired a refcount with
>>>>> +        // the previous call to `get_file`. Furthermore, the refcount will not drop to zero during
>>>>> +        // an `fdget` call, since we defer the `fput` until after returning to userspace.
>>>>> +        unsafe { *file_field = file };
>>>>
>>>> A synchronization question: who guarantees that this write is actually
>>>> available to the cpu that executes `do_close_fd`? Is there some
>>>> synchronization run when returning to userspace?
>>>
>>> It's on the same thread, so it's just a sequenced-before relation.
>>>
>>> It's not like an interrupt. It runs after the syscall invocation has
>>> exited, but before it does the actual return-to-userspace stuff.
>>
>> Reasonable, can you also put this in a comment?
> 
> What do you want me to add? I already say that it will be executed on
> the same thread.

Seems I missed that, then no need to add anything.

>>>>> +/// Represents a failure to close an fd in a deferred manner.
>>>>> +#[derive(Copy, Clone, Eq, PartialEq)]
>>>>> +pub enum DeferredFdCloseError {
>>>>> +    /// Closing the fd failed because we were unable to schedule a task work.
>>>>> +    TaskWorkUnavailable,
>>>>> +    /// Closing the fd failed because the fd does not exist.
>>>>> +    BadFd,
>>>>> +}
>>>>> +
>>>>> +impl From<DeferredFdCloseError> for Error {
>>>>> +    fn from(err: DeferredFdCloseError) -> Error {
>>>>> +        match err {
>>>>> +            DeferredFdCloseError::TaskWorkUnavailable => ESRCH,
>>>>
>>>> This error reads "No such process", I am not sure if that is the best
>>>> way to express the problem in that situation. I took a quick look at the
>>>> other error codes, but could not find a better fit. Do you have any
>>>> better ideas? Or is this the error that C binder uses?
>>>
>>> This is the error code that task_work_add returns. (It can't happen in
>>> Binder.)
>>>
>>> And I do think that it is a reasonable choice, because the error only
>>> happens if you're calling the method from a context that has no
>>> userspace process associated with it.
>>
>> I see.
>>
>> What do you think of making the Rust error more descriptive? So instead
>> of implementing `Debug` like you currently do, you print
>>
>>     $error ($variant)
>>
>> where $error = Error::from(*self) and $variant is the name of the
>> variant?
>>
>> This is more of a general suggestion, I don't think that this error type
>> in particular warrants this. But in general with Rust we do have the
>> option to have good error messages for every error while maintaining
>> efficient error values.
> 
> I can #[derive(Debug)] instead, I guess?

Hmm I thought that might not be ideal, since then you would not have the
error code, only `TaskWorkUnavailable` or `BadFd`.
But if that is also acceptable, then I would go with the derived debug.

-- 
Cheers,
Benno