Re: [PATCH 2 of 5 V3] tools/hotplug: Remus network buffering setup scripts

[Ian Campbell <Ian.Campbell@citrix.com>]

On Thu, 2013-10-31 at 14:06 -0700, Shriram Rajagopalan wrote:
> On Thu, Oct 31, 2013 at 1:21 PM, Ian Campbell
> <Ian.Campbell@citrix.com> wrote:
>         > +
>         
>         > +case "$command" in
>         > +    setup)
>         > +     check_libnl_tools
>         > +     check_modules
>         > +
>         > +     claim_lock "pickifb"
>         > +     setup_ifb
>         > +     redirect_vif_traffic "$vifname" "$IFB"
>         > +     add_plug_qdisc "$vifname" "$IFB"
>         > +     release_lock "pickifb"
>         > +
>         > +     #not using xenstore_write that automatically exits on
>         error
>         > +     # because we need to cleanup
>         
>         
>         whitespace inconsistency.
>         
>         
> 
> 
> I seem to get this wrong again and again..
> Did you mean the space trailing the second # ? or should the code
> block inside setup/teardown
> be indented with two tabs instead of one ?

I meant "#foo" (1st line) vs "# foo" (2nd line) which just caught my
eye. They should be the same. More normal would be a space after the # I
think.

I don't think we have a particularly strict coding style for shell
scripts, but code being consistent with itself is a good start.

> 
> 
>         
>         > +     _xenstore_write "$XENBUS_PATH/ifb" "$IFB" ||
>         xs_write_failed "$vifname" "$IFB"
>         > +     ;;
>         > +    teardown)
>         > +     : ${IFB?}
>         
>         
>         Do you mean log debug or something here?
>         
>         
> 
> 
> This was to just make sure that the IFB variable was supplied as part
> of the environment..
> Just like the two checks on top of this script..
> "
> : ${vifname?}
> : ${XENBUS_PATH?}
> "

Do these result in useful error reporting to the end user? Bearing in
mind that the end user might be using libxl but not xl and therefore not
see stdout/err (which might be going into some daemon's log file).

Also, either I'm missing it or the bash manpage only documents
${parameter:?} and not ${parameter?}. Are both actually valid?

Ian.