[Ipmitool-devel] Code Review - ID: 84 bmc-snmp-proxy bug fixes

[Zdenek Styblik]

On Mon, Dec 9, 2013 at 10:01 AM, Zdenek Styblik

Hello Charles,
here is my review of your patch.
~~~
+# printf "view bmcview included %s 80\n" "${BMC_OID}"
+# printf "com2sec -Cn bmc_ctx bmc_sec default bmc_cmty\n"
+# printf "group bmc_grp v1 bmc_sec\n"
+# printf "access bmc_grp bmc_ctx any noauth exact bmcview none none\n"
+# printf "proxy -Cn bmc_ctx -v 1 %s\n" "${PROXY_TOKEN}"
+ printf "proxy -v 1 %s\n" "${PROXY_TOKEN}"
~~~
Why are we going to keep commented stuff around?
~~~
+ mkdir ${SNMPD_BMC_CONF_DIR}
+ [ $? -ne 0 ] && RETVAL=7
~~~
-> ``mkdir ${SNMPD_BMC_CONF_DIR} || RETVAL=7''
I'd add a white space here and there.
I haven't tested your changes and review is based on reading-through.
Best regards,
Z.

I forgot to add ipmitool-devel@ on CC.

Z.

--
Zdenek Styblik

[C***@Dell.com]

Post by Zdenek Styblik
On Mon, Dec 9, 2013 at 10:01 AM, Zdenek Styblik

Hello Charles,
here is my review of your patch.

Thank you. I will include your comments in an updated patch to #289. Please close this ticket.

Post by Zdenek Styblik

~~~
+# printf "view bmcview included %s 80\n" "${BMC_OID}"
+# printf "com2sec -Cn bmc_ctx bmc_sec default bmc_cmty\n"
+# printf "group bmc_grp v1 bmc_sec\n"
+# printf "access bmc_grp bmc_ctx any noauth exact bmcview none none\n"
+# printf "proxy -Cn bmc_ctx -v 1 %s\n" "${PROXY_TOKEN}"
+ printf "proxy -v 1 %s\n" "${PROXY_TOKEN}"

I will change this to this instead. So users who might want to turn on v3 security can uncomment these:
printf "#view ... "

Post by Zdenek Styblik

~~~
Why are we going to keep commented stuff around?
~~~
+ mkdir ${SNMPD_BMC_CONF_DIR}
+ [ $? -ne 0 ] && RETVAL=7
~~~
-> ``mkdir ${SNMPD_BMC_CONF_DIR} || RETVAL=7''

Agree.

Post by Zdenek Styblik

I'd add a white space here and there.

Will do.

Post by Zdenek Styblik

I haven't tested your changes and review is based on reading-through.
Best regards,
Z.

Z.

--
Zdenek Styblik

--
Charles Rose
Linux Engineering
Dell Inc.