On Thu, 22 Sep 2011, Zdenek Kabelac wrote:
> Dne 22.9.2011 12:36, Lukas Czerner napsal(a):
> > On Thu, 22 Sep 2011, Zdenek Kabelac wrote:
> >
> >> Dne 21.9.2011 18:45, Lukas Czerner napsal(a):
> >>> Remove command allows to remove unused devices from the pool (volume
> >>> group).
> >>>
> >>> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> >>> ---
> >>> scripts/fsadm.sh | 82 ++++++++++++++++++++++++++++++++++++++++-------------
> >>> 1 files changed, 62 insertions(+), 20 deletions(-)
> >>>
> >>> diff --git a/scripts/fsadm.sh b/scripts/fsadm.sh
> >>> index 6617de0..4a4f625 100755
> >>> --- a/scripts/fsadm.sh
> >>> +++ b/scripts/fsadm.sh
> >>> @@ -823,11 +823,12 @@ list_filesystems() {
> >>> IFS=$NL
> >>> local c=0
> >>> for line in $(LANG=C $LVM lvs -o lv_path,lv_size,segtype --noheadings --separator ' ' --nosuffix --units k 2> /dev/null); do
> >>> - c=$((c+1))
> >>> line=$(echo $line | sed -e 's/^ *\//\//')
> >>> volume=$(echo $line | cut -d' ' -f1)
> >>> - volumes[$c]=$volume
> >>> - segtype[$c]=$(echo $line | cut -d' ' -f3)
> >>> + [ "$volume" == "$last_volume" ] && continue
> >>> + c=$((c+1))
> >>> + local volumes[$c]=$volume
> >>> + local segtype[$c]=$(echo $line | cut -d' ' -f3)
> >>> detect_fs $volume
> >>> detect_mounted
> >>> detect_fs_size
> >>
> >> Could you please update/cleanup the patch set - so it doesn't rework same code
> >> multiple times over and over ?
> >> (It's a waste of time to review new code, which gets replaced several times
> >> during the whole patch set)
> >
> > Unfortunately that is what I have mentioned in the patch 0. The main
> > problem is that the fsadm is one file script and while adding other
> > functionality I was building helpers and yes also changing other parts
> > of the code.
> >
> > It was not possible for me make one feature, be done with it and move to
> > the another feature. Some of them actually supports each other and not
> > having to rebase with every change made my work significantly easier. So
> > at this point I am not going to rework the patches, since it would take
> > more time that actually write that code :).
> >
> > But, since it is a single file script, and I changed the whole thing
> > significantly (there is not much left from the original code) I can
> > create one big patch covering all the new features, so it will be easier
> > for you to review it. Will that be acceptable for you ?
>
>
> Yes - I could imagine in certain cases it might be useful to squash several
> patches together to improve readability here.
>
> But before you change some code - please look at how it has looked and do not
> put problem which were already resolved.
I am sorry, but I am not sure what you have in mind. Cold you be more
specific ?
>
> Few notes for script writing: use "$VAR" (since path may contain spaces)
> Preferred way for using shell test is 'test ' instead of []
Yes, as Stephane already said, using "$var" is really what we need to
use. I'll change it. But I am really not sure why would using 'test' be
preferred over [] ? doing things like [ -b "$dev" ] or similar seems
just better readable for me. But maybe I am missing something.
>
>
> Zdenek
>
>
>
>
_______________________________________________
linux-lvm mailing list
linux-lvm@redhat.com
https://www.redhat.com/mailman/listinfo/linux-lvm
read the LVM HOW-TO at http://tldp.org/HOWTO/LVM-HOWTO/
