Bug 1182225 - .postinstall is not reentrant and it contains error-prone code from EWS 2.1
Summary: .postinstall is not reentrant and it contains error-prone code from EWS 2.1
Keywords:
Status: CLOSED EOL
Alias: None
Product: JBoss Web Server 3
Classification: Retired
Component: httpd
Version: 3.0.0
Hardware: Unspecified
OS: Linux
urgent
urgent
Target Milestone: DR02
: 3.0.0
Assignee: Weinan Li
QA Contact: Michal Karm Babacek
URL:
Whiteboard:
Depends On:
Blocks: 1178630 1180562
TreeView+ depends on / blocked
 
Reported: 2015-01-14 16:35 UTC by Michal Karm Babacek
Modified: 2020-03-27 19:37 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2020-03-27 19:37:25 UTC
Embargoed:


Attachments (Terms of Use)

Description Michal Karm Babacek 2015-01-14 16:35:39 UTC
1) .postinstall is not reentrant

Dear folks, we can't have these constructs in .postinstall:

> +++ 
>  51 echo "DefaultRuntimeDir ${currentDir}/run" >> conf.d/00-base.conf
> +++
>  70 mv conf.d/00-base.conf conf/
> +++
>  71 sed -i -e 's:Include conf.d/\*\.conf:Include conf/00-base.conf:' conf/httpd.conf
> +++

If you run the .postinstall twice in a row, 00-base.conf ends up containing only the "DefaultRuntimeDir" string.
Let's minimize the .postinstall to the following, which is, in my humble opinion, its sole purpose:

- replace paths so as to reflect the installation directory (${currentDir})
- generate keys and files that are to be generated
- create a user, carry out any system specific configuration
- configure stuff unknown at package(compile) time (e.g. machine fqdn, etc.)

IMHO, any usage of sed/awk on strings _known_ at package time does not belong to .postinstall, it belongs to the productization compose suite; in fact files in question could be edited so as no replace is needed.



2) As a follow up, please, remove any static replacements from .postinstall to the respective files, e.g.:

This does not belong to .postinstall, it's static; i.e. make the change in the actual file instead:

> sed -i -e "s:LoadModule proxy_balancer_module modules/mod_proxy_balancer.so:#LoadModule proxy_balancer_module modules/mod_proxy_balancer.so:" conf.d/00-proxy.conf

> sed -i -e "s:/var/www:jws-3.0/httpd/www:g" -e "s:/etc/httpd:jws-3.0/httpd:g" www/error/noindex.html

> # bz1180562
> mv conf.d/00-base.conf conf/
> sed -i -e 's:Include conf.d/\*\.conf:Include conf/00-base.conf:' conf/httpd.conf
> echo 'Include conf.d/*.conf' >> conf/httpd.conf

On the other hand, these lines are O.K to have in .postinstall, because they configure the installation directory unknown at package time, e.g.

> echo "PidFile ${currentDir}/run/httpd.pid" >> conf.d/00-mpm.conf
> echo "DefaultRuntimeDir ${currentDir}/run" >> conf.d/00-base.conf



3)

If we try to look at it from yet another point of view, it might be worth a shot to get rid of most of these ${currentDir} lines anyway by using a token, e.g. @@ServerRoot@@ and replacing it with `find` globally in all *.conf.in files before moving *.conf.in to *.conf.  That is the proper way to do it [1].



Conclusion
==========
Please, make the .postinstall reentrant again and remove superfluous code wherever possible by making permanent changes to the concerned files.  

Thank you very much for your effort and last but not least, thank you for reading this rather exhaustive BugZilla :-)

If you disagree with these claims, please, do comment. It is indeed highly likely that I missed and/or misunderstood something.


Cheers ♡


[1] Let me back up my claim with http://archive.apache.org/dist/httpd/httpd-2.4.6.tar.bz2

Comment 4 Weinan Li 2015-01-28 16:41:29 UTC
Hi Michal,

Thanks for detail dscription :-) I have moved static parts to build process and add conditional tests in postinstall to make it re-entrant.

Because ${currentDir} parts are determined by user themselves, so I can't move them to build process.

In conclusion, here's the fix:

[weli@dhcp-66-78-87 jws-compose]$ git diff
diff --git a/jws-compose.spec b/jws-compose.spec
index fec63d6..d7e7c29 100644
--- a/jws-compose.spec
+++ b/jws-compose.spec
@@ -398,10 +398,14 @@ cp %{_libdir}/%{httpd}/modules/mod_ssl.so httpd/modules/
 mv httpd/conf.modules.d/* httpd/conf.d/
 rm -rf httpd/conf.modules.d
 sed -i -e '/IncludeOptional conf\.d\/\*\.conf/d' -e  's/Include conf\.modules\.d/Include conf\.d/' httpd/conf/httpd.conf
+
 # bz1175307 merge ssl conf
 echo -e "0r httpd/conf.d/00-ssl.conf\nw" | ed httpd/conf.d/ssl.conf
 rm httpd/conf.d/00-ssl.conf

+#JBPAPP-9446
+sed -i -e "s:LoadModule proxy_balancer_module modules/mod_proxy_balancer.so:#LoadModule proxy_balancer_module modules/mod_proxy_balancer.so:" httpd/conf.d/00-proxy.conf
+
 %ifnarch ppc64
 #jsvc
 mkdir -p extras
diff --git a/postinstall b/postinstall
index 858d975..161f8ad 100644
--- a/postinstall
+++ b/postinstall
@@ -28,6 +28,9 @@ fi

 currentDir=`pwd`
 sed -i -e "s: modules/: $currentDir/modules/:g" -e "s:/var/www:$currentDir/www:g" -e "s:ServerRoot \"/etc/httpd\":ServerRoot \"$currentDir\":" conf/httpd.conf
+
+grep -F 'OPTIONS="-f' sbin/apachectl 1> /dev/null
+if [ $? -ne 0 ]; then
 cat > .tmppostinstallfile << EOF
 # the options for httpd command
 OPTIONS="-f $currentDir/conf/httpd.conf -E $currentDir/logs/httpd.log"
@@ -36,6 +39,7 @@ export LD_LIBRARY_PATH=$currentDir/lib
 EOF
 sed -i -e "s:HTTPD='./httpd':HTTPD='$currentDir/sbin/httpd':g" -e "/HTTPD=/r .tmppostinstallfile" sbin/apachectl
 rm -f .tmppostinstallfile
+fi

 #JBPAPP-9446
 if [ -f 'conf.d/mod_cluster.conf' ]; then
@@ -45,19 +49,17 @@ fi
 #JBPAPP-9381
 sed -i -e "s:/var/www:${pkgdir}/httpd/www:g" -e "s:/etc/httpd:${pkgdir}/httpd:g" www/error/noindex.html

-#JBPAPP-9446
-sed -i -e "s:LoadModule proxy_balancer_module modules/mod_proxy_balancer.so:#LoadModule proxy_balancer_module modules/mod_proxy_balancer.so:" conf.d/00-proxy.conf
-
-echo "DefaultRuntimeDir ${currentDir}/run" >> conf.d/00-base.conf
-
 sed -i -e "s:/usr/libexec/:${currentDir}/sbin/:" -e "s:/var/www:${currentDir}/www:g" -e "s:/run/httpd/:${currentDir}/run/:" conf.d/ssl.conf

 sed -i -e "s:/usr/share/httpd/manual:${currentDir}/../doc/httpd:g" conf.d/manual.conf

-sed -i -e "s:/usr/share/httpd/noindex/index.html:${currentDir}/www/error/noindex.html:g" -e "s:/usr/share/httpd/noindex:${currentDir}/www/error:g" conf.d/welcome.conf
-
 # bz1176804 httpd: could not log pid to file /run/httpd/httpd.pid
-echo "PidFile ${currentDir}/run/httpd.pid" >> conf.d/00-mpm.conf
+grep 'PidFile' conf.d/00-mpm.conf 1> /dev/null
+if [ $? -ne 0 ]; then
+    echo "PidFile ${currentDir}/run/httpd.pid" >> conf.d/00-mpm.conf
+fi
+
+sed -i -e "s:/usr/share/httpd/noindex/index.html:${currentDir}/www/error/noindex.html:g" -e "s:/usr/share/httpd/noindex:${currentDir}/www/error:g" conf.d/welcome.conf

 # bz1178816
 /usr/sbin/useradd -c "Apache" -u 48 \
@@ -67,7 +69,15 @@ mkdir -p ${dav_lockdb_dir} 2> /dev/null
 chown apache ${dav_lockdb_dir} 2> /dev/null

 # bz1180562
-mv conf.d/00-base.conf conf/
-sed -i -e 's:Include conf.d/\*\.conf:Include conf/00-base.conf:' conf/httpd.conf
-echo 'Include conf.d/*.conf' >> conf/httpd.conf
+if [ -f conf.d/00-base.conf ]; then
+    mv conf.d/00-base.conf conf/
+    echo "DefaultRuntimeDir ${currentDir}/run" >> conf/00-base.conf
+    sed -i -e 's:Include conf.d/\*\.conf:Include conf/00-base.conf:' conf/httpd.conf
+fi
+
+grep -F 'Include conf.d/*.conf' conf/httpd.conf 1> /dev/null
+if [ $? -ne 0 ]; then
+    echo 'Include conf.d/*.conf' >> conf/httpd.conf
+fi
+

Comment 5 Weinan Li 2015-01-28 16:43:19 UTC
Verification:

[weli@dhcp-66-78-87 x86_64]$ rpm2cpio jws-compose-zip-3.0.0-23.el6.x86_64.rpm | cpio -idv
./usr/share/java/jbossas-fordev
./usr/share/java/jbossas-fordev/jws-application-servers-3.0.0-RHEL6-x86_64.zip
unzip./usr/share/java/jbossas-fordev/jws-docs-3.0.0.zip
./usr/share/java/jbossas-fordev/jws-examples-3.0.0.zip
 ./usr/share/java/jbossas-fordev/jws-httpd-3.0.0-RHEL6-x86_64.zip
./usr/share/java/jbossas-fordev/jws-src-3.0.0.zip
143269 blocks
[weli@dhcp-66-78-87 x86_64]$ unzip -q ./usr/share/java/jbossas-fordev/jws-httpd-3.0.0-RHEL6-x86_64.zip
[weli@dhcp-66-78-87 x86_64]$ cd jws-3.0/
[weli@dhcp-66-78-87 jws-3.0]$ cd httpd/
[weli@dhcp-66-78-87 httpd]$ ls
ABOUT_APACHE  bin  cache  CHANGES  conf  conf.d  include  lib  LICENSE  logs  modules  NOTICE  README  run  sbin  var  VERSIONING  www
[weli@dhcp-66-78-87 httpd]$ sudo ./.postinstall
[weli@dhcp-66-78-87 httpd]$ cd ..
[weli@dhcp-66-78-87 jws-3.0]$ cp -r httpd httpd-bkup
[weli@dhcp-66-78-87 jws-3.0]$ cd httpd
[weli@dhcp-66-78-87 httpd]$ sudo ./.postinstall
[weli@dhcp-66-78-87 httpd]$ cd ..
[weli@dhcp-66-78-87 jws-3.0]$ diff -Nur httpd httpd-bkup

Comment 6 Michal Karm Babacek 2015-03-25 10:53:37 UTC
Thanks a lot Jan Stefl for helping out.

Comment 7 Michal Karm Babacek 2015-04-02 09:45:01 UTC
Well, reassigning...

Comment 8 Michal Karm Babacek 2015-04-02 10:43:01 UTC
I take "Starting JBoss Enterprise Web Server" for the name of the Apache HTTP Server in the realm of JWS and flip this issue to VERIFIED.
If the aforementioned string might cause branding issues, please, reopen.


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