Bug 606634 - Output of 'plymouth-set-default-theme' is incorrect when no theme is set
Summary: Output of 'plymouth-set-default-theme' is incorrect when no theme is set
Keywords:
Status: CLOSED UPSTREAM
Alias: None
Product: Fedora
Classification: Fedora
Component: plymouth
Version: 13
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Ray Strode [halfline]
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-06-22 06:25 UTC by Bruce Jerrick
Modified: 2010-12-17 15:41 UTC (History)
4 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2010-07-06 15:18:35 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Bruce Jerrick 2010-06-22 06:25:08 UTC
Description of problem:
When no plymouth theme is set, 'plymouth-set-default-theme' returns ".plymouth",
and that causes errors in 'plymouth-populate-initrd', because the latter expects
blank output from 'plymouth-set-default-theme' in this case.

(And when 'plymouth-populate-initrd' soldiers on instead of exiting, the error
message it ultimately produces is somewhat nonsensical.)

README FIRST:
Despite the verbosity of this report, the fix is trivial -- see FIX in
"Additional info" below.

--------------------------------------------------------------------------------
Version-Release number of selected component (if applicable):
plymouth-scripts-0.8.2-3.fc13.i686

--------------------------------------------------------------------------------
How reproducible:
100%

Steps to Reproduce:
Some conditions have to be met:
    1. The contents of /etc/plymouth/plymouthd.conf have to be original, i.e.:

	# Administrator customizations go in this file
	#[Daemon]
	#Theme=fade-in

    2. Package plymouth-theme-charge has to be *not* installed (reason is given
       at end of "Additional info" below).

    3. /usr/share/plymouth/themes/default.plymouth must not exist (typically
       a symlink to something like text/text.plymouth, set automagically by
       something I haven't figured out yet; it didn't happen at my F13 OS
       installs).

Then, run (with no args):
plymouth-set-default-theme

--------------------------------------------------------------------------------
Actual results:
.plymouth

As a result, when 'plymouth-populate-initrd' is run during a kernel upgrade,
it produces:

    grep: /usr/share/plymouth/themes/.plymouth/.plymouth.plymouth: \
	No such file or directory
    The default plymouth plugin () doesn't exist

--------------------------------------------------------------------------------
Expected results:
No output -- that is what 'plymouth-populate-initrd' expects when no default
theme is set (see "Additional info" below).

--------------------------------------------------------------------------------
Additional info:
(I haven't given full pathnames of the scripts involved, for brevity; they're
easily found with 'locate' or 'rpmls'.)

FIX:
Just add double quotes around the first arg (the readlink return) in
the basename call in the get_default_theme function in
'plymouth-set-default-theme'.
There's a patch attached that does this.  (Just as a matter of coding practice
it also does the same thing to an earlier instance of 'basename', but that's
not strictly necessary.)
UPDATE: I can't attach the patch -- the aptly-named *bug*zilla interface insists
that I choose a method of choosing content-type, yet there seems to be no way
to do that.  I'll add a comment later with the patch.
Note that in the F12 version there are further instances of 'basename' that
should also be fixed.

For those interested in the pathology:

The problem lies in the following code from 'plymouth-set-default-theme' (in
  function get_default_theme):

  THEME_NAME=$(basename \
    $(readlink ${PLYMOUTH_DATADIR}/plymouth/themes/default.plymouth) .plymouth)

which returns ".plymouth" if .../default.plymouth does not exist (basename sees 
".plymouth" as its only argument).

The problem caused by using ".plymouth" in 'plymouth-populate-initrd' is shown
in the following code from that script.  It does not work as intended; it
expects a blank return from 'plymouth-set-default-theme' when no theme is set:

  ... PLYMOUTH_THEME_NAME=$(plymouth-set-default-theme)
  ...
  if [ -z "$PLYMOUTH_THEME_NAME" ]; then
      echo "No default plymouth plugin is set" > /dev/stderr
      ...

(BTW, "No default plymouth theme is set" would be more to the point.)
                           ^^^^^
Why the plymouth-theme-charge package has to be not installed:
  If /usr/share/plymouth/themes/charge/charge.plymouth exists,
  that will be reported as the default theme (being set as such
  in /usr/share/plymouth/plymouthd.defaults).

--------------------------------------------------------------------------------

Comment 1 Bruce Jerrick 2010-06-22 06:26:11 UTC
Here's the patch that Mr. Bugzilla couldn't attach:

--- .Fedora/plymouth-set-default-theme	2010-05-07 12:05:53.000000000 -0700
+++ .LOCAL/plymouth-set-default-theme	2010-06-21 16:05:12.589182802 -0700
@@ -38,7 +38,7 @@ function list_themes ()
 {
         for theme in ${PLYMOUTH_DATADIR}/plymouth/themes/*/*.plymouth; do
                 [ -f $theme ] || continue;
-                echo "$(basename $theme .plymouth)"
+                echo "$(basename "$theme" .plymouth)"
         done
 }
 
@@ -64,7 +64,7 @@ function get_default_theme ()
         fi
 
         if [ -z "$THEME_NAME" -o ! -r "${PLYMOUTH_DATADIR}/plymouth/themes/$THEME_NAME/$THEME_NAME.plymouth" ]; then
-                THEME_NAME=$(basename $(readlink ${PLYMOUTH_DATADIR}/plymouth/themes/default.plymouth) .plymouth)
+                THEME_NAME=$(basename "$(readlink ${PLYMOUTH_DATADIR}/plymouth/themes/default.plymouth)" .plymouth)
         fi
         [ -z "$THEME_NAME" ] || echo $THEME_NAME && exit 1
 }

Comment 2 Ray Strode [halfline] 2010-07-06 15:18:35 UTC
Hi,

I fixed this upstream in a slightly different way a few weeks ago.  See:

http://cgit.freedesktop.org/plymouth/commit/?id=070d2a0a5ad746046ef15065a4fa654cd22906e5

You're change is still quite valid though (from a "good practices" point of view), so I've pushed it here:

http://cgit.freedesktop.org/plymouth/commit/?id=95bf9eed86b4bf267bcd77774ee37c08dc5b9162

Thanks for the bug report, analysis, and patch.

Comment 3 MarcH 2010-07-30 11:13:45 UTC
(In reply to comment #0)
> (And when 'plymouth-populate-initrd' soldiers on instead of exiting, the error
> message it ultimately produces is somewhat nonsensical.)

Bruce, since you seem to have a very thorough understanding of this bug would you know if there are any consequences besides the error message?

Comment 4 Matt McCutchen 2010-12-17 15:41:07 UTC
I noticed that bug 606626, bug 606627, bug 606630, and bug 606632 are all duplicates of this one.  Bug 606625 could perhaps be changed to a duplicate of this one as well.


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