diff mbox series

[v6,3/5] Add fchmodat2 fallback definition

Message ID 20240802-fchmodat2-v6-3-dcb0293979b3@suse.com
State Accepted
Headers show
Series Add fchmodat2 testing suite | expand

Commit Message

Andrea Cervesato Aug. 2, 2024, 10:59 a.m. UTC 
From: Andrea Cervesato <andrea.cervesato@suse.com>

Reviewed-by: Petr Vorel <pvorel@suse.cz>
Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
 include/lapi/stat.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Petr Vorel Aug. 2, 2024, 11:30 a.m. UTC | #1 
Hi Andrea,


> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
> ---
>  include/lapi/stat.h | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)

> diff --git a/include/lapi/stat.h b/include/lapi/stat.h
> index 3606c9eb0..8e38ecfef 100644
> --- a/include/lapi/stat.h
> +++ b/include/lapi/stat.h
> @@ -229,4 +229,20 @@ static inline int statx(int dirfd, const char *pathname, unsigned int flags,
>  # define STATX_ATTR_VERITY	0x00100000
>  #endif

> +#define SAFE_FCHMODAT2(dfd, filename, mode, flags) \
> +	safe_fchmodat2(__FILE__, __LINE__, (dfd), (filename), (mode), (flags))
> +
> +static inline int safe_fchmodat2(const char *file, const int lineno,
> +		int dfd, const char *filename, mode_t mode, int flags)
> +{
> +	int ret;
> +
> +	ret = tst_syscall(__NR_fchmodat2, dfd, filename, mode, flags);
> +	if (ret == -1)
> +		tst_brk_(file, lineno, TBROK | TERRNO, "%s(%d,%s,%d,%d) error",
> +			__func__, dfd, filename, mode, flags);
I'm sorry, I overlook that errno check is not needed. tst_syscall() does it
(see include/lapi/syscalls.h). Please remove it before merge.

With this:
Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr
Andrea Cervesato Aug. 2, 2024, 11:34 a.m. UTC | #2 
On 8/2/24 13:30, Petr Vorel wrote:
> Hi Andrea,
>
>
>> Reviewed-by: Petr Vorel <pvorel@suse.cz>
>> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
>> ---
>>   include/lapi/stat.h | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>> diff --git a/include/lapi/stat.h b/include/lapi/stat.h
>> index 3606c9eb0..8e38ecfef 100644
>> --- a/include/lapi/stat.h
>> +++ b/include/lapi/stat.h
>> @@ -229,4 +229,20 @@ static inline int statx(int dirfd, const char *pathname, unsigned int flags,
>>   # define STATX_ATTR_VERITY	0x00100000
>>   #endif
>> +#define SAFE_FCHMODAT2(dfd, filename, mode, flags) \
>> +	safe_fchmodat2(__FILE__, __LINE__, (dfd), (filename), (mode), (flags))
>> +
>> +static inline int safe_fchmodat2(const char *file, const int lineno,
>> +		int dfd, const char *filename, mode_t mode, int flags)
>> +{
>> +	int ret;
>> +
>> +	ret = tst_syscall(__NR_fchmodat2, dfd, filename, mode, flags);
>> +	if (ret == -1)
>> +		tst_brk_(file, lineno, TBROK | TERRNO, "%s(%d,%s,%d,%d) error",
>> +			__func__, dfd, filename, mode, flags);
> I'm sorry, I overlook that errno check is not needed. tst_syscall() does it
> (see include/lapi/syscalls.h). Please remove it before merge.
>
> With this:
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
>
> Kind regards,
> Petr

As far as I see, tst_syscall() only handles ENOSYS and invalid syscalls, 
so it makes sense to handle TBROK | TERRNO inside the safe_* function.

Andrea
Petr Vorel Aug. 5, 2024, 3:36 p.m. UTC | #3 
> On 8/2/24 13:30, Petr Vorel wrote:
> > Hi Andrea,


> > > Reviewed-by: Petr Vorel <pvorel@suse.cz>
> > > Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
> > > ---
> > >   include/lapi/stat.h | 16 ++++++++++++++++
> > >   1 file changed, 16 insertions(+)
> > > diff --git a/include/lapi/stat.h b/include/lapi/stat.h
> > > index 3606c9eb0..8e38ecfef 100644
> > > --- a/include/lapi/stat.h
> > > +++ b/include/lapi/stat.h
> > > @@ -229,4 +229,20 @@ static inline int statx(int dirfd, const char *pathname, unsigned int flags,
> > >   # define STATX_ATTR_VERITY	0x00100000
> > >   #endif
> > > +#define SAFE_FCHMODAT2(dfd, filename, mode, flags) \
> > > +	safe_fchmodat2(__FILE__, __LINE__, (dfd), (filename), (mode), (flags))
> > > +
> > > +static inline int safe_fchmodat2(const char *file, const int lineno,
> > > +		int dfd, const char *filename, mode_t mode, int flags)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = tst_syscall(__NR_fchmodat2, dfd, filename, mode, flags);
> > > +	if (ret == -1)
> > > +		tst_brk_(file, lineno, TBROK | TERRNO, "%s(%d,%s,%d,%d) error",
> > > +			__func__, dfd, filename, mode, flags);
> > I'm sorry, I overlook that errno check is not needed. tst_syscall() does it
> > (see include/lapi/syscalls.h). Please remove it before merge.

> > With this:
> > Reviewed-by: Petr Vorel <pvorel@suse.cz>

> > Kind regards,
> > Petr

> As far as I see, tst_syscall() only handles ENOSYS and invalid syscalls, so
> it makes sense to handle TBROK | TERRNO inside the safe_* function.

Yes, I'm sorry, I was wrong. But in that case handling ret < -1 is missing.
I have sent a patch fixing this.

Kind regards,
Petr

> Andrea
diff mbox series

Patch

diff --git a/include/lapi/stat.h b/include/lapi/stat.h
index 3606c9eb0..8e38ecfef 100644
--- a/include/lapi/stat.h
+++ b/include/lapi/stat.h
@@ -229,4 +229,20 @@  static inline int statx(int dirfd, const char *pathname, unsigned int flags,
 # define STATX_ATTR_VERITY	0x00100000
 #endif
 
+#define SAFE_FCHMODAT2(dfd, filename, mode, flags) \
+	safe_fchmodat2(__FILE__, __LINE__, (dfd), (filename), (mode), (flags))
+
+static inline int safe_fchmodat2(const char *file, const int lineno,
+		int dfd, const char *filename, mode_t mode, int flags)
+{
+	int ret;
+
+	ret = tst_syscall(__NR_fchmodat2, dfd, filename, mode, flags);
+	if (ret == -1)
+		tst_brk_(file, lineno, TBROK | TERRNO, "%s(%d,%s,%d,%d) error",
+			__func__, dfd, filename, mode, flags);
+
+	return ret;
+}
+
 #endif /* LAPI_STAT_H__ */