[libvirt] [PATCH]: Fix allocation of tapfds when starting qemu
All, When doing the conversion to danpb's new memory API, a small bug was introduced into the qemudNetworkIfaceConnect() function. In particular, there is a call: if (VIR_ALLOC_N(vm->tapfds, vm->ntapfds+2) < 0) goto no_memory; However, the tapfds structure is used to track *all* of the tap fds, and is called once for each network that is being attached to the domain. VIR_ALLOC_N maps to calloc(). So the first network would work just fine, but if you had more than one network, subsequent calls to this function would blow away the stored fd's that were already there and fill them all in with zeros. This causes multiple problems, from the qemu domains not starting properly to improper cleanup on shutdown. The attached patch just changes the VIR_ALLOC_N() to a VIR_REALLOC_N(), and everything is happy again. Signed-off-by: Chris Lalancette <clalance@redhat.com>
On Thu, Jun 19, 2008 at 12:06:28PM +0200, Chris Lalancette wrote:
All, When doing the conversion to danpb's new memory API, a small bug was introduced into the qemudNetworkIfaceConnect() function. In particular, there is a call:
if (VIR_ALLOC_N(vm->tapfds, vm->ntapfds+2) < 0) goto no_memory;
However, the tapfds structure is used to track *all* of the tap fds, and is called once for each network that is being attached to the domain. VIR_ALLOC_N maps to calloc(). So the first network would work just fine, but if you had more than one network, subsequent calls to this function would blow away the stored fd's that were already there and fill them all in with zeros. This causes multiple problems, from the qemu domains not starting properly to improper cleanup on shutdown. The attached patch just changes the VIR_ALLOC_N() to a VIR_REALLOC_N(), and everything is happy again.
Signed-off-by: Chris Lalancette <clalance@redhat.com>
Dohh, okay, +1 please push :-) thanks ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Chris Lalancette <clalance@redhat.com> wrote:
All, When doing the conversion to danpb's new memory API, a small bug was introduced into the qemudNetworkIfaceConnect() function. In particular, there is a call:
if (VIR_ALLOC_N(vm->tapfds, vm->ntapfds+2) < 0) goto no_memory;
However, the tapfds structure is used to track *all* of the tap fds, and is called once for each network that is being attached to the domain. VIR_ALLOC_N maps to calloc(). So the first network would work just fine, but if you had more than one network, subsequent calls to this function would blow away the stored fd's that were already there and fill them all in with zeros. This causes multiple problems, from the qemu domains not starting properly to improper cleanup on shutdown. The attached patch just changes the VIR_ALLOC_N() to a VIR_REALLOC_N(), and everything is happy again.
Signed-off-by: Chris Lalancette <clalance@redhat.com> Index: src/qemu_conf.c =================================================================== RCS file: /data/cvs/libvirt/src/qemu_conf.c,v retrieving revision 1.78 diff -u -r1.78 qemu_conf.c --- a/src/qemu_conf.c 13 Jun 2008 07:56:59 -0000 1.78 +++ b/src/qemu_conf.c 19 Jun 2008 10:01:53 -0000 @@ -2317,7 +2317,7 @@ if (!(retval = strdup(tapfdstr))) goto no_memory;
- if (VIR_ALLOC_N(vm->tapfds, vm->ntapfds+2) < 0) + if (VIR_REALLOC_N(vm->tapfds, vm->ntapfds+2) < 0) goto no_memory;
vm->tapfds[vm->ntapfds++] = tapfd;
Yow. Another good catch. That's obviously the right fix. ACK. I went looking for similar bugs and actually found one! $ g show d3470efcda15f59549ac0aaa76cd25df319c217b \ |grep -A2 realloc|grep VIR_ALLOC + if (VIR_ALLOC_N(vm->tapfds, vm->ntapfds+2) < 0) + if (VIR_ALLOC_N(buf, alloc) < 0) { That's in the fread_file_lim function, and the fix is the same. To demonstrate, make virsh read a file containing more than BUFSIZ bytes, e.g., Create a legit definition, but with enough spaces at the end to push the size over the limit: { ./virsh -c test:///default dumpxml 1; printf %9000s ' ' } > t Demonstrate that virsh-0.4.2 reads/parses it fine: $ virsh --version 0.4.2 $ virsh --connect test:///default define t Domain test defined from t Show that just-built (pre-patch) virsh fails: $ ./virsh --version 0.4.3 $ ./virsh --connect test:///default define t libvir: Test error : XML description for domain is not well formed or invalid error: Failed to define domain from t [Exit 1] Show that patched, it works fine: $ ./virsh --connect test:///default define t Domain test defined from t $ Here's the patch I'll push soon:
From 02172b2c2e529a0afd04d5880ff8f32ad480ed9d Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 19 Jun 2008 12:36:36 +0200 Subject: [PATCH] virsh fails to read files larger than BUFSIZ bytes
* src/util.c (fread_file_lim): Use VIR_REALLOC_N, not VIR_ALLOC_N. Bug introduced in d3470efcda15f59549ac0aaa76cd25df319c217b --- src/util.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/src/util.c b/src/util.c index ad7683d..5e50ef2 100644 --- a/src/util.c +++ b/src/util.c @@ -306,7 +306,7 @@ fread_file_lim (FILE *stream, size_t max_len, size_t *length) if (alloc < size + BUFSIZ + 1) alloc = size + BUFSIZ + 1; - if (VIR_ALLOC_N(buf, alloc) < 0) { + if (VIR_REALLOC_N(buf, alloc) < 0) { save_errno = errno; break; } @@ -797,4 +797,3 @@ int virDiskNameToIndex(const char *name) { return idx; } - -- 1.5.6.rc3.23.gc3bdd
Jim Meyering wrote:
diff --git a/src/util.c b/src/util.c index ad7683d..5e50ef2 100644 --- a/src/util.c +++ b/src/util.c @@ -306,7 +306,7 @@ fread_file_lim (FILE *stream, size_t max_len, size_t *length) if (alloc < size + BUFSIZ + 1) alloc = size + BUFSIZ + 1;
- if (VIR_ALLOC_N(buf, alloc) < 0) { + if (VIR_REALLOC_N(buf, alloc) < 0) { save_errno = errno; break; } @@ -797,4 +797,3 @@ int virDiskNameToIndex(const char *name) {
return idx; }
Yep. Good catch. Confirmed by following your test procedure, and confirmed that this fixes the issue. ACK Chris Lalancette
Chris Lalancette <clalance@redhat.com> wrote:
Jim Meyering wrote:
diff --git a/src/util.c b/src/util.c index ad7683d..5e50ef2 100644 --- a/src/util.c +++ b/src/util.c @@ -306,7 +306,7 @@ fread_file_lim (FILE *stream, size_t max_len, size_t *length) if (alloc < size + BUFSIZ + 1) alloc = size + BUFSIZ + 1;
- if (VIR_ALLOC_N(buf, alloc) < 0) { + if (VIR_REALLOC_N(buf, alloc) < 0) { save_errno = errno; break; } @@ -797,4 +797,3 @@ int virDiskNameToIndex(const char *name) {
return idx; }
Yep. Good catch. Confirmed by following your test procedure, and confirmed that this fixes the issue.
ACK
Thanks for the quick review. I've gone ahead and added a small test script to exercise the bug, so the following is what I now expect to commit.
From d13e980b70194528a191ffb95333960d7b3d49fd Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 19 Jun 2008 13:41:23 +0200 Subject: [PATCH] virsh fails to read files larger than BUFSIZ bytes
* src/util.c (fread_file_lim): Use VIR_REALLOC_N, not VIR_ALLOC_N. Bug introduced in d3470efcda15f59549ac0aaa76cd25df319c217b. * tests/Makefile.am (test_scripts): Add read-bufsiz. * tests/read-bufsiz: New test for the above. --- src/util.c | 3 +-- tests/Makefile.am | 1 + tests/read-bufsiz | 43 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 2 deletions(-) create mode 100755 tests/read-bufsiz diff --git a/src/util.c b/src/util.c index ad7683d..5e50ef2 100644 --- a/src/util.c +++ b/src/util.c @@ -306,7 +306,7 @@ fread_file_lim (FILE *stream, size_t max_len, size_t *length) if (alloc < size + BUFSIZ + 1) alloc = size + BUFSIZ + 1; - if (VIR_ALLOC_N(buf, alloc) < 0) { + if (VIR_REALLOC_N(buf, alloc) < 0) { save_errno = errno; break; } @@ -797,4 +797,3 @@ int virDiskNameToIndex(const char *name) { return idx; } - diff --git a/tests/Makefile.am b/tests/Makefile.am index 303388c..4021a39 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -47,6 +47,7 @@ noinst_PROGRAMS = xmlrpctest xml2sexprtest sexpr2xmltest virshtest conftest \ test_scripts = \ daemon-conf \ int-overflow \ + read-bufsiz \ read-non-seekable \ vcpupin diff --git a/tests/read-bufsiz b/tests/read-bufsiz new file mode 100755 index 0000000..3037452 --- /dev/null +++ b/tests/read-bufsiz @@ -0,0 +1,43 @@ +#!/bin/sh +# ensure that reading a file larger than BUFSIZ works + +# Copyright (C) 2008 Free Software Foundation, Inc. + +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. + +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. + +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +if test "$VERBOSE" = yes; then + set -x + virsh --version +fi + +. $srcdir/test-lib.sh + +fail=0 + +# Output a valid definition, to be used as input. +virsh -c test:///default dumpxml 1 > xml || fail=1 + +for i in before after; do + # The largest BUFSIZ I've seen is 128K. This is slightly larger. + printf %132000s ' ' > sp || fail=1 + in=in-$i + # Append or prepend enough spaces to push the size over the limit: + ( test $i = before && cat sp xml || cat xml sp ) > $in || fail=1 + + virsh --connect test:///default define $in > out || fail=1 + printf "Domain test defined from $in\n\n" > exp || fail=1 + compare out exp || fail=1 +done + +(exit $fail); exit $fail -- 1.5.6.rc3.23.gc3bdd
On Thu, Jun 19, 2008 at 01:56:11PM +0200, Jim Meyering wrote:
Chris Lalancette <clalance@redhat.com> wrote:
Jim Meyering wrote:
diff --git a/src/util.c b/src/util.c index ad7683d..5e50ef2 100644 --- a/src/util.c +++ b/src/util.c @@ -306,7 +306,7 @@ fread_file_lim (FILE *stream, size_t max_len, size_t *length) if (alloc < size + BUFSIZ + 1) alloc = size + BUFSIZ + 1;
- if (VIR_ALLOC_N(buf, alloc) < 0) { + if (VIR_REALLOC_N(buf, alloc) < 0) { save_errno = errno; break; } @@ -797,4 +797,3 @@ int virDiskNameToIndex(const char *name) {
return idx; }
Yep. Good catch. Confirmed by following your test procedure, and confirmed that this fixes the issue.
ACK
Thanks for the quick review. I've gone ahead and added a small test script to exercise the bug, so the following is what I now expect to commit.
yes please ! +1 thanks ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Thu, Jun 19, 2008 at 12:06:28PM +0200, Chris Lalancette wrote:
All, When doing the conversion to danpb's new memory API, a small bug was introduced into the qemudNetworkIfaceConnect() function. In particular, there is a call:
if (VIR_ALLOC_N(vm->tapfds, vm->ntapfds+2) < 0) goto no_memory;
However, the tapfds structure is used to track *all* of the tap fds, and is called once for each network that is being attached to the domain. VIR_ALLOC_N maps to calloc(). So the first network would work just fine, but if you had more than one network, subsequent calls to this function would blow away the stored fd's that were already there and fill them all in with zeros. This
Ahhh, so this is why it was only hitting oVirt - all my tests normally just had a single network interface. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (4)
-
Chris Lalancette -
Daniel P. Berrange -
Daniel Veillard -
Jim Meyering