Alon Bar-Lev has posted comments on this change.

Change subject: Extend vdsm-tool: moving configure libvirt to external shell 
script
......................................................................


Patch Set 19: (3 inline comments)

....................................................
File lib/vdsm/tool/libvirt_configure.sh.in
Line 45:         echo 'FAILED: Could not read SSL configuration' 1>&2
Line 46:         return 3
Line 47:     fi
Line 48: 
Line 49:     if [ "$ssl" = "true" ]; then
all variables should go into "${xxx}" notation.
Line 50:         echo "SUCCESS: ssl configured to true. No conflicts"
Line 51:         return 0
Line 52:     fi
Line 53: 


Line 56:     local spice_tls="$(get_libvirt_conf_item "${qconf}" spice_tls)"
Line 57: 
Line 58:     if [ "$listen_tcp" = "1" -a \
Line 59:         "$auth_tcp" = '"none"' -a \
Line 60:         "$spice_tls" = "0" ]; then
decide tabs or spaces, but to the entire file. I love tabs... but it is a 
python project...
Line 61:         echo "SUCCESS: No conflicts between configuration files"
Line 62:         return 0
Line 63:     else
Line 64:         echo "FAILED: conflicting vdsm and libvirt-qemu tls 
configuration."


Line 283:             # Conf files as variables to allow pass them as parameter
Line 284:             start_configure "$2" "$3" "$4" "$5" "$6"
Line 285:         else
Line 286:             start_configure "$LCONF" "$QCONF" "$LDCONF" "$QLCONF" "$2"
Line 287:         fi
I think you should switch to long arguments and not positional.

 [--force] [--lconf=xxx] [--qconf=yyy] ...

See example[1] of simple argument parsing.

now that you pass transparent arguments no change in python is required.

[1] 
http://gerrit.ovirt.org/gitweb?p=ovirt-engine.git;a=blob;f=packaging/bin/pki-create-ca.sh;hb=HEAD#l102
Line 288:         RETVAL=$?
Line 289:         ;;
Line 290:      test_conflict_configurations)
Line 291:         echo "Checking conflicts ..."


-- 
To view, visit http://gerrit.ovirt.org/15216
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id58b129afbf141a47a85b421961bf5b1776b41e4
Gerrit-PatchSet: 19
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Bala.FA <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Giuseppe Vallarelli <[email protected]>
Gerrit-Reviewer: Yaniv Bronhaim <[email protected]>
Gerrit-Reviewer: Zhou Zheng Sheng <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Reply via email to