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