[libcamera-devel] [PATCH 4/4] libcamera: file: Create the file on open() if it doesn't exist

[Kieran Bingham]

On 13/07/2020 12:57, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Sun, Jul 12, 2020 at 08:51:41PM +0100, Kieran Bingham wrote:
>> Hi Laurent,
>>
>> On 12/07/2020 15:44, Laurent Pinchart wrote:
>>> When a file is opened in WriteOnly or ReadWrite mode, create it if it
>>> doesn't exist.
>>
>> Indeed, this solved my issue when using this series before.
>>
>> Thanks!
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>> ---
>>>  src/libcamera/file.cpp | 9 ++++++---
>>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/libcamera/file.cpp b/src/libcamera/file.cpp
>>> index 8bd109090919..77701666e438 100644
>>> --- a/src/libcamera/file.cpp
>>> +++ b/src/libcamera/file.cpp
>>> @@ -148,8 +148,9 @@ bool File::exists() const
>>>   * \param[in] mode The open mode
>>>   *
>>>   * This function opens the file specified by fileName() in \a mode. If the file
>>> - * doesn't exist and the mode is WriteOnly or ReadWrite, this
>>> - * function will attempt to create the file.
>>> + * doesn't exist and the mode is WriteOnly or ReadWrite, this function will
>>> + * attempt to create the file with initial permissions set to 0666 (modified by
>>> + * the process' umask).
>>>   *
>>>   * The error() status is updated.
>>>   *
>>> @@ -163,8 +164,10 @@ bool File::open(File::OpenMode mode)
>>>  	}
>>>  
>>>  	int flags = (mode & ReadWrite) - 1;
>>> +	if (mode & WriteOnly)
>>
>> This looks a bit awkward, as it's actually WriteOnly or ReadWrite (i.e.
>> it's not 'Write .. Only') because we know the bit will be set in both
>> cases ... but that is detailed above in the function comment anyway, so
>> this is fine, and I'm sure if anyone was looking they'd figure out it
>> works for RW files too :)
>>
>> In otherwords, it's only the use of the word 'only' in the enum that's
>> awkward, but not something worthy of a change of the enum.
> 
> Should we instead have File::OpenMode::Read and File::OpenMode::Write,
> and left the user combine those ? If I had been creating this from
> scratch that's likely what I would have done, but ReadOnly, WriteOnly
> and ReadWrite are established terms.

I don't think it matters now, and as I said this doesn't warrant
changing the enum now.

It works, it's commented, ship it ;-)

--
Kieran

>>> +		flags |= O_CREAT;
>>>  
>>> -	fd_ = ::open(name_.c_str(), flags);
>>> +	fd_ = ::open(name_.c_str(), flags, 0666);
>>>  	if (fd_ < 0) {
>>>  		error_ = -errno;
>>>  		return false;
>>>
> 

-- 
Regards
--
Kieran