Re: [PATCH 0/9][btrfs-progs] Remove unused dirstream variable

[Goffredo Baroncelli <kreijack@inwind.it>]

On 08/02/2024 18.28, Goffredo Baroncelli wrote:
> On 07/02/2024 11.16, David Sterba wrote:
>> On Sat, Dec 30, 2023 at 12:20:54PM +0100, Goffredo Baroncelli wrote:
>>> On 14/12/2023 17.17, David Sterba wrote:
>>>> On Sat, Dec 09, 2023 at 07:53:20PM +0100, Goffredo Baroncelli wrote:
>>>>> From: Goffredo Baroncelli <kreijack@inwind.it>
>>>>>
>>>>> For historical reason, the helpers [btrfs_]open_[file_or_]dir() work with
>>>>> directory returning the 'fd' and a 'dirstream' variable returned by
>>>>> opendir(3).
>>>>>
>>>>> If the path is a file, the 'fd' is computed from open(2) and
>>>>> dirstream is set to NULL.
>>>>> If the path is a directory, first the directory is opened by opendir(3), then
>>>>> the 'fd' is computed using dirfd(3).
>>>>> However the 'dirstream' returned by opendir(3) is left open until 'fd'
>>>>> is not needed anymore.
>>>>>
>>>>> In near every case the 'dirstream' variable is not used. Only 'fd' is
>>>>> used.
>>>>
>>>> As I'm reading dirfd manual page, dirfd returns the internal file
>>>> descriptor of the dirstream and it gets closed after call to closedir().
>>>> This means if we pass a directory and want a file descriptor then its
>>>> lifetime matches the correspoinding DIR.
>>>>
>>>>> A call to close_file_or_dir() freed both 'fd' and 'dirstream'.
>>>>>
>>>>> Aim of this patch set is to getrid of this complexity; when the path of
>>>>> a directory is passed, the fd is get directly using open(path, O_RDONLY):
>>>>> so we don't need to use readdir(3) and to maintain the not used variable
>>>>> 'dirstream'.
>>>>
>>>> Does this work the same way as with the dirstream?
>>>>
>>>
>>> Hi David, are you interested in this patch ? I think that it is a
>>> great simplification.
>>
>> Sorry, I got distracted by other things.
>>
>> The patchset does not apply cleanly on 6.7 so I've used 6.6.3 as the
>> base for testing. There's one failure in the cli tests, result can be
>> seen here https://github.com/kdave/btrfs-progs/actions/runs/7813042695/job/21311365019
>>
>> ====== RUN MUSTFAIL /home/runner/work/btrfs-progs/btrfs-progs/btrfs filesystem resize 1:-128M /home/runner/work/btrfs-progs/btrfs-progs/tests/test.img
>> ERROR: not a btrfs filesystem: /home/runner/work/btrfs-progs/btrfs-progs/tests/test.img
>> failed (expected): /home/runner/work/btrfs-progs/btrfs-progs/btrfs filesystem resize 1:-128M /home/runner/work/btrfs-progs/btrfs-progs/tests/test.img
>> no expected error message in the output 2
>> test failed for case 003-fi-resize-args
>>
>> I think it's related to the changes to dirstream, however I can't
>> reproduce it locally, only on the github actions CI.
>>
>> Unlike other commands in the test, this one is called on an image and it
>> must fail to prevent accidentaly resizing underlying filesystem.
> 
> 
> I will rebase on 6.7 and I will try to reproduce the problem. Could you share the full script, or point me where is it ?

Ignore my latter request. I found the test code.

> 
> BR

-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5