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