On Tue, Mar 14, 2017 at 8:44 AM, Abhishek Tiwari <erabhishektiwar...@gmail.com> wrote: > 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 >>
Is it fine to group statfs+ statfs64+ fstatfs + fstatfs64 + ustat as %statfs or it should be (statfs+statfs64 + ustat) and (fstatfs+ftsatfs64) i.e. two different classes ? ------------------------------------------------------------------------------ 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
