Bug 114435 - svclib_filesystem shell-script include file - problems with
svclib_filesystem shell-script include file - problems with
Status: CLOSED ERRATA
Product: Red Hat Enterprise Linux 2.1
Classification: Red Hat
Component: clumanager (Show other bugs)
2.1
i386 Linux
medium Severity medium
: ---
: ---
Assigned To: Lon Hohberger
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2004-01-27 21:58 EST by Peter Giorgilli
Modified: 2007-11-30 17:06 EST (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2004-03-19 10:46:24 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Patch against 1.0.25 to fix problem (2.21 KB, patch)
2004-01-28 08:58 EST, Lon Hohberger
no flags Details | Diff
Patch to fix both mktemp / backslash weirdness (4.72 KB, patch)
2004-01-29 11:15 EST, Lon Hohberger
no flags Details | Diff

  None (edit)
Description Peter Giorgilli 2004-01-27 21:58:33 EST
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 08:58:27 EST
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 18:15:46 EST
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 09:35:18 EST
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 11:15:14 EST
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-29 19:53:37 EST
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 10:01:28 EST
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-01 19:48:24 EST
I should've looked more carefully at the patch!

Thanks for the explanation. Case closed!
Comment 11 Lon Hohberger 2004-02-02 09:58:36 EST
Waiting for erratum / update to close; leaving as 'MODIFIED' for now.
Comment 12 Lon Hohberger 2004-03-09 15:18:48 EST
Note: This also affects RHEL3
Comment 13 Suzanne Hillman 2004-03-19 10:46:24 EST
Unable to test this, but the opener seemed to be fine with it. Closing
as errata.

Note You need to log in before you can comment on or make changes to this bug.