10:50 a.m.
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