4

I found the following patch of code:

function some_fun() {    
#[...]
    [ -d ${MKAPP_BUILDDIR}/tmp ] || mkdir ${MKAPP_BUILDDIR}/tmp || return 1

    if [ $? -ne 0 ]; then   # never true?
            mount -t tmpfs none ${MKAPP_BUILDDIR}/tmp || return 1
    fi
}

As far as I understand it, it checks if there is a tmp directory in the build directory and, if that is not the case, tries to create it. If that also fails it exits the functions with status code 1.

If that is the case, wouldn't checking for an unsuccessful exit code in the if statement below be pointless? There can't be an unsuccessful command before, because it would've led to the function returning and the if statement never being processed.

Am I correct in thinking that, or is the syntax playing tricks on me?

I'm using bash, btw.

  • Good catch. In other matters, you should always quote all references to shell variables unless you have a good reason not to, and you're sure you know what you're doing. By contrast, while braces can be important, they're not as important as quotes, so "$MKAPP_BUILDDIR" is better than ${MKAPP_BUILDDIR}. – G-Man Oct 21 '14 at 21:33
  • @G-Man So [ -d "$MKAPP_BUILDDIR/tmp" ] would be better style in your opinion? Or just combine the two to make [ -d "${MKAPP_BUILDDIR}/tmp" ] ? – Minix Oct 22 '14 at 6:58
  • You're getting into the realm of personal preference. I would use the first of those options, because the braces in the second aren't functionally necessary, but if you used both quotes and braces, I wouldn't say anything. – G-Man Oct 22 '14 at 15:59
3

Yes you are correct. The return procedure will exit from the function to the originating caller in the script with an exit status of 1.

Therefore the mount command will never get processed.

To resolve this, just strip the if .. then statement from the mount command:

[ -d ${MKAPP_BUILDDIR}/tmp ] || mkdir ${MKAPP_BUILDDIR}/tmp || return 1

mount -t tmpfs none ${MKAPP_BUILDDIR}/tmp || return 1

Therefore if the directory cannot be created: exit with status of 1.
If the creation is successful, then run the mount command.

  • I recently got the code from someone else. The return statement is behind basically every statement, so that the whole function stops, if one of the statements fails ( I would presume ). Looks like he added it there by mistake. I will add this to a list of questions I will ask that person. Thank you for the quick answer. – Minix Oct 21 '14 at 10:49
  • @Minix you could have tried some configs out by adding echo statements yourself. I tried that and came (too late to answer) to the same conclusion as geedoubleya. Don't forget to accept his answer, and upvote (once you have that privilege (which you should get when you accept). – Anthon Oct 21 '14 at 10:53
  • @Anthon I did, but I wanted to check if there may be some meaning or some special case, that escapes me. Since I still don't have a full overview of all the nooks and crannies of shell script, I thought it wouldn't hurt to ask. Thanks for taking the time to look for an answer. – Minix Oct 21 '14 at 10:58
  • 2
    I agree with the first half of your answer (the mount command is currently unreachable) but not the second half (what to do about it). You don’t want to mount onto ${MKAPP_BUILDDIR}/tmp unless it exists, so (IMNSHO) the correct fix to the script is to leave the || return 1 alone, and delete the if [ $? -ne 0 ]; then and the fi. – G-Man Oct 21 '14 at 21:24
  • Good spot @G-Man - thanks! Changed it accordingly. – geedoubleya Oct 21 '14 at 21:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.