Hi, Christian.
First of all, thank you very much for your contribution. I will merge
them to release a new version of usbmount.
On Oct 28 2009, Cristian Ionescu-Idbohrn wrote:
> On Sun, 25 Oct 2009, Rogério Brito wrote:
> > I'm fine with getting things as simple as possible, as long as they
> > are portable.
>
> Another patch (includes previous patch) against svn trunk, attached.
Some of the modifications that you propose are what I had in mind, while
other are better than what I was thinking about.
There are some things that I have to think about, though:
I would prefer if you could contribute patches that did fewer, more
focused changes, like, say, one patch only for removing the useless
quoting, other patch for changing the character classes (on top of the
previous one, etc).
I like your changes, BTW. I only have some minor comments.
> +SYMLINK_MODEL_DIR=/var/run/usbmount
I'm not really sure that we should use a variable to carry a certain
value if we don't plan on making it a changeable setting.
> # Exit if both vendor and model name are empty.
> -test -n "$UM_VENDOR" || test -n "$UM_MODEL" || exit 0
> +[ "$UM_VENDOR" ] || [ "$UM_MODEL" ] || {
I'm not really sure if I like the short-circuiting idiom instead of a
plain if-else.
Can you comment on favor of it? The elimination of the "test" command in
place of the brackets is definitely welcome, as we had already talked
about.
> - if [ $1 != debug ] || expr "$VERBOSE" : "[yY]" > /dev/null; then
> - logger -p user.$1 -t "usbmount[$$]" -- "$2"
> +log() {
> + local level
> +
> + level=$1
> + if [ "$level" != debug ] || is_verbose; then
> + shift
> + logger -p user.$level -t $TAG -- "$*"
This was something that I missed: logging all parameter. Very good idea.
> - if ! echo $USAGE | egrep -q "(filesystem|disklabel)"; then
> - log info "$DEVNAME does not contain a filesystem or disklabel"
> - exit 1
> - fi
> + case $USAGE in
> + filesystem|disklabel)
> + ;;
> + *)
> + log info "$DEVNAME does not contain a filesystem or disklabel"
> + exit 1
> + ;;
> + esac
Excellent.
> + if egrep -q "^[[:blank:]]*$DEVNAME[[:blank:]]" $FSTAB; then
> + cmd="mount $DEVNAME"
> + log info "executing command: $cmd"
> + $cmd
> + elif [ "$UUID" ] && egrep -q "^[[:blank:]]*UUID=$UUID[[:blank:]]"
> $FSTAB; then
> + cmd="mount -U $UUID"
> + log info "executing command: $cmd"
> + $cmd
Good. We can factor-out the logging and the execution of the mount command.
> - if in_list "$fstype" "$FILESYSTEMS"; then
> + if in_list "$FSTYPE" "$FILESYSTEMS"; then
I prefer to have shell variables in caps. Are your changes just
stylistic ones or with some other motivation?
For consistency's sake, I guess that everything can be done in all caps.
> + #//# Much shorter, less forks.
Very good.
> + log debug "surprise: action '$1'"
> + exit 1
Just nitpicking here, but, perhaps, if we are going to exit with an
error, perhaps that should not be logged as a debug only message, but as
an error.
I like your changes. Thank you for your contribution. If you want, you
can co-maintain usbmount with me, to bring it to a better shape.
Thanks, Rogério Brito.
--
Rogério Brito : rbr...@{mackenzie,ime.usp}.br : GPG key 1024D/7C2CAEB8
http://www.ime.usp.br/~rbrito : http://meusite.mackenzie.com.br/rbrito
Projects: algorithms.berlios.de : lame.sf.net : vrms.alioth.debian.org
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
