FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > ArchLinux > ArchLinux General Discussion

 
 
LinkBack Thread Tools
 
Old 06-30-2010, 09:47 PM
Victor Lowther
 
Default Bashification of initscripts

Despite efforts to make the initscripts POSIX, we use bash 4.0 features.

Bashifying this framework should result in about a 30% speedup, assuming no
IO latency and that all programs we call also take zero time.
---
functions | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/functions b/functions
index 672eed2..023de35 100644
--- a/functions
+++ b/functions
@@ -1,4 +1,4 @@
-#
+#!/bin/bash
# initscripts functions
#

--
1.7.1
 
Old 06-30-2010, 09:55 PM
Thomas Bächler
 
Default Bashification of initscripts

Am 30.06.2010 23:47, schrieb Victor Lowther:
> Despite efforts to make the initscripts POSIX, we use bash 4.0 features.
>
> Bashifying this framework should result in about a 30% speedup, assuming no
> IO latency and that all programs we call also take zero time.
> ---
> functions | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/functions b/functions
> index 672eed2..023de35 100644
> --- a/functions
> +++ b/functions
> @@ -1,4 +1,4 @@
> -#
> +#!/bin/bash
> # initscripts functions

I don't see the need for this patch. functions is not supposed to be
executed standalone, it is only source'd from bash scripts.
 
Old 06-30-2010, 10:07 PM
Victor Lowther
 
Default Bashification of initscripts

On Wed, 2010-06-30 at 23:55 +0200, Thomas Bächler wrote:
> Am 30.06.2010 23:47, schrieb Victor Lowther:
> > Despite efforts to make the initscripts POSIX, we use bash 4.0 features.
> >
> > Bashifying this framework should result in about a 30% speedup, assuming no
> > IO latency and that all programs we call also take zero time.
> > ---
> > functions | 2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/functions b/functions
> > index 672eed2..023de35 100644
> > --- a/functions
> > +++ b/functions
> > @@ -1,4 +1,4 @@
> > -#
> > +#!/bin/bash
> > # initscripts functions
>
> I don't see the need for this patch. functions is not supposed to be
> executed standalone, it is only source'd from bash scripts.

It is a habit I have -- including the shebang line at the top makes sure
my text editors automatically detect the right shell syntax for syntax
highlighting.

--
Victor Lowther
LPIC2 UCP RHCE
 
Old 06-30-2010, 10:28 PM
Thomas Bächler
 
Default Bashification of initscripts

Am 01.07.2010 00:07, schrieb Victor Lowther:
> On Wed, 2010-06-30 at 23:55 +0200, Thomas Bächler wrote:
>> Am 30.06.2010 23:47, schrieb Victor Lowther:
>>> Despite efforts to make the initscripts POSIX, we use bash 4.0 features.
>>>
>>> Bashifying this framework should result in about a 30% speedup, assuming no
>>> IO latency and that all programs we call also take zero time.
>>> ---
>>> functions | 2 +-
>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/functions b/functions
>>> index 672eed2..023de35 100644
>>> --- a/functions
>>> +++ b/functions
>>> @@ -1,4 +1,4 @@
>>> -#
>>> +#!/bin/bash
>>> # initscripts functions
>>
>> I don't see the need for this patch. functions is not supposed to be
>> executed standalone, it is only source'd from bash scripts.
>
> It is a habit I have -- including the shebang line at the top makes sure
> my text editors automatically detect the right shell syntax for syntax
> highlighting.
>

Fair enough, but then that should be in the commit message.
 
Old 07-02-2010, 02:58 AM
Caleb Cushing
 
Default Bashification of initscripts

On Wed, Jun 30, 2010 at 6:07 PM, Victor Lowther
<victor.lowther@gmail.com> wrote:
>> I don't see the need for this patch. functions is not supposed to be
>> executed standalone, it is only source'd from bash scripts.
>
> It is a habit I have -- including the shebang line at the top makes sure
> my text editors automatically detect the right shell syntax for syntax
> highlighting.

It has the added benefit of making it quite clear that the script will
not work on another POSIX shell.

--
Caleb Cushing

http://xenoterracide.blogspot.com
 
Old 07-02-2010, 03:41 AM
Victor Lowther
 
Default Bashification of initscripts

On Thu, 2010-07-01 at 22:58 -0400, Caleb Cushing wrote:
> On Wed, Jun 30, 2010 at 6:07 PM, Victor Lowther
> <victor.lowther@gmail.com> wrote:
> >> I don't see the need for this patch. functions is not supposed to be
> >> executed standalone, it is only source'd from bash scripts.
> >
> > It is a habit I have -- including the shebang line at the top makes sure
> > my text editors automatically detect the right shell syntax for syntax
> > highlighting.
>
> It has the added benefit of making it quite clear that the script will
> not work on another POSIX shell.

Indeed.

--
Victor Lowther
LPIC2 UCP RHCE
 
Old 07-07-2010, 04:03 AM
"Allan McRae"
 
Default Bashification of initscripts

Here is a quick review on all these patches. I recommend that the lvm
and crypttab changes get a decent amount of testing before these go live
as they are the biggest changes being done.



Tighten up the console size finding code a bit.
Add some white space in test construct:
if ((STAT_COL==0)); then
if (( STAT_COL == 0 )); then
and throughout these patches


Simplify the code that clears USECOLOR.
The following condition is removed with no commit message to explain why
if [ $? = 3 ]; then
TERM_COLOURS=8


Replace trivial use of grep with bash regex conditional.
- if [ -n "$CONSOLEMAP" ] && echo "$LOCALE" | /bin/grep -qi utf ; then
+ [[ $CONSOLEMAP && $LOCALE =~ UTF|utf ]] && CONSOLEMAP=""

Use ... && ${LOCALE,,} == utf ]] to accurately replicate the grep


Replace slightly too long echo staement with a here document.
^^ typo
I actually find the echo more readable


Change the daemon runnign loop to use a case statement.
Quote daemon names.
Merge these commits


Both rc.single and rc.shutdown use the same code to kill everything.
+ # $1 = where we are being called from.
+ # This is used to determine which hooks to run.
-> Add separater line here...
+ # Find daemons NOT in the DAEMONS array. Shut these down first

Why has this been removed:
-if [ -x /etc/rc.local.shutdown ]; then
- /etc/rc.local.shutdown
-fi
Ah... it has been moved to another place in another commit. Please
document these sorts of changes in your commit message and preferably do
the entire move in one commit.



Flatten LVM deactivation if block in rc.shutdown.
This change does not do the same thing and I do not see where it gets
replicated
-if [ -f /etc/crypttab -a -n "$(/bin/grep -v ^# /etc/crypttab |
/bin/grep -v ^$)" ]; then

+if [[ -f /etc/crypttab ]]; then

Also another:
+if [[ $USELVM =~ yes|YES
-> ${USELVM,,} == yes


bashify bringing up the loopback adaptor.
Add a commit message as that is doing a lot more than bashifing.


Bashify locale setting.
$LOCALE =~ utf|UTF


Allan
 
Old 07-07-2010, 04:25 AM
Dan McGee
 
Default Bashification of initscripts

On Tue, Jul 6, 2010 at 11:03 PM, Allan McRae <allan@archlinux.org> wrote:
> Here is a quick review on all these patches. * I recommend that the lvm and
> crypttab changes get a decent amount of testing before these go live as they
> are the biggest changes being done.
>
> *Why has this been removed:
> * *-if [ -x /etc/rc.local.shutdown ]; then
> * *- /etc/rc.local.shutdown
> * *-fi
> *Ah... it has been moved to another place in another commit. *Please
> document these sorts of changes in your commit message and preferably do the
> entire move in one commit.

If there was one thing I wasn't so fond of in these patches, it seemed
like there were too many. The beauty of git is the ability to go back
and squash and split patches in a way that makes a lot more sense to
others- it might not have been the way or order you did things in, but
you should try as hard as possible to make a commit the largest
logical unit that makes sense, but still small enough to grasp fully.

If there are ever closely-related changes strewn across multiple
patches in a patch set, you should probably think about merging those
commits.

-Dan
 
Old 07-07-2010, 11:49 AM
Victor Lowther
 
Default Bashification of initscripts

On Wed, 2010-07-07 at 14:03 +1000, Allan McRae wrote:
> Here is a quick review on all these patches. I recommend that the lvm
> and crypttab changes get a decent amount of testing before these go live
> as they are the biggest changes being done.
>
>
> Tighten up the console size finding code a bit.
> Add some white space in test construct:
> if ((STAT_COL==0)); then
> if (( STAT_COL == 0 )); then
> and throughout these patches

Maybe when all the other bugs are fixed.

> Simplify the code that clears USECOLOR.
> The following condition is removed with no commit message to explain why
> if [ $? = 3 ]; then
> TERM_COLOURS=8

An exitval of 3 means tput has no idea what terminal type we are running
on, and I figured that it is after to default to not using colorized
output in that case.

> Replace trivial use of grep with bash regex conditional.
> - if [ -n "$CONSOLEMAP" ] && echo "$LOCALE" | /bin/grep -qi utf ; then
> + [[ $CONSOLEMAP && $LOCALE =~ UTF|utf ]] && CONSOLEMAP=""
>
> Use ... && ${LOCALE,,} == utf ]] to accurately replicate the grep

Hmmm... I had not seen that parameter expansion before. New to bash
4.1?

> Replace slightly too long echo staement with a here document.
> ^^ typo
> I actually find the echo more readable

Well, then Thomas can keep it or drop it as he prefers.

> Change the daemon runnign loop to use a case statement.
> Quote daemon names.
> Merge these commits
>
>
> Both rc.single and rc.shutdown use the same code to kill everything.
> + # $1 = where we are being called from.
> + # This is used to determine which hooks to run.
> -> Add separater line here...
> + # Find daemons NOT in the DAEMONS array. Shut these down first
>
> Why has this been removed:
> -if [ -x /etc/rc.local.shutdown ]; then
> - /etc/rc.local.shutdown
> -fi
> Ah... it has been moved to another place in another commit. Please
> document these sorts of changes in your commit message and preferably do
> the entire move in one commit.

Will fix.

> Flatten LVM deactivation if block in rc.shutdown.
> This change does not do the same thing and I do not see where it gets
> replicated
> -if [ -f /etc/crypttab -a -n "$(/bin/grep -v ^# /etc/crypttab |
> /bin/grep -v ^$)" ]; then
> +if [[ -f /etc/crypttab ]]; then

All the second bit of that test does is to see if there is actual
content in /etc/crypttab. I handle that in the read loop by zapping
comments and blank lines with parameter expansion and conditional checks
instead -- the greps end up reading the whole file anyways.

Replicated via parameter expansion and conditionals:

[[ $line && ${line:0:1} != '#' ]] || continue
eval nspo=("${line%#*}")

> Also another:
> +if [[ $USELVM =~ yes|YES
> -> ${USELVM,,} == yes
>
>
> bashify bringing up the loopback adaptor.
> Add a commit message as that is doing a lot more than bashifing.

Already fixed in the last round of rebasing.

> Bashify locale setting.
> $LOCALE =~ utf|UTF
>
>
> Allan

--
Victor Lowther
LPIC2 UCP RHCE
 
Old 07-07-2010, 12:40 PM
Victor Lowther
 
Default Bashification of initscripts

On Jul 6, 2010, at 11:25 PM, Dan McGee <dpmcgee@gmail.com> wrote:

If there was one thing I wasn't so fond of in these patches, it seemed
like there were too many.


Some people like larger patches, some like smaller ones. It is easier
to merge smaller ones together than it is to split larger ones apart.



The beauty of git is the ability to go back
and squash and split patches in a way that makes a lot more sense to
others- it might not have been the way or order you did things in, but
you should try as hard as possible to make a commit the largest
logical unit that makes sense, but still small enough to grasp fully.


Ya, I wanted to get feedback to see if the larger Arch community was
interested before going too much farther.




If there are ever closely-related changes strewn across multiple
patches in a patch set, you should probably think about merging those
commits.

-Dan
 

Thread Tools




All times are GMT. The time now is 08:31 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright ©2007 - 2008, www.linux-archive.org