| Message ID | 20200712144419.21457-4-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@ideasonboard.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart@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. > + flags |= O_CREAT; > > - fd_ = ::open(name_.c_str(), flags); > + fd_ = ::open(name_.c_str(), flags, 0666); > if (fd_ < 0) { > error_ = -errno; > return false; >
Hi Laurent, Thanks for your patch. On 2020-07-12 17:44:19 +0300, Laurent Pinchart wrote: > When a file is opened in WriteOnly or ReadWrite mode, create it if it > doesn't exist. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > 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) > + flags |= O_CREAT; > > - fd_ = ::open(name_.c_str(), flags); > + fd_ = ::open(name_.c_str(), flags, 0666); > if (fd_ < 0) { > error_ = -errno; > return false; > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@ideasonboard.com> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@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. > > + flags |= O_CREAT; > > > > - fd_ = ::open(name_.c_str(), flags); > > + fd_ = ::open(name_.c_str(), flags, 0666); > > if (fd_ < 0) { > > error_ = -errno; > > return false; > >
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@ideasonboard.com> >> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@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; >>> >
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) + flags |= O_CREAT; - fd_ = ::open(name_.c_str(), flags); + fd_ = ::open(name_.c_str(), flags, 0666); if (fd_ < 0) { error_ = -errno; return false;
When a file is opened in WriteOnly or ReadWrite mode, create it if it doesn't exist. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/file.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)