On 03/11/2014 06:40 AM, Cédric Bosdonnat wrote:
> ---
> Makefile.am | 2 +-
> configure.ac | 1 +
> examples/lxcconvert/Makefile.am | 19 ++++++++++
> examples/lxcconvert/virt-lxc-convert | 67 ++++++++++++++++++++++++++++++++++++
> 4 files changed, 88 insertions(+), 1 deletion(-)
> create mode 100644 examples/lxcconvert/Makefile.am
> create mode 100644 examples/lxcconvert/virt-lxc-convert
>
> +++ b/examples/lxcconvert/Makefile.am
...
> +EXTRA_DIST= \
> + virt-lxc-convert
> +
Fails 'make syntax-check':
prohibit_empty_lines_at_EOF
examples/lxcconvert/Makefile.am
maint.mk: empty line(s) or no newline at EOF
> +++ b/examples/lxcconvert/virt-lxc-convert
> @@ -0,0 +1,67 @@
> +#!/usr/bin/env sh
'env sh' is overkill; you are guaranteed that /bin/sh exists; and
furthermore, since we are talking about LXC and therefore Linux, that it
is a POSIX sh (compared to the more generic case where you have to worry
about braindead Solaris /bin/sh).
> +
> +function show_help()
'function' is a bash-ism. To be portable to /bin/sh, this MUST be:
show_help ()
> +{
> + cat << EOF
> +$0 /path/to/lxc/config/file
> +
> +Wrapper around virsh domxml-from-native to ease conversion of LXC
> +containers configuration to libvirt domain XML.
> +EOF
> +}
> +
> +if test "$#" != 1; then
"" around $# is redundant (doesn't hurt if you are using a consistent
style, but the result is guaranteed to be a number not subject to word
splitting)
It might also be nice to support '--help'.
> + show_help
> + exit 1
although if you do add support for --help, that particular case should
exit with status 0 (or more technically correct, with the exit status of
the cat process, in case there was a write failure when printing the help)
> +
> +conf_new=/tmp/config-$$
> +domain_new=/tmp/config-$$.xml
Eww. Totally insecure. Consider using mktemp(1)
> +
> +cp $conf $conf_new
Needs "" around $conf, since it comes from the user and may contain
spaces. Your choice of $conf_new does not contain spaces, but it can't
hurt to be consistent in the use of "".
> +
> +# Do we have lxc.mount, and is it pointing to a readable file?
> +fstab=`grep 'lxc.mount[[:space:]]*=' $conf_new | sed -e 's/[[:space:]]\+=[[:space:]]\+/=/' | cut -f 2 -d '='`
Use $(), not ``
grep|sed is a waste of a process. Also, \+ is not portable sed. It's
simpler to:
sed -n '/lxc.mount[[:space:]]*=/ s/[[:space:]]*=[[:space:]]*/=/p' | cut...
Long lines; it would be nice to keep things under 80 columns.
> +if test \( -n "$fstab" \) -a \( -r "$fstab" \); then
'test \(' and 'test -a' are not portable. This MUST be:
if test -n "$fstab" && test -r "$fstab"; then
> + sed -i -e 's/^lxc.mount[[:space:]]*=.*$//' $conf_new
'sed -i' is not portable. But since LXC targets Linux, we can be
reasonably safe assuming that GNU sed is installed and you can get away
with it.
> + sed -e 's/^\([^#]\)/lxc.mount.entry = \1/' $fstab | cat $conf_new - >${conf_new}.tmp
Need "" around $fstab.
> + mv ${conf_new}.tmp $conf_new
Useless use of cat and mv. Just do 'sed ... >> $conf_new'.
> +fi
> +
> +memory=`free | grep 'Mem:' | sed -e 's: \+: :g' | cut -f 2 -d ' '`
$(). free is a Linux-ism (but this deals with LXC, so I guess we are
okay). grep|sed is a waste of a process. \+ is not portable sed.
's:::' looks odd compared to the typical 's///'.
> +default_tmpfs="size=$((memory/2))"
> +
> +# Do we have tmpfs without size param?
> +grep -e 'lxc.mount.entry[[:space:]]*=[[:space:]]*tmpfs[[:space:]]' $conf_new | while read line; do
Why 'grep -e' when you didn't use any extended regex capabilities?
> + has_size=`echo $line | grep -v -e 'size='`
Why 'grep -e'? $() instead of ``, or even better to do this natively in
shell instead of forking processes:
case $line in
size=*) has_size=: ;;
*) has_size=false ;;
esac
> + # Add the default size here (50%) if no size is given
> + if test -n "$has_size"; then
and if you use case to determine has_size, then this would be
if $has_size; then
> + sed -i -e "\;$line;s/\([[:space:]]\+[0-9][[:space:]]\+[0-9][::space::]*$\)/,$default_tmpfs\1/" $conf_new
Unsafe if $line contains ';'. And I'm probably fighting a losing battle
for avoiding \+ or sed -i
> + fi
> + # Convert relative sizes
> + has_rel_size=`echo $line | grep -e 'size=[0-9]\+%'`
Similar comments as for $has_size via 'case' instead of forking
> + if test -n "$has_rel_size"; then
> + percent=`echo "$line" | sed -e 's:size=\([0-9]\+\)%:\n\1%\n:' | grep '\%' | sed -e 's/%//'`
Simpler and more portable as:
percent=$(echo "$line" | sed 's/.*size=\([0-9][0-9]\*\)%.*/\1/')
> + size="$((memory*percent/100))"
> + sed -i -e "\;$line;s/size=[0-9]\+%/size=${size}/" $conf_new
Again, problematic if $line contains ';'
> + fi
> +done
> +
> +# Do we have any memory limit set?
> +mem_limit=`grep 'lxc.cgroup.memory.limit_in_bytes[[:space:]]*=' $conf_new`
$()
> +if test -z "$mem_limit"; then
> + sed -i -e "1s:^:lxc.cgroup.memory.limit_in_bytes = $memory\n:" $conf_new
Why use sed when you can just:
echo "lxc.cgroup.memory.limit_in_bytes = $memory" >> $conf_new
> +fi
> +
> +virsh -c lxc:/// domxml-from-native lxc-tools $conf_new >$domain_new
> +
> +cat $domain_new
> +
> +# Remove the temporary config
> +rm $conf_new
> +rm $domain_new
This should probably be installed via a trap handler to happen even if
the script gets terminated by Ctrl-C in the middle of work.
> +
> +exit 0
>
Misleading to exit with success if something failed along the way; for
example, you didn't check if 'virsh' succeeded. I'm NOT a fan of 'set
-e', and prefer manual error handling; but some people manage to use
'set -e' successfully and I can at least review for pitfalls if you
choose to attempt that.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list