On Tue, Mar 14, 2017 at 2:00 AM, Dmitry V. Levin <l...@altlinux.org> wrote: > On Tue, Mar 14, 2017 at 12:40:46AM +0530, Abhishek Tiwari wrote: >> > Per-file summary is not in GNU change log format, please refer to >> > https://www.gnu.org/prep/standards/html_node/Style-of-Change-Logs.html . >> > Specifically, it lacks asterisks at the beginning of each new file >> > description and has excess spaces between file part and description, >> > otherwise looks fine. >> > >> > The patch wires up {,l}stat{,64}, fstatat/fstatat64 syscalls (used for >> > obtaining >> > file status), ustat syscall (which is a deprecated way to get some FS >> > statistics) and some statfs syscalls (which are more contemporary way >> > obtaining FS statistics information). On the other side, at least >> > fstat/fstat64, fstatfs/fstatfs64, old{,f,l}stat, osf_{,f}statfs, and >> > various >> > (mostly unsupported) syscalls with osf_, svr4_, sysv_, bsd43_, and posix_ >> > prefixes, present on alpha and mips, are omitted. I'm not sure whether >> > it was intended. >> > >> > There are minor tabulation irregularities introduced (at least) for >> > newfstatat and fstatat64 syscall entries, it is better to avoid this. >> >> I have fixed the commit entries. >> Now the patch includes all the stat-like syscalls that can be >> displayed by following command-> > > The idea is not to throw all syscalls that do some kind of stat into a > single class, but to create a class of syscalls that could be useful. > As Eugene already noted, there is little use of grouping syscalls that > stat files with those that stat filesystems. > > Actually, there is a software project that could benefit from more > fine-grained classes, e.g. oldstat+stat+stat64, oldlstat+lstat+lstat64, > and newfstatat+fstatat64.
Ok, so oldstat+stat+stat64 is one class that is to be grouped under %stat, oldstat+lstat+lstat64 as another class and fstatat+newfstatat+fstatat64 and some other class. I read in mailing list it is intended to group syscalls that perform same action across different versions. But it is little confusing to decide what to group into one class. > There may be some use of a class that groups all filesystem stat syscalls > like ustat and *statfs*. This might appear to be easier for you to > implement, especially the part that tests it. I would be doing this part first. Grouping ustat and *statfs* under %statfs. Is this class name fine? >> grep -nr stat linux/*/syscallent* > > btw, why do you pass -nr to this grep command? This grep command is just to display the places where I have made the change. The command actually used is mentioned in commit. >> >> --- a/syscall.c >> >> +++ b/syscall.c >> >> @@ -77,6 +77,7 @@ >> >> #define TS TRACE_SIGNAL >> >> #define TM TRACE_MEMORY >> >> #define TSC TRACE_SCHED >> >> +#define TST TRACE_STAT >> > Tabulation is used for separating macro declaration and definition, but >> > neighbouring lines utilise space for this purpose. >> >> Please elaborate, i don't get what correction is intended here? > > The rule of thumb is simple: > when patching a piece of code, follow its coding style. > > If the code uses spaces, use spaces. If it uses tabs, use tabs. > strace has a long history with a lot of contributors so the style > used in different parts might differ slightly. Sorry, that space was not intensional. >> >> +++ b/tests/stat_like.test >> >> @@ -0,0 +1,58 @@ >> >> +#!/bin/sh >> >> + >> >> +# Check how stat-like syscalls are traced. >> >> +# >> >> +# Copyright (c) 2017 The strace developers. >> >> +# All rights reserved. >> >> +# >> >> +# Redistribution and use in source and binary forms, with or without >> >> +# modification, are permitted provided that the following conditions >> >> +# are met: >> >> +# 1. Redistributions of source code must retain the above copyright >> >> +# notice, this list of conditions and the following disclaimer. >> >> +# 2. Redistributions in binary form must reproduce the above copyright >> >> +# notice, this list of conditions and the following disclaimer in the >> >> +# documentation and/or other materials provided with the distribution. >> >> +# 3. The name of the author may not be used to endorse or promote >> >> products >> >> +# derived from this software without specific prior written >> >> permission. >> >> +# >> >> +# THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR >> >> +# IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED >> >> WARRANTIES >> >> +# OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. >> >> +# IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT, >> >> +# INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, >> >> BUT >> >> +# NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF >> >> USE, >> >> +# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY >> >> +# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT >> >> +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE >> >> OF >> >> +# THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. >> >> + >> >> +STAT_TESTS='21 fstat >> >> +33 ustat >> >> +32 statx >> >> +17 statfs >> >> +21 oldfstat' >> >> + >> >> +NON_STAT_TESTS='11 fchdir >> >> +27 futex >> >> +10 fsync' >> >> + >> >> +. "${srcdir=.}/init.sh" >> >> + >> >> +echo "$STAT_TESTS" | while read w i >> >> +do >> >> + run_prog ./$i > /dev/null >> >> + run_strace -a$w -e%stat ./$i > "$EXP" >> >> + match_diff "$LOG" "$EXP" >> >> +done >> >> + >> >> +echo '+++ exited with 0 +++' > "$EXP" >> >> + >> >> +echo "$NON_STAT_TESTS" | while read w i >> >> +do >> >> + run_prog ./$i > /dev/null >> >> + run_strace -a$w -e%stat ./$i > /dev/null >> >> + match_diff "$LOG" "$EXP" >> >> +done >> >> + >> >> +rm "$EXP" >> > This test produces the following diagnostics: >> > >> > 1,2c1,2 >> > < fstat(0, 0x7f4dc9b2ffe0) = -1 EFAULT (Bad address) >> > < fstat(0, {st_dev=makedev(9, 1), st_ino=8761399, >> > st_mode=S_IFREG|0640, st_nlink=1, st_uid=1000, st_gid=1000, >> > st_blksize=4096, st_blocks=0, st_size=43147718418, >> > st_atime=1969-12-31T21:59:17+0100.000000135, >> > st_mtime=1969-12-31T21:59:19+0100.000000246, >> > st_ctime=2017-03-13T07:03:55+0100.962268762}) = 0 >> > --- >> > > stat("/etc/localtime", {st_mode=S_IFREG|0644, st_size=2246, ...}) = 0 >> > > stat("/etc/localtime", {st_mode=S_IFREG|0644, st_size=2246, ...}) = 0 >> > stat_like.test: failed test: ../strace -a21 -e%stat ./fstat output >> > mismatch >> > >> > It exits with status 0, however. >> >> Please help me with this part, how can I run this test on my machine. > > Do you ask about running this particular test? I think it's as simple as > $ make check TESTS=stat_like VERBOSE=1 Thank you, I just never did testing this way and I am new to this part. > > -- > ldv > > ------------------------------------------------------------------------------ > Check out the vibrant tech community on one of the world's most > engaging tech sites, Slashdot.org! http://sdm.link/slashdot > _______________________________________________ > Strace-devel mailing list > Strace-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/strace-devel > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ Strace-devel mailing list Strace-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/strace-devel