On Thu, 2006-12-21 at 15:28 +0100, Jens Axboe wrote:
> On Thu, Dec 21 2006, Ming Zhang wrote:
> > On Thu, 2006-12-21 at 14:59 +0100, Jens Axboe wrote:
> > > On Thu, Dec 21 2006, Ming Zhang wrote:
> > > > On Thu, 2006-12-21 at 08:42 +0100, Jens Axboe wrote:
> > > > > On Wed, Dec 20 2006, Ming Zhang wrote:
> > > > > > On Wed, 2006-12-20 at 14:09 -0600, Seetharami Seelam wrote:
> > > > > > >
> > > > > > > On 12/20/06, Ming Zhang <[EMAIL PROTECTED]> wrote:
> > > > > > > On Wed, 2006-12-20 at 17:45 +0100, Jens Axboe wrote:
> > > > > > > > On Wed, Dec 20 2006, Ming Zhang wrote:
> > > > > > > > >
> > > > > > > > > On Wed, 2006-12-20 at 14:02 +0100, Jens Axboe wrote:
> > > > > > > > > > On Wed, Dec 20 2006, Jens Axboe wrote:
> > > > > > > > > > > On Wed, Dec 20 2006, Jens Axboe wrote:
> > > > > > > > > > > > Irk, do_pipe() needs to know what the input is,
> > > > > > > obviously. Lemme add
> > > > > > > > > > > > that as well.
> > > > > > > > > > >
> > > > > > > > > > > This has a better chance of working, still not
> > > > > > > tested
> > > > > > > though. If you can
> > > > > > > > > > > test, I'll commit it once we have it working.
> > > > > > > > > >
> > > > > > > > > > It works for me, just tested it. Patch committed.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > yes, works. thx.
> > > > > > > > >
> > > > > > > > > now the fifo will use a name pattern like foo. but
> > > > > > > regular
> > > > > > > file will use
> > > > > > > > > foo.blktrace.X and if you give full name, blkparse
> > > > > > > does
> > > > > > > not report any
> > > > > > > > > warning or error, just return 0 results. this drove me
> > > > > > > nuts before i
> > > > > > > > > read the code.
> > > > > > > > >
> > > > > > > > > [ [EMAIL PROTECTED] blktrace]$ ./blkparse
> > > > > > > a.a.blktrace.0
> > > > > > > > > WARNING: full file name given. Should give trace name
> > > > > > > foo,
> > > > > > > > > instead of file name foo.blktrace.x
> > > > > > > >
> > > > > > > > Better would be to just fix it up for the user, the
> > > > > > > current
> > > > > > > setup is a
> > > > > > > > little un-intuitive. Care to patch that up? :-)
> > > > > > > >
> > > > > > >
> > > > > > > are these what u want?
> > > > > > >
> > > > > > > * user supplied a foo, we automatically match it with
> > > > > > > foo.blktrace.X and
> > > > > > > open.
> > > > > > > * user supplied a foo.blktrace.X, we do not add extra
> > > > > > > blktace.X and open
> > > > > > > it directly.
> > > > > > >
> > > > > > > then if user have foo.blktrace.0 and foo.blktrace.1,
> > > > > > > current
> > > > > > > code works
> > > > > > > when use "-i foo". then shall we support "-i
> > > > > > > foo.blktrace.0"
> > > > > > > and open
> > > > > > > foo.blktrace.1 automatically?
> > > > > > >
> > > > > > >
> > > > > > > Perhaps, if user explicity supplies the name(s), you
> > > > > > > should
> > > > > > > open just that (those) file(s). May be you should open all
> > > > > > > when the name is supplied as foo.blktrace.*
> > > > > >
> > > > > > key point here is not the implementation difficulty, but how we
> > > > > > decide a
> > > > > > consistent rule. i agree with the rules u set, if i understand
> > > > > > correctly.
> > > > > >
> > > > > > * if "-", then read from stdin;
> > > > > > * if file name is foo.blktrace.*, then we try to open all;
> > > > > > * all other file name pattern, we open only _one_ file with _exact_
> > > > > > file
> > > > > > name.
> > > > > >
> > > > > > See if others like this.
> > > > >
> > > > > I think that is the best approach, if the case where the full name is
> > > > > given we check and warn if other CPU files are there. It could just
> > > > > be a
> > > > > pilot error, and we should warn in that case.
> > > > >
> > > > > "You specified file foo.blktrace.0 and files from other CPUs also
> > > > > exist.
> > > > > blkparse will only read the given file, which may not be what you
> > > > > want.
> > > > > Use 'foo' as the filename to read all saved data."
> > > > >
> > > > > or something to that effect.
> > > >
> > > > ok. i will give it a try.
> > >
> > > On 2nd thought, is it _ever_ a good idea not to read all the files? The
> > > message will whiz by and nobody will ever see it, they will just get
> > > partial (and bad) data. So probably that case of giving foo.blktrace.x
> > > should just work like doing foo.
> > >
> >
> > let blkparse to do all the fuzzy logic to try to open all files, will
> > increase the code complexity and might still miss one or two.
> >
> > we can add a name check function to check name pattern, if can not pass
> > the check, simply not to go ahead. users would like to get accurate data
> > by paying some attention to command input. so suggest the rule to be
> >
> > * - as stdin
> > * foo is a fifo, then open it
> > * foo is a regular file, then open foo.blktrace.X,
> > * other print a error msg to describe this rule and stop.
> >
> > so people have a clear msg on what to do and will not get any wrong
> > data.
>
> That's fine with me, I'm not suggesting an elaborate scheme. I just want
> to prevent people accidentally doin foo.blktrace.0 from missing events
> from CPUs 1...N. So:
>
> * - for stdin
>
> * 'x' is a fifo, open that.
>
> * foo and foo.blktrace.[0...N] exists, open foo.blktrace.[0...N]
>
> Up until now, that is no change from what blkparse currently does, I'm
> just describing it. So the new rule I'm proposing is:
>
> * foo.blktrace.0 given, and foo.blktrace.[1...N] exists, print a warning
> and add those files.
>
> If someone really wants to use only that file (obscure case, I cannot
> imagine a real world scenario where that is the case. The weird HT
> experimentation case that Seelam gave is easy - just delete the damn
> file, it wont even be valid anymore since the first file is
> overwritten), then they can use '-' as input and cat the file.
>
no warning is given. add silently for 4th.
diff --git a/blkparse.c b/blkparse.c
index 6bb5308..11e81c6 100644
--- a/blkparse.c
+++ b/blkparse.c
@@ -2307,9 +2307,22 @@ static int handle(struct ms_stream *msp)
return 1;
}
+static int name_check(char *name)
+{
+ char *b;
+
+ if (!name)
+ return -EINVAL;
+
+ b = strstr(name, ".blktrace.");
+ if (b)
+ *b = '\0';
+ return 0;
+}
+
static int do_file(void)
{
- int i, cpu;
+ int i, cpu, res;
struct per_dev_info *pdi;
/*
@@ -2317,6 +2330,9 @@ static int do_file(void)
*/
for (i = 0; i < ndevices; i++) {
pdi = &devices[i];
+ res = name_check(pdi->name);
+ if (res)
+ return res;
for (cpu = 0; setup_file(pdi, cpu); cpu++)
;
}
-
To unsubscribe from this list: send the line "unsubscribe linux-btrace" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
