Bug 114435

Summary: svclib_filesystem shell-script include file - problems with
Product: Red Hat Enterprise Linux 2.1 Reporter: Peter Giorgilli <pgiorgilli>
Component: clumanagerAssignee: Lon Hohberger <lhh>
Status: CLOSED ERRATA QA Contact:
Severity: medium Docs Contact:
Priority: medium    
Version: 2.1   
Target Milestone: ---   
Target Release: ---   
Hardware: i386   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2004-03-19 15:46:24 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Attachments:
Description Flags
Patch against 1.0.25 to fix problem
none
Patch to fix both mktemp / backslash weirdness none

Description Peter Giorgilli 2004-01-28 02:58:33 UTC
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.0.2)
Gecko/20030716

Description of problem:
startFilesystems() and stopFilesystems() shell functions

There's a bug in the error handling code relating to the creation
of temporary files:

1. If the "mktemp" should fail, as determined by testing whether
the shell variable "TMPFILE" is empty, the "logandPrint" function
is called to log the error. If you look carefully, you'll notice
that there's a missing backslash before the newline and thus the
command is not continued. In the event of an error, this would likely
prove very confusing.

2. If you look closer still at the assignment of the "TMPFILE"
variable (see below), you'll notice that the "ret_val" variable
is assigned as part of the same sub-shell that first calls
the "mktemp" command. As a result, the value won't be accessible to
the calling shell. At least, that's what I understand from my shell
programming.

typeset TMPFILE=$(mktemp $TMPFILE_STR; ret_val=$?)

Version-Release number of selected component (if applicable):
clumanager-1.0.25-1

How reproducible:
Always

Steps to Reproduce:
N/A
    

Additional info:

A quick "grep mktemp *" inside the /usr/share/cluster/services
directory would indicate that this same code might be present in
other include files.

Comment 1 Lon Hohberger 2004-01-28 13:58:27 UTC
Created attachment 97298 [details]
Patch against 1.0.25 to fix problem

Here's a patch against 1.0.25 which fixes the problem.	It also appears in
svclib_device.

Comment 3 Peter Giorgilli 2004-01-28 23:15:46 UTC
Thanks for the patch! However, I don't believe the patch addresses the
second point in my original bug report. I think you'll find that the
code snippet below does not set the value of the "ret_val" variable in
the calling shell for the reason already explained.

typeset TMPFILE=$(mktemp $TMPFILE_STR; ret_val=$?)

Try it yourself.

In fact, I don't think it's possible to do what you're trying to do.
IMHO, it's better to code this in a more straightforward way a la
following:

typeset TMPFILE

if { ! mktemp $TMPFILE_STR; } then
   etc...
fi

Then again, given "mktemp" only returns 0 or 1, you might as well
just test whether the string is empty and just leave out the "ret_val"
bit all together.

The code as it is is misleading.

Comment 4 Lon Hohberger 2004-01-29 14:35:18 UTC
Right, sorry.  You're right, the following is not possible:

typeset TMPFILE=$(mktemp $TEMPFILE_STR; ret_val=$?)

I'm not sure how the author came up with that line.  In general,
you're more likely to run out of inodes on your file system, in which
case, 'mktemp' failing is the least of your worries. ;)

I can change the code to fix it so that it will correctly do the
assignments.  It's not elegant (e.g. still misleading), but it will
ensure that the log message actually gets out in the unlikely case
that mktemp fails, and has minimal impact on the existing code.

Comment 5 Lon Hohberger 2004-01-29 16:15:14 UTC
Created attachment 97336 [details]
Patch to fix both mktemp / backslash weirdness

Patch against 1.0.25 which fixes both of the potential problems.

Comment 7 Peter Giorgilli 2004-01-30 00:53:37 UTC
Sorry to be a pedant...but it still doesn't work. Here's why:

[peterg@aolinux tmp]$ ( cat /etc/redhat-release; echo $SHELL; typeset
-i ret_val; typeset SOMEVAR=$(/bin/false); ret_val=$?; echo $ret_val; )
Red Hat Linux release 8.0 (Psyche)
/bin/bash
0

[peterg@aolinux tmp]$ ( cat /etc/redhat-release; echo $SHELL; typeset
-i ret_val; typeset SOMEVAR=$(/bin/true); ret_val=$?; echo $ret_val; )
Red Hat Linux release 8.0 (Psyche)
/bin/bash
0

[peterg@aolinux tmp]$ ( cat /etc/redhat-release; echo $SHELL; typeset
-i ret_val; typeset SOMEVAR=$(/bin/anyolthing); ret_val=$?; echo
$ret_val; )
Red Hat Linux release 8.0 (Psyche)
/bin/bash
bash: /bin/anyolthing: No such file or directory
0

[peterg@aolinux tmp]$ ( cat /etc/redhat-release; echo $SHELL; typeset
-i ret_val; typeset -abc SOMEVAR=$(/bin/anyolthing); ret_val=$?; echo
$ret_val; )
Red Hat Linux release 8.0 (Psyche)
/bin/bash
bash: /bin/anyolthing: No such file or directory
bash: typeset: -b: invalid option
typeset: usage: typeset [-afFirtx] [-p] name[=value] ...
2

In other words, it's the exit value of the "typeset" command (which
will always be true!) that is being assigned to "ret_val" -- not the
"mktemp".

IMHO, it's better to omit the "ret_val" all together as it doesn't
tell you anything. Ultimately, it's whether the value is non-empty
that counts. If you really want to be paranoid you might also test
whether the file exists and is a regular file. That is:

if ! [[ -n "$tmpfile" && -f "$tmpfile" ]]; then
    logAndPrint ...
fi

Comment 8 Lon Hohberger 2004-01-30 15:01:28 UTC
The typeset lines and assignments are on different lines with the
patch applied.  The patch makes the code behave in the following
manner, to use the above examples:

typeset -i ret_val
typeset SOMEVAR
SOMEVAR=$(/bin/false); ret_val=$?

The above causes the correct behavior.  Consequently, what the
examples above should have done is:

( cat /etc/redhat-release; echo $SHELL; typeset -i ret_val; typeset
SOMEVAR; SOMEVAR=$(/bin/false); ret_val=$?; echo $ret_val; )


This works properly:

[lhh@eagle lhh]$ ( cat /etc/redhat-release; echo $SHELL; typeset -i
ret_val; typeset SOMEVAR; SOMEVAR=$(/bin/false); ret_val=$?; echo
$ret_val; )
Red Hat Enterprise Linux WS release 3 (Taroon)
/bin/bash
1
[lhh@eagle lhh]$

Comment 9 Peter Giorgilli 2004-02-02 00:48:24 UTC
I should've looked more carefully at the patch!

Thanks for the explanation. Case closed!

Comment 11 Lon Hohberger 2004-02-02 14:58:36 UTC
Waiting for erratum / update to close; leaving as 'MODIFIED' for now.


Comment 12 Lon Hohberger 2004-03-09 20:18:48 UTC
Note: This also affects RHEL3

Comment 13 Suzanne Hillman 2004-03-19 15:46:24 UTC
Unable to test this, but the opener seemed to be fine with it. Closing
as errata.