[prev in list] [next in list] [prev in thread] [next in thread] 

List:       oprofile-list
Subject:    Re: [PATCH 01/10] opcontrol: fix vecho output
From:       Maynard Johnson <maynardj () us ! ibm ! com>
Date:       2009-09-14 20:23:57
Message-ID: 4AAEA65D.5050302 () us ! ibm ! com
[Download RAW message or body]

Robert Richter wrote:
> On 04.09.09 10:29:35, Maynard Johnson wrote:
> > Robert Richter wrote:
> > > There are unreachable vecho commands in the code that are never
> > Specifically, which vecho commands are never executed?  The only one
> > I can find is in get_image_range when it is called from do_options
> > when KERNEL_RANGE is not set.
> 
> Right, this is the only one. The reason I wrote this patch was the
> parameter dump patch that follows next. However, I can't find why it
> is needed for this, maybe the originally reason disappeared in
> between.
> 
> I think the vecho implementation may lead to unexpected behavior
> because output may vary depending on when the option is parsed. So,
> when using the verbose option you *think* vecho is called and should
> print something, but it doesn't. In the end it is not usable in
> functions other than do_operations. Since opcontrol is a huge file it
> is not always obviously if vecho is already initialized or not. So
> moving the verbose option to the check_options_early is in my opinion
> a code improvement.
> 
> > Personally, I think this invocation of get_image_range is a bug,
> > since the value of KERNEL_RANGE depends on the order of the options
> > you pass in.  If you pass --kernel-range before --vmlinux option,
> > then KERNEL_RANGE is set; if you pass these options in the opposite
> > order, then KERNEL_RANGE is not set.  In do_start_daemon, we call
> > get_image_range again, which will always do the right thing since
> > all options have been processed by then.  So I think the right fix
> > is to remove the calls to get_image_range in do_setup.
> > 
> 
> Yes, this is a bug.

Right, I'll post a patch with this fix.

> 
> > Another reason why I'm not inclined to accept this patch is that if
> > you pass --verbose during setup, you don't get *all* the verbose
> > info that you would if you pass it with --start-daemon or --start
> > (e.g., options to oprofiled).  So users would probably want to still
> > pass --verbose at start time, resulting in a lot of duplicate
> > verbose information.
> 
> If there is too much verbose information the user may not want to set
> the verbose option.
Yes, my point exactly.  So for that reason, I don't think I'll commit this patch as \
the end result would be redundant verbose information displayed to the user.

Regards,
-Maynard
> 
> However, the patch is not as important to me. I think it improves the
> code and you should apply it, but you decide.
> 
> -Robert
> 


------------------------------------------------------------------------------
Come build with us! The BlackBerry&reg; Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay 
ahead of the curve. Join us from November 9&#45;12, 2009. Register now&#33;
http://p.sf.net/sfu/devconf
_______________________________________________
oprofile-list mailing list
oprofile-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/oprofile-list


[prev in list] [next in list] [prev in thread] [next in thread]