| Message ID | mvmk1w9lreq.fsf@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: mvmk1w9lreq.fsf@suse.de
Subject: [Qemu-devel] [PATCH] linux-user: implement renameat2
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
t [tag update] patchew/1516611841-5526-1-git-send-email-anton.nefedov@virtuozzo.com -> patchew/1516611841-5526-1-git-send-email-anton.nefedov@virtuozzo.com
* [new tag] patchew/mvmk1w9lreq.fsf@suse.de -> patchew/mvmk1w9lreq.fsf@suse.de
Switched to a new branch 'test'
59d79798ac linux-user: implement renameat2
=== OUTPUT BEGIN ===
Checking PATCH 1/1: linux-user: implement renameat2...
WARNING: architecture specific defines should be avoided
#21: FILE: linux-user/syscall.c:602:
+#if defined(__NR_renameat2)
ERROR: spaces required around that '*' (ctx:WxV)
#27: FILE: linux-user/syscall.c:608:
+ int newfd, const chat *new, int flags)
^
ERROR: braces {} are necessary for all arms of this statement
#29: FILE: linux-user/syscall.c:610:
+ if (flags == 0)
[...]
ERROR: braces {} are necessary for all arms of this statement
#50: FILE: linux-user/syscall.c:8368:
+ if (!p || !p2)
[...]
+ else
[...]
ERROR: line over 90 characters
#54: FILE: linux-user/syscall.c:8372:
+ target_to_host_bitmask(arg5, fcntl_flags_tbl)));
total: 4 errors, 1 warnings, 45 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
Le 22/01/2018 à 18:44, Andreas Schwab a écrit : > This is needed for new architectures like RISC-V which do not provide any > other rename-like syscall. > > Signed-off-by: Andreas Schwab <schwab@suse.de> > --- > linux-user/syscall.c | 33 +++++++++++++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index 5e54889522..12ca06c65a 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -598,6 +598,23 @@ static int sys_utimensat(int dirfd, const char *pathname, > #endif > #endif /* TARGET_NR_utimensat */ > > +#ifdef TARGET_NR_renameat2 > +#if defined(__NR_renameat2) > +#define __NR_sys_renameat2 __NR_renameat2 > +_syscall5(int, sys_renameat2, int, oldfd, const char *, old, int, newfd, > + const char *, new, unsigned int, flags) > +#else > +static int sys_renameat2(int oldfd, const char *old, > + int newfd, const chat *new, int flags) > +{ > + if (flags == 0) > + return renameat(oldfd, old, newfd, new); Please fix style problem reported by patchew (or ./scripts/checkpatch.pl) > + errno = ENOSYS; > + return -1; > +} > +#endif > +#endif /* TARGET_NR_renameat2 */ > + > #ifdef CONFIG_INOTIFY > #include <sys/inotify.h> > > @@ -8342,6 +8359,22 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, > } > break; > #endif > +#if defined(TARGET_NR_renameat2) > + case TARGET_NR_renameat2: > + { > + void *p2; > + p = lock_user_string(arg2); > + p2 = lock_user_string(arg4); > + if (!p || !p2) > + ret = -TARGET_EFAULT; > + else > + ret = get_errno(sys_renameat2(arg1, p, arg3, p2, > + target_to_host_bitmask(arg5, fcntl_flags_tbl))); You can't use fcntl_flags_tbl (because it converts fcntl/open flags O_*). I think you can provide directly arg5 to sys_renameat2() because flags are the same for all architectures (and value is already byte-swapped). Thanks, Laurent
Le 22/01/2018 à 18:44, Andreas Schwab a écrit : > This is needed for new architectures like RISC-V which do not provide any > other rename-like syscall. > > Signed-off-by: Andreas Schwab <schwab@suse.de> > --- > linux-user/syscall.c | 33 +++++++++++++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index 5e54889522..12ca06c65a 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -598,6 +598,23 @@ static int sys_utimensat(int dirfd, const char *pathname, > #endif > #endif /* TARGET_NR_utimensat */ > > +#ifdef TARGET_NR_renameat2 > +#if defined(__NR_renameat2) > +#define __NR_sys_renameat2 __NR_renameat2 > +_syscall5(int, sys_renameat2, int, oldfd, const char *, old, int, newfd, > + const char *, new, unsigned int, flags) > +#else > +static int sys_renameat2(int oldfd, const char *old, > + int newfd, const chat *new, int flags) "const char *" would look better. And, please, test it (build/run). Thanks, Laurent
On Jan 23 2018, Laurent Vivier <laurent@vivier.eu> wrote: > Please fix style problem reported by patchew > (or ./scripts/checkpatch.pl) This was mostly copy-pasted from surrounding code. :-) >> @@ -8342,6 +8359,22 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, >> } >> break; >> #endif >> +#if defined(TARGET_NR_renameat2) >> + case TARGET_NR_renameat2: >> + { >> + void *p2; >> + p = lock_user_string(arg2); >> + p2 = lock_user_string(arg4); >> + if (!p || !p2) >> + ret = -TARGET_EFAULT; >> + else >> + ret = get_errno(sys_renameat2(arg1, p, arg3, p2, >> + target_to_host_bitmask(arg5, fcntl_flags_tbl))); > > You can't use fcntl_flags_tbl Of course! I was confused by another patch I was working on which does need such a conversion. :-/ Andreas.
On Jan 23 2018, Laurent Vivier <laurent@vivier.eu> wrote:
> And, please, test it (build/run).
This was tested by bootstrapping openSUSE Factory for RISC-V.
Andreas.
Le 23/01/2018 à 11:26, Andreas Schwab a écrit : > On Jan 23 2018, Laurent Vivier <laurent@vivier.eu> wrote: > >> And, please, test it (build/run). > > This was tested by bootstrapping openSUSE Factory for RISC-V. I have no doubt on this part, but you should test the "#else" part too, by, for instance, undefining "__NR_renameat2" locally/temporarily. Thanks, Laurent
diff --git a/linux-user/syscall.c b/linux-user/syscall.c index 5e54889522..12ca06c65a 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -598,6 +598,23 @@ static int sys_utimensat(int dirfd, const char *pathname, #endif #endif /* TARGET_NR_utimensat */ +#ifdef TARGET_NR_renameat2 +#if defined(__NR_renameat2) +#define __NR_sys_renameat2 __NR_renameat2 +_syscall5(int, sys_renameat2, int, oldfd, const char *, old, int, newfd, + const char *, new, unsigned int, flags) +#else +static int sys_renameat2(int oldfd, const char *old, + int newfd, const chat *new, int flags) +{ + if (flags == 0) + return renameat(oldfd, old, newfd, new); + errno = ENOSYS; + return -1; +} +#endif +#endif /* TARGET_NR_renameat2 */ + #ifdef CONFIG_INOTIFY #include <sys/inotify.h> @@ -8342,6 +8359,22 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, } break; #endif +#if defined(TARGET_NR_renameat2) + case TARGET_NR_renameat2: + { + void *p2; + p = lock_user_string(arg2); + p2 = lock_user_string(arg4); + if (!p || !p2) + ret = -TARGET_EFAULT; + else + ret = get_errno(sys_renameat2(arg1, p, arg3, p2, + target_to_host_bitmask(arg5, fcntl_flags_tbl))); + unlock_user(p2, arg4, 0); + unlock_user(p, arg2, 0); + } + break; +#endif #ifdef TARGET_NR_mkdir case TARGET_NR_mkdir: if (!(p = lock_user_string(arg1)))
This is needed for new architectures like RISC-V which do not provide any other rename-like syscall. Signed-off-by: Andreas Schwab <schwab@suse.de> --- linux-user/syscall.c | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+)