From: Will C. <wc...@re...> - 2003-12-08 19:32:13
|
Is there any particular reason that /dev/oprofile/dump is only writeable by root? Earlier incarnations of OProfile had the dump file writeable by anyone, so that normal users could force a dump of the profiling data. Could /dev/oprofile/dump be made world writeable and OProfile have an opdump script again? This would be useful in the situation where the administrator sets up the machine in the default time-based sampling and the normal user could dump the data to make sure that they got all the samples for the executable they are testing out. Otherwise the user has to have run opcontrol as root or just hope all the samples are dumped from the kernel. -Will |
From: Will C. <wc...@re...> - 2004-03-02 23:37:55
Attachments:
userdump7.diff
oproffs.patch
|
I have reworked the patch for opcontrol to minimize the stuff done for the user dump by factoring out code that is common betweeen the root and normal user. I also removed the unneeded check of "OPROFILE_AVAILABLE" in the data dump. The comment that check for the presences of the daemon has be changed to reflect that the check doesn't verify that the daemon is actually running. The oproffs.patch changes the /dev/oprofile/dump to be world writeable. The combination of the two patches should allow normal users to flush sample data. -Will |
From: Philippe E. <ph...@wa...> - 2004-03-03 07:19:31
|
On Tue, 02 Mar 2004 at 18:32 +0000, Will Cohen wrote: > I have reworked the patch for opcontrol to minimize the stuff done for > the user dump by factoring out code that is common betweeen the root and > normal user. I also removed the unneeded check of "OPROFILE_AVAILABLE" > in the data dump. The comment that check for the presences of the daemon > has be changed to reflect that the check doesn't verify that the daemon > is actually running. > > The oproffs.patch changes the /dev/oprofile/dump to be world writeable. > The combination of the two patches should allow normal users to flush > sample data. > > -Will > +do_dump_data() > { > - # make sure that the daemon is running > + # make sure that the daemon is not dead and gone > if test -e "$DIR/lock"; then > OPROFILED_PID=`cat $DIR/lock` > if test ! -d "/proc/$OPROFILED_PID"; then > @@ -1073,14 +1084,14 @@ > # find current time > TMPFILE=`mktemp /tmp/oprofile.XXXXXX` || exit 1 > echo 1 > $MOUNT/dump > - # loop until there is a file to check > - while [ ! -e "$DIR/complete_dump" ] > - do > - sleep 1; > - done > - # loop until modification data of $MOUNT/dump after TMPFILE > - while [ "$TMPFILE" -nt "$DIR/complete_dump" ] > + # loop until there is a file to check and > + # the modification data of $MOUNT/dump after TMPFILE > + while [ \( ! -e "$DIR/complete_dump" \) -a \( "$TMPFILE" -nt "$DIR/complete_dump" \) ] I think we need -o here else if complete_dump doesn't exists we silently do nothing. > do > + if test ! -d "/proc/$OPROFILED_PID"; then here we know daemon was running (or a dangling lock file existed) can we add an. echo "dump fail: either daemon died during last run or dies during dump" in some case we can get twice error message but this is exceptional and usefull for us. > + rm $TMPFILE > + return 1 > + fi > sleep 1; > done > rm $TMPFILE > @@ -1092,6 +1103,18 @@ > return 0; > } > > +# do_dump > +# returns 0 if successful > +# exits if unsuccessful > +do_dump() > +{ if [ $OPROFILE_AVAILABLE != "yes" ]; then echo "dump: daemon not running, nothing to do" return 0 fi I don"t remember if we can't do that here, in this case we know the daemon is running and can tweak error message by removing the one I suggested in do_dump and tweak the next echo. > + do_dump_data > + if test $? -ne 0 -a "$ONLY_DUMP" = "yes"; then > + echo "Unable to complete dump of oprofile data" >& 2 > + exit 1; > + fi > + return 0; > +} > +#determine which module is loaded > +check_version() > +{ > + OPROFILE_AVAILABLE=no > + grep oprofilefs /etc/mtab >/dev/null > + if test "$?" -eq 0; then > + # need to have oprofilefs mounted for this to work on 2.6 > + KERNEL_SUPPORT=yes > + OPROFILE_AVAILABLE=yes > + return > + fi > + # need to have /proc/oprof available for this to work on 2.4 > + grep oprof /proc/devices >/dev/null > + if test "$?" -eq 0; then > + KERNEL_SUPPORT=no > + OPROFILE_AVAILABLE=yes > + return > + fi > +} > + > + > # main > > # determine the location of opcontrol and related programs > @@ -1252,8 +1292,17 @@ > fi > > if test "$UID" != "0"; then > - echo "Must be root to use oprofile." >&2 > + if test "$1" = "--dump" -a -z "$2"; then We can't assume the kernel use the version allowing a user dump, we must test it else someone using 2.3 driver with 0.8.0 can get an access denied and loop forever in the wait for complete_dump change > + check_version > + do_init_daemon_vars > + decide_oprofile_device > + ONLY_DUMP=yes > + do_dump > + exit 0; > + else > + echo "Normal users are limited to '--dump'." >&2 > exit 1 > + fi > fi > > load_module I've not checked if this will work with 2.4 (can we do a user dump with 2.4 ?) regards, Phil |
From: Will C. <wc...@re...> - 2004-03-03 17:25:44
Attachments:
userdump8.diff
|
Philippe Elie wrote: > On Tue, 02 Mar 2004 at 18:32 +0000, Will Cohen wrote: > > >>I have reworked the patch for opcontrol to minimize the stuff done for >>the user dump by factoring out code that is common betweeen the root and >>normal user. I also removed the unneeded check of "OPROFILE_AVAILABLE" >>in the data dump. The comment that check for the presences of the daemon >>has be changed to reflect that the check doesn't verify that the daemon >>is actually running. >> >>The oproffs.patch changes the /dev/oprofile/dump to be world writeable. >>The combination of the two patches should allow normal users to flush >>sample data. >> >>-Will > > >>+do_dump_data() >> { >>- # make sure that the daemon is running >>+ # make sure that the daemon is not dead and gone >> if test -e "$DIR/lock"; then >> OPROFILED_PID=`cat $DIR/lock` >> if test ! -d "/proc/$OPROFILED_PID"; then >>@@ -1073,14 +1084,14 @@ >> # find current time >> TMPFILE=`mktemp /tmp/oprofile.XXXXXX` || exit 1 >> echo 1 > $MOUNT/dump >>- # loop until there is a file to check >>- while [ ! -e "$DIR/complete_dump" ] >>- do >>- sleep 1; >>- done >>- # loop until modification data of $MOUNT/dump after TMPFILE >>- while [ "$TMPFILE" -nt "$DIR/complete_dump" ] >>+ # loop until there is a file to check and >>+ # the modification data of $MOUNT/dump after TMPFILE >>+ while [ \( ! -e "$DIR/complete_dump" \) -a \( "$TMPFILE" -nt "$DIR/complete_dump" \) ] > > > I think we need -o here else if complete_dump doesn't exists we silently > do nothing. Yes, -a should be -o. > >> do >>+ if test ! -d "/proc/$OPROFILED_PID"; then > > > here we know daemon was running (or a dangling lock file existed) can we > add an. > > echo "dump fail: either daemon died during last run or dies during dump" > > in some case we can get twice error message but this is exceptional > and usefull for us. No problem giving additional details that oprofiled is no longer around. > > > >>+ rm $TMPFILE >>+ return 1 >>+ fi >> sleep 1; >> done >> rm $TMPFILE >>@@ -1092,6 +1103,18 @@ >> return 0; >> } >> >>+# do_dump >>+# returns 0 if successful >>+# exits if unsuccessful >>+do_dump() >>+{ > > > if [ $OPROFILE_AVAILABLE != "yes" ]; then > echo "dump: daemon not running, nothing to do" > return 0 > fi > > I don"t remember if we can't do that here, in this case we know the daemon > is running and can tweak error message by removing the one I suggested in > do_dump and tweak the next echo. > > >>+ do_dump_data >>+ if test $? -ne 0 -a "$ONLY_DUMP" = "yes"; then >>+ echo "Unable to complete dump of oprofile data" >& 2 >>+ exit 1; >>+ fi >>+ return 0; >>+} > > > > >>+#determine which module is loaded >>+check_version() >>+{ >>+ OPROFILE_AVAILABLE=no >>+ grep oprofilefs /etc/mtab >/dev/null >>+ if test "$?" -eq 0; then >>+ # need to have oprofilefs mounted for this to work on 2.6 >>+ KERNEL_SUPPORT=yes >>+ OPROFILE_AVAILABLE=yes >>+ return >>+ fi >>+ # need to have /proc/oprof available for this to work on 2.4 >>+ grep oprof /proc/devices >/dev/null >>+ if test "$?" -eq 0; then >>+ KERNEL_SUPPORT=no >>+ OPROFILE_AVAILABLE=yes >>+ return >>+ fi >>+} >>+ >>+ >> # main >> >> # determine the location of opcontrol and related programs >>@@ -1252,8 +1292,17 @@ >> fi >> >> if test "$UID" != "0"; then >>- echo "Must be root to use oprofile." >&2 >>+ if test "$1" = "--dump" -a -z "$2"; then > > > We can't assume the kernel use the version allowing a user dump, we must > test it else someone using 2.3 driver with 0.8.0 can get an access denied > and loop forever in the wait for complete_dump change I have added a check to make sure that the write to $MOUNT/dump was successful. > > >>+ check_version >>+ do_init_daemon_vars >>+ decide_oprofile_device >>+ ONLY_DUMP=yes >>+ do_dump >>+ exit 0; >>+ else >>+ echo "Normal users are limited to '--dump'." >&2 >> exit 1 >>+ fi >> fi >> >> load_module > > > I've not checked if this will work with 2.4 (can we do a user dump with 2.4 ?) All the Red Hat distro use a back port of the 2.6 mechanism, so I haven't tried it on 2.4. > > regards, > Phil |
From: Philippe E. <ph...@wa...> - 2004-03-03 19:04:32
|
On Wed, 03 Mar 2004 at 12:20 +0000, Will Cohen wrote: A last bit and you can commit it. I'll send the driver part when 2.6.4 will be out (too late for 2.6.4) > All the Red Hat distro use a back port of the 2.6 mechanism, so I > haven't tried it on 2.4. dump is 0666 access mode on 2.4 it should be ok. > +do_dump() > +{ > + do_dump_data > + if test $? -ne 0 -a "$ONLY_DUMP" = "yes"; then > + echo "Unable to complete dump of oprofile data" >& 2 in normal condition (daemon not dead in previous run etc.) this can occur only if profiler is not running so something like is more appropriate: echo -n "Unable to complete dump of oprofile data: are you sure profiling is on ?" >& 2 Feel free to reword it if there is a better way. regards, Phil |
From: John L. <le...@mo...> - 2004-03-21 00:57:36
|
On Wed, Mar 03, 2004 at 12:20:04PM -0500, Will Cohen wrote: > if test ! -d "/proc/$OPROFILED_PID"; then > + echo "dump fail: either daemon died during last run or dies during dump" >&2 > return 1; Please fix the message. Error messages in opcontrol have a capital letter at the start. The English could do with a little tidying too. > # determine the location of opcontrol and related programs > @@ -1252,8 +1298,17 @@ > fi > > if test "$UID" != "0"; then > - echo "Must be root to use oprofile." >&2 > + if test "$1" = "--dump" -a -z "$2"; then > + check_version > + do_init_daemon_vars Rather than two call sites, why not just call check_version/do_init_daemon_vars once before this if/else ? If you fix these trivialities, then it looks OK to apply to me. Phil will have to take care of the kernel patch regards john -- "Spammers get STABBED by GOD." - Ron Echeverri |
From: Will C. <wc...@re...> - 2004-03-22 15:37:57
Attachments:
opcontrol_message.diff
|
John Levon wrote: > On Wed, Mar 03, 2004 at 12:20:04PM -0500, Will Cohen wrote: > > >> if test ! -d "/proc/$OPROFILED_PID"; then >>+ echo "dump fail: either daemon died during last run or dies during dump" >&2 >> return 1; > > > Please fix the message. Error messages in opcontrol have a capital > letter at the start. The English could do with a little tidying too. Attached patch attempts to tidy the error messages. >> # determine the location of opcontrol and related programs >>@@ -1252,8 +1298,17 @@ >> fi >> >> if test "$UID" != "0"; then >>- echo "Must be root to use oprofile." >&2 >>+ if test "$1" = "--dump" -a -z "$2"; then >>+ check_version >>+ do_init_daemon_vars > > > Rather than two call sites, why not just call > check_version/do_init_daemon_vars once before this if/else ? A normal user can't load the module and the mondule needs to be loaded to know what version to use. Rather than trying to put additional checking in load_module to handle things differently for a normal user the minimal stuff required for supporting the dump was split out. -Will > If you fix these trivialities, then it looks OK to apply to me. Phil > will have to take care of the kernel patch > > regards > john |
From: John L. <le...@mo...> - 2004-03-22 15:56:29
|
On Mon, Mar 22, 2004 at 10:37:48AM -0500, Will Cohen wrote: > Attached patch attempts to tidy the error messages. Fine thanks. > >>if test "$UID" != "0"; then > >>- echo "Must be root to use oprofile." >&2 > >>+ if test "$1" = "--dump" -a -z "$2"; then > >>+ check_version > >>+ do_init_daemon_vars > > > > > >Rather than two call sites, why not just call > >check_version/do_init_daemon_vars once before this if/else ? > > A normal user can't load the module and the mondule needs to be loaded > to know what version to use. Rather than trying to put additional > checking in load_module to handle things differently for a normal user > the minimal stuff required for supporting the dump was split out. Hmm one of us isn't following the other. You have code like this check_version() { ... } do_init_daemon_vars() { ... } do_init() { ... check_version do_init_daemon_vars ... } <------ A if uid != 0; then check_version do_init_daemon_Vars else do_init fi Why can't the calls to check_version and do_init_daemon_vars by moved to the point labelled by A ? regards john |
From: Will C. <wc...@re...> - 2004-03-22 16:35:02
Attachments:
opcontrol_factor.diff
|
John Levon wrote: > On Mon, Mar 22, 2004 at 10:37:48AM -0500, Will Cohen wrote: > > >>Attached patch attempts to tidy the error messages. > > > Fine thanks. > > >>>>if test "$UID" != "0"; then >>>>- echo "Must be root to use oprofile." >&2 >>>>+ if test "$1" = "--dump" -a -z "$2"; then >>>>+ check_version >>>>+ do_init_daemon_vars >>> >>> >>>Rather than two call sites, why not just call >>>check_version/do_init_daemon_vars once before this if/else ? >> >>A normal user can't load the module and the mondule needs to be loaded >>to know what version to use. Rather than trying to put additional >>checking in load_module to handle things differently for a normal user >>the minimal stuff required for supporting the dump was split out. > > > Hmm one of us isn't following the other. You have code like this > > check_version() > { ... } > do_init_daemon_vars() > { ... } > > do_init() > { > ... > check_version > do_init_daemon_vars > ... > } > > <------ A > if uid != 0; then > check_version > do_init_daemon_Vars > else > do_init > fi > > > Why can't the calls to check_version and do_init_daemon_vars by moved to > the point labelled by A ? You are right that that do_init_daemon_vars can be factored out and placed at "A". Moving the check_version to A won't help as much. The load_module would have to be before check_version. Moving the load_module earlier requires a test around it to make sure that it was only being run by root because of the modprobes. Something like the attached? > > regards > john |
From: John L. <le...@mo...> - 2004-03-22 16:56:31
|
On Mon, Mar 22, 2004 at 11:34:58AM -0500, Will Cohen wrote: > of the modprobes. Something like the attached? Looks great regards john |
From: Philippe E. <ph...@wa...> - 2003-12-08 22:21:24
|
Will Cohen wrote: > Is there any particular reason that /dev/oprofile/dump is only writeable > by root? Earlier incarnations of OProfile had the dump file writeable by > anyone, so that normal users could force a dump of the profiling data. > Could /dev/oprofile/dump be made world writeable and OProfile have an > opdump script again? This would be useful in the situation where the > administrator sets up the machine in the default time-based sampling and > the normal user could dump the data to make sure that they got all the > samples for the executable they are testing out. Otherwise the user has > to have run opcontrol as root or just hope all the samples are dumped > from the kernel. John, I think it's usefull. I've a small preference to fix opcontrol --dump whilst Will think it's better to add an opdump script (opcontrol change are not so easy). Any comments ? regards, Phil |
From: Will C. <wc...@re...> - 2003-12-10 14:32:56
|
The oproffs.patch is the a patch for the linux 2.6 kernel that sets the perms to allow a normal user to trigger a dump. The kernel patch also corrects the permissions for status files that should be read only. I have code up a opdump script to provide provide just the dump operation for the normal user. userdump3.patch is a patch that allows a normal user to use "opcontrol --dump", if the user attempts other operations it warns of a problem. I still lean towards having a separate opdump script. However, either one should work. There is one significant change in the logic for do_dump routine for both of the scripts. The loop check to make sure that the daemon is still exists. There have been times during development that the daemon would die, e.g. kernel missing lookup_dcookie syscall, and the do_dump would hang. If the daemon disappears the do_dump exits immediately. Comments on the proposed changes. -Will Philippe Elie wrote: > Will Cohen wrote: > >> Is there any particular reason that /dev/oprofile/dump is only >> writeable by root? Earlier incarnations of OProfile had the dump file >> writeable by anyone, so that normal users could force a dump of the >> profiling data. Could /dev/oprofile/dump be made world writeable and >> OProfile have an opdump script again? This would be useful in the >> situation where the administrator sets up the machine in the default >> time-based sampling and the normal user could dump the data to make >> sure that they got all the samples for the executable they are testing >> out. Otherwise the user has to have run opcontrol as root or just hope >> all the samples are dumped from the kernel. > > > > John, I think it's usefull. I've a small preference to fix > opcontrol --dump whilst Will think it's better to add an > opdump script (opcontrol change are not so easy). Any comments ? > > regards, > Phil > |
From: John L. <le...@mo...> - 2003-12-10 14:51:12
|
On Wed, Dec 10, 2003 at 09:32:48AM -0500, Will Cohen wrote: > I have code up a opdump script to provide provide just the dump > operation for the normal user. userdump3.patch is a patch that allows a > normal user to use "opcontrol --dump", if the user attempts other I'd prefer the latter. I still don't really get why it's useful to allow the user to dump without being root though. They can't do any other control as non-root. > + else > + echo "Normal user can only use '--dump'." >&2 This is somewhat misleading. regards john |
From: Will C. <wc...@re...> - 2003-12-10 15:31:50
|
John Levon wrote: > On Wed, Dec 10, 2003 at 09:32:48AM -0500, Will Cohen wrote: > > >>I have code up a opdump script to provide provide just the dump >>operation for the normal user. userdump3.patch is a patch that allows a >>normal user to use "opcontrol --dump", if the user attempts other > > > I'd prefer the latter. > > I still don't really get why it's useful to allow the user to dump > without being root though. They can't do any other control as non-root. Not everyone has root access on a machine. The system adminstrator could set oprofile for a reasonable default for time-based measurement such as "opcontrol --setup --vmlinux=/boot/vmlinuz-`uname -r` --separate=library; opcontrol --start". OProfile in this case is running as a replacement for traditional gprof data collection. The users need to make sure that samples are flushed to the sample files before they do the analysis (analysis can be performed as a normal user). Giving regular users root access just to dump data is undesirable. >>+ else >>+ echo "Normal user can only use '--dump'." >&2 > > > This is somewhat misleading. How about "Normal users are limited to the '--dump' option." -Will |
From: Will C. <wc...@re...> - 2004-02-11 17:02:08
Attachments:
userdump4.diff
oproffs.patch
|
John Levon wrote: > On Wed, Dec 10, 2003 at 09:32:48AM -0500, Will Cohen wrote: > > >>I have code up a opdump script to provide provide just the dump >>operation for the normal user. userdump3.patch is a patch that allows a >>normal user to use "opcontrol --dump", if the user attempts other > > > I'd prefer the latter. > > I still don't really get why it's useful to allow the user to dump > without being root though. They can't do any other control as non-root. > > >>+ else >>+ echo "Normal user can only use '--dump'." >&2 > > > This is somewhat misleading. > > regards > john This should make it easier for normal users on systems to use OProfile as a replacement for gprof. The user can ensure that the data in the sample files is up to data and reflects recently run programs. I made the message more informative. + echo "Normal users are limited to '--dump'." >&2 The code in opcontrol also avoid hanging if for some reason the daemon is no longer around. The oproffs.patch will be needed to be put in the kernel to make the /dev/oprofile/dump a user-writeable file. I applied the patch to the current oprofile checkout, built oprofile, and ran with a 2.6 kernel patched with the oproffs.patch. The code compiled and worked as expected. -Will |
From: John L. <le...@mo...> - 2004-02-11 19:51:33
|
On Wed, Feb 11, 2004 at 12:01:19PM -0500, Will Cohen wrote: > +check_version() > +{ > + > + > + if test "$1" = "--dump" -a -z "$2"; then > + check_version > + if test "$OPROFILE_AVAILABLE" != "yes"; then Why are we doing the check like this ? Shouldn't we just abstract out the test at the top of do_dump ? Specifically you don't actually see if the daemon *is* ruinning, just whether we have oprofile. And it's more code. john |
From: Will C. <wc...@re...> - 2004-02-12 18:13:03
Attachments:
userdump6.diff
|
John Levon wrote: > On Wed, Feb 11, 2004 at 12:01:19PM -0500, Will Cohen wrote: > > >>+check_version() >>+{ >>+ >>+ >>+ if test "$1" = "--dump" -a -z "$2"; then >>+ check_version >>+ if test "$OPROFILE_AVAILABLE" != "yes"; then > > > Why are we doing the check like this ? Shouldn't we just abstract out > the test at the top of do_dump ? You're referring to the OPROFILE_AVAILABLE test? I have moved that into the function that does the actual data dump. The revised patch also factors out the error message code. The revised patch is attached. > Specifically you don't actually see if the daemon *is* ruinning, just > whether we have oprofile. And it's more code. You are correct, this just check there is a daemon process around. It could be stopped, a zombie, or somehow unresponsive. The code catches the case where forcing a dump causes the daemon to die. However, this is an improvement over the current code which can hang if the daemon goes away while the dump is occuring. -Will |
From: Philippe E. <ph...@wa...> - 2004-02-12 19:05:57
|
On Thu, 12 Feb 2004 at 13:11 +0000, Will Cohen wrote: > >Specifically you don't actually see if the daemon *is* ruinning, just > >whether we have oprofile. And it's more code. > > You are correct, this just check there is a daemon process around. It > could be stopped, a zombie, or somehow unresponsive. The code catches > the case where forcing a dump causes the daemon to die. However, this is > an improvement over the current code which can hang if the daemon goes > away while the dump is occuring. agreed it's *very* boring opcontrol --dump hang if daemon die especially when tracking bug in daemon ... What about this alternative (not tested) actually we do: if test -e "$DIR/lock"; then OPROFILED_PID=`cat $DIR/lock` if test ! -d "/proc/$OPROFILED_PID"; then return 1; fi else return 1; fi while [ ! -e "$DIR/complete_dump" ] do sleep 1; done while [ "$TMPFILE" -nt "$DIR/complete_dump" ] do sleep 1; done can't we add the test for a living daemon in both while condition ? if test -e "$DIR/lock"; then OPROFILED_PID=`cat $DIR/lock` if test ! -d "/proc/$OPROFILED_PID"; then return 1; fi else return 1; fi while [ ! -e "$DIR/complete_dump" -a ! -d "/proc/$OPROFILED_PID" ] do sleep 1; done while [ "$TMPFILE" -nt "$DIR/complete_dump" -a ! -d "/proc/$OPROFILED_PID"] do sleep 1; done I dunno the right syntax but you get the figure. (yes it can fail if daemon die and the pid is re-used ...) regards, Phil |
From: Will C. <wc...@re...> - 2004-02-12 22:43:57
|
Philippe Elie wrote: > On Thu, 12 Feb 2004 at 13:11 +0000, Will Cohen wrote: > > >>>Specifically you don't actually see if the daemon *is* ruinning, just >>>whether we have oprofile. And it's more code. >> >>You are correct, this just check there is a daemon process around. It >>could be stopped, a zombie, or somehow unresponsive. The code catches >>the case where forcing a dump causes the daemon to die. However, this is >>an improvement over the current code which can hang if the daemon goes >>away while the dump is occuring. > > > agreed it's *very* boring opcontrol --dump hang if daemon die especially > when tracking bug in daemon ... > > What about this alternative (not tested) > > actually we do: > > if test -e "$DIR/lock"; then > OPROFILED_PID=`cat $DIR/lock` > if test ! -d "/proc/$OPROFILED_PID"; then > return 1; > fi > else > return 1; > fi > while [ ! -e "$DIR/complete_dump" ] > do > sleep 1; > done > while [ "$TMPFILE" -nt "$DIR/complete_dump" ] > do > sleep 1; > done > > can't we add the test for a living daemon in both while condition ? I think the follow case is taken care of in the patch. The loops are merged into one loop with the test for the OPROFILED_PID inside the loop body. > if test -e "$DIR/lock"; then > OPROFILED_PID=`cat $DIR/lock` > if test ! -d "/proc/$OPROFILED_PID"; then > return 1; > fi > else > return 1; > fi > while [ ! -e "$DIR/complete_dump" -a ! -d "/proc/$OPROFILED_PID" ] > do > sleep 1; > done > while [ "$TMPFILE" -nt "$DIR/complete_dump" -a ! -d "/proc/$OPROFILED_PID"] > do > sleep 1; > done Here is what is currently in the patch: while [ \( ! -e "$DIR/complete_dump" \) -a \( "$TMPFILE" -nt "$DIR/complete_dump" \) ] do if test ! -d "/proc/$OPROFILED_PID"; then rm $TMPFILE return 1 fi sleep 1; done > I dunno the right syntax but you get the figure. (yes it can fail if daemon > die and the pid is re-used ...) Yes, it could fail if the PID is reused. However, the machine would need to be going through the PID quickly to reuse the same PID within on second if the previous processing using it dying. -Will |
From: John L. <le...@mo...> - 2004-02-13 00:56:58
|
On Thu, Feb 12, 2004 at 01:11:32PM -0500, Will Cohen wrote: > >the test at the top of do_dump ? > > You're referring to the OPROFILE_AVAILABLE test? I have moved that into > the function that does the actual data dump. The revised patch also > factors out the error message code. The revised patch is attached. No ! I'm referring to check_version existing *at all*. It's bullshit. We are supposed to be testing whether the daemon is running before we try a --dump. So how about doing that ? > +#determine which module is loaded > +check_version() > +{ > + OPROFILE_AVAILABLE=no > + grep oprofilefs /etc/mtab >/dev/null > + if test "$?" -eq 0; then > + # need to have oprofilefs mounted for this to work on 2.6 > + KERNEL_SUPPORT=yes > + OPROFILE_AVAILABLE=yes > + return > + fi > + # need to have /proc/oprof available for this to work on 2.4 > + grep oprof /proc/devices >/dev/null > + if test "$?" -eq 0; then > + KERNEL_SUPPORT=no > + OPROFILE_AVAILABLE=yes > + return > + fi > +} This give some random irrelevant information. Nobody cares about this at this point. We only care if --dump will succeed. john -- "Spammers get STABBED by GOD." - Ron Echeverri |