[libvirt] [PATCH] Added example script on how to convert LXC container config

Thu Mar 27 15:50:06 UTC 2014

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140327/75018e02/attachment-0001.sig>