Re: [libvirt] [PATCH]: Fix allocation of tapfds when starting qemu
On Thu, Jun 19, 2008 at 01:56:11PM +0200, Jim Meyering wrote:
> Chris Lalancette <[EMAIL PROTECTED]> 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/
[EMAIL PROTECTED] | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH]: Fix allocation of tapfds when starting qemu
Chris Lalancette <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]>
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 000..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
Re: [libvirt] [PATCH]: Fix allocation of tapfds when starting qemu
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 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH]: Fix allocation of tapfds when starting qemu
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
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH]: Fix allocation of tapfds when starting qemu
Chris Lalancette <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]>
> 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 - 1.78
> +++ b/src/qemu_conf.c 19 Jun 2008 10:01:53 -
> @@ -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
invaliderror: 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 <[EMAIL PROTECTED]>
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
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH]: Fix allocation of tapfds when starting qemu
Daniel Veillard wrote: > On Thu, Jun 19, 2008 at 12:06:28PM +0200, Chris Lalancette wrote: > > Dohh, okay, +1 please push :-) > >thanks ! Committed. Chris Lalancette -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH]: Fix allocation of tapfds when starting qemu
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 <[EMAIL PROTECTED]> Dohh, okay, +1 please push :-) thanks ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ [EMAIL PROTECTED] | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[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 <[EMAIL PROTECTED]> 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 - 1.78 +++ b/src/qemu_conf.c 19 Jun 2008 10:01:53 - @@ -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; -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
