Review Request 670: openlmi-providers [1/1] Add logging to the openlmi-mof-register script

Roman Rakus reviewboard at gallagherhome.com
Thu Aug 1 13:53:24 UTC 2013


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-openlmi.rhcloud.com/r/670/#review845
-----------------------------------------------------------



openlmi-mof-register
<http://reviewboard-openlmi.rhcloud.com/r/670/#comment453>

    Bash has native print option to print current time:
    printf "%(%Y-%m-%d)T" -1



openlmi-mof-register
<http://reviewboard-openlmi.rhcloud.com/r/670/#comment450>

    log "COMMAND:" "$@"
    looks more natural for me. Using this, you will pass as 1st argument the type and other arguments will be command (with parameters) what you like.



openlmi-mof-register
<http://reviewboard-openlmi.rhcloud.com/r/670/#comment451>

    Using eval is very error prone; the command injection is very easy to do, we must be careful.
    
    We must also take care that parameters doesn't have any shell functionality.
    
    To be honest, I don't like it.



openlmi-mof-register
<http://reviewboard-openlmi.rhcloud.com/r/670/#comment452>

    Also split to log "CODE:" "$?"



openlmi-mof-register
<http://reviewboard-openlmi.rhcloud.com/r/670/#comment454>

    This will not work. You are checking return value from log_command, it's return value is return value from last executed command, which is log, in turn it is printf.



openlmi-mof-register
<http://reviewboard-openlmi.rhcloud.com/r/670/#comment455>

    log STARTING "$0" "$@"



openlmi-providers.spec
<http://reviewboard-openlmi.rhcloud.com/r/670/#comment456>

    You can encapsulate the all commands to {} and redirect once. Something like:
    if ...
      {
      } >>...
    fi
    
    or do the redirection for the if command:
    if...
    fi >>...
    
    It's simpler, readable and less error prone.


- Roman Rakus


On Aug. 1, 2013, 1:19 p.m., Radek Novacek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard-openlmi.rhcloud.com/r/670/
> -----------------------------------------------------------
> 
> (Updated Aug. 1, 2013, 1:19 p.m.)
> 
> 
> Review request for OpenLMI Developers.
> 
> 
> Repository: openlmi-providers
> 
> 
> Description
> -------
> 
> Add logging to the openlmi-mof-register script
> 
> Each command in the registration script is now logged into
> /var/log/openlmi-install.log file.
> 
> 
> Diffs
> -----
> 
>   openlmi-mof-register 4e60e31786e22b063e4c3a42116e05038f5275c3 
>   openlmi-providers.spec 0552a8cc01c8c3979a2304463f0a663fcc993a95 
> 
> Diff: http://reviewboard-openlmi.rhcloud.com/r/670/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Radek Novacek
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.fedorahosted.org/pipermail/openlmi-devel/attachments/20130801/b84e9671/attachment-0001.html>
-------------- next part --------------
_______________________________________________
openlmi-reviews mailing list
openlmi-reviews at lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/openlmi-reviews


More information about the openlmi-devel mailing list