[libvirt] [PATCH] doc: fix incorrect option in blockjob
* tools/virsh.pod: change 'live' to 'abort' based on actual option. Signed-off-by: Alex Jia a...@redhat.com --- tools/virsh.pod |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/tools/virsh.pod b/tools/virsh.pod index f6f9908..01b8fd6 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -445,7 +445,7 @@ Ibandwidth specifies copying bandwidth limit in Mbps. Manage active block operations. Ipath specifies fully-qualified path of the disk. -If I--live is specified, the active job on the specified disk will +If I--abort is specified, the active job on the specified disk will be aborted. If I--info is specified, the active job information on the specified disk will be printed. -- 1.7.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] doc: fix incorrect option in blockjob
On Tue, Aug 02, 2011 at 02:09:31PM +0800, a...@redhat.com wrote: * tools/virsh.pod: change 'live' to 'abort' based on actual option. Signed-off-by: Alex Jia a...@redhat.com --- tools/virsh.pod |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/tools/virsh.pod b/tools/virsh.pod index f6f9908..01b8fd6 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -445,7 +445,7 @@ Ibandwidth specifies copying bandwidth limit in Mbps. Manage active block operations. Ipath specifies fully-qualified path of the disk. -If I--live is specified, the active job on the specified disk will +If I--abort is specified, the active job on the specified disk will be aborted. If I--info is specified, the active job information on the specified disk will be printed. Ah, right. Applied and pushed, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] virsh: fix memory leak in cmdNetworkInfo
I have ever discussed this issue with Matthias, and he has fixed this leak in cmdNetworkInfo codes, but he hasn't committed a patch to upstream so that bug 722806 is always assigned status, so commit it to avoid this patch missing. * tools/virsh.c: avoid memory leak in cmdNetworkInfo. * how to reproduce? % valgrind -v --leak-check=yes virsh net-info default https://bugzilla.redhat.com/show_bug.cgi?id=722806 Signed-off-by: Alex Jia a...@redhat.com --- tools/virsh.c |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 23e71d7..cf8e2a5 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -5597,6 +5597,7 @@ cmdNetworkInfo(vshControl *ctl, const vshCmd *cmd) if (bridge) vshPrint(ctl, %-15s %s\n, _(Bridge:), bridge); +VIR_FREE(bridge); virNetworkFree(network); return true; } -- 1.7.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virsh: fix memory leak in cmdNetworkInfo
于 2011年08月02日 16:31, Alex Jia 写道: I have ever discussed this issue with Matthias, and he has fixed this leak in cmdNetworkInfo codes, but he hasn't committed a patch to upstream so that bug 722806 is always assigned status, so commit it to avoid this patch missing. * tools/virsh.c: avoid memory leak in cmdNetworkInfo. * how to reproduce? % valgrind -v --leak-check=yes virsh net-info default https://bugzilla.redhat.com/show_bug.cgi?id=722806 Signed-off-by: Alex Jiaa...@redhat.com --- tools/virsh.c |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 23e71d7..cf8e2a5 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -5597,6 +5597,7 @@ cmdNetworkInfo(vshControl *ctl, const vshCmd *cmd) if (bridge) vshPrint(ctl, %-15s %s\n, _(Bridge:), bridge); +VIR_FREE(bridge); virNetworkFree(network); return true; } Ack and pushed. Osier -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] rpc: avoid libvirtd crash on unexpected client close
On Mon, Aug 01, 2011 at 01:53:19PM -0600, Eric Blake wrote:
Steps to reproduce this problem (vm1 is not running):
for i in `seq 50`; do virsh managedsave vm1 done; killall virsh
Pre-patch, virNetServerClientClose could end up setting client-sock
to NULL prior to other cleanup functions trying to use client-sock.
This fixes things by checking for NULL in more places, and by deferring
the cleanup until after all queued messages have been served.
* src/rpc/virnetserverclient.c (virNetServerClientRegisterEvent)
(virNetServerClientGetFD, virNetServerClientIsSecure)
(virNetServerClientLocalAddrString)
(virNetServerClientRemoteAddrString): Check for closed socket.
(virNetServerClientClose): Rearrange close sequence.
Analysis from Wen Congyang.
---
This fixes the problem using the reproduction formula (that is,
pre-patch I reproduced the failure; valgrind showed it at:
==29390== Thread 4:
==29390== Invalid read of size 4
==29390==at 0x3D16608FA0: pthread_mutex_lock (pthread_mutex_lock.c:50)
==29390==by 0x4E9FED2: virMutexLock (threads-pthread.c:85)
==29390==by 0x45DA5E: virNetSocketGetLocalIdentity (virnetsocket.c:741)
==29390==by 0x45554A: virNetServerClientGetLocalIdentity
(virnetserverclient.c:401)
==29390==by 0x4420E3: remoteDispatchAuthList (remote.c:1682)
==29390==by 0x4224E4: remoteDispatchAuthListHelper (remote_dispatch.h:19)
==29390==by 0x453EFD: virNetServerProgramDispatchCall
(virnetserverprogram.c:375)
==29390==by 0x4539FF: virNetServerProgramDispatch
(virnetserverprogram.c:252)
==29390==by 0x456B20: virNetServerHandleJob (virnetserver.c:155)
==29390==by 0x4EA06A3: virThreadPoolWorker (threadpool.c:98)
==29390==by 0x4EA01D6: virThreadHelper (threads-pthread.c:157)
==29390==by 0x3D16606CCA: start_thread (pthread_create.c:301)
==29390== Address 0x10 is not stack'd, malloc'd or (recently) free'd
post-patch, libvirtd stays alive and valgrind no longer reports
an invalid read). But I'd really like danpb's opinion, if there
is time to get that before 0.9.4.
/me note to self 'git send-email --cc=...' uses cc as well as
honoring .git/config --to to the list; but the list manager
sometimes strips cc: lines. Converserly, 'git send-email --to=...'
overrides .git/config settings, leaving out the list. I can't
win; sorry for the private mails on my previous send attempt.
src/rpc/virnetserverclient.c | 29 +++--
1 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index 3c0dba8..2f6c040 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -177,7 +177,8 @@ static int
virNetServerClientRegisterEvent(virNetServerClientPtr client)
client-refs++;
VIR_DEBUG(Registering client event callback %d, mode);
-if (virNetSocketAddIOCallback(client-sock,
+if (!client-sock ||
+virNetSocketAddIOCallback(client-sock,
mode,
virNetServerClientDispatchEvent,
client,
@@ -386,9 +387,10 @@ int
virNetServerClientGetTLSKeySize(virNetServerClientPtr client)
int virNetServerClientGetFD(virNetServerClientPtr client)
{
-int fd = 0;
+int fd = -1;
virNetServerClientLock(client);
-fd = virNetSocketGetFD(client-sock);
+if (client-sock)
+fd = virNetSocketGetFD(client-sock);
virNetServerClientUnlock(client);
return fd;
}
@@ -396,9 +398,10 @@ int virNetServerClientGetFD(virNetServerClientPtr client)
int virNetServerClientGetLocalIdentity(virNetServerClientPtr client,
uid_t *uid, pid_t *pid)
{
-int ret;
+int ret = -1;
virNetServerClientLock(client);
-ret = virNetSocketGetLocalIdentity(client-sock, uid, pid);
+if (client-sock)
+ret = virNetSocketGetLocalIdentity(client-sock, uid, pid);
virNetServerClientUnlock(client);
return ret;
}
@@ -413,7 +416,7 @@ bool virNetServerClientIsSecure(virNetServerClientPtr
client)
if (client-sasl)
secure = true;
#endif
-if (virNetSocketIsLocal(client-sock))
+if (client-sock virNetSocketIsLocal(client-sock))
secure = true;
virNetServerClientUnlock(client);
return secure;
@@ -502,12 +505,16 @@ void
virNetServerClientSetDispatcher(virNetServerClientPtr client,
const char *virNetServerClientLocalAddrString(virNetServerClientPtr client)
{
+if (!client-sock)
+return NULL;
return virNetSocketLocalAddrString(client-sock);
}
const char *virNetServerClientRemoteAddrString(virNetServerClientPtr client)
{
+if (!client-sock)
+return NULL;
return virNetSocketRemoteAddrString(client-sock);
}
All thes changes look correct desirable.
@@ -570,10 +577,7 @@ void
Re: [libvirt] virNetClientPtr leak in remote driver
On Mon, Aug 01, 2011 at 06:35:21PM +0200, Matthias Bolte wrote: 2011/8/1 Eric Blake ebl...@redhat.com: On 07/28/2011 12:07 PM, Matthias Bolte wrote: 2011/7/27 Matthias Bolte matthias.bo...@googlemail.com: doRemoteClose doesn't free the virNetClientPtr and this creates a 260kb leak per remote connection. This happens because virNetClientFree doesn't remove the last ref, because virNetClientNew creates the virNetClientPtr with a refcount of 2. The memory leak I saw was due to virsh calling virEventRegisterDefaultImpl, but not calling virEventRunDefaultImpl. Because the event loop is initialized, the call to virNetSocketAddIOCallback in virNetClientNew succeeds. But virsh not driving the event loop results in not removing callbacks that were marked for deletion. Finally this leaks the virNetClientPtr using in the remote driver. I used a virsh -c qemu:///system to test. I was able fix this by calling virEventRunDefaultImpl after virConnectClose in virsh. But I don't think that this is the correct general solution. Where do we stand with 0.9.4? Is this something where we need the general fix before the release, or is just the virsh hack to call virEventRunDefaultImpl good enough for the release, or is this problem not severe enough and we can release as-is? virsh is a bit special here as it registers/initializes the default even loop but doesn't drive it properly. It only drives it for the console command. That's the problem in virsh. This can be avoided by calling virEventRunDefaultImpl after virConnectClose iff it's a remote connection, in other cases virEventRunDefaultImpl might block. Therefore, we shouldn't be calling it in general after a virConnectClose in virsh. If we assume that an application that registers an event loop will drive it properly then this problem is not critical, as it doesn't exist. Just virsh is a special case that leaks 260kb per closed remote connection. When we assume that a typical virsh user uses a single connection per virsh invocation then we can view this as a static leak. Yeah, this is a case I never thought of. The easy fix is to just make virsh spawn a new thread to run the event loop in the background. The hard fix is to make virsh I/O entirely event driven, so that we don't just sit in a blocking read of stdin waiting for input, but instead use the event loop to process stdin. Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv4 1/2] conf: add listen subelement to domain graphics element
On Thu, Jul 28, 2011 at 11:09:40AM -0600, Eric Blake wrote: On 07/28/2011 12:11 AM, Laine Stump wrote: Once it's plugged in, thelisten element will be an optional replacement for the listen attribute that graphics elements already have. If thelisten element is type='address', it will have an attribute called 'address' which will contain an IP address or dns name that the guest's display server should listen on. If, however, type='network', thelisten element should have an attribute called 'network' that will be set to the name of a network configuration to get the IP address from. I think we're there. Thanks for putting up with the review churn. +++ b/docs/formatdomain.html.in @@ -2046,6 +2046,12 @@ qemu-kvm -net nic,model=? /dev/null lt;graphics type='vnc' port='5904'/gt; It looks fishy to have one... lt;graphics type='rdp' autoport='yes' multiUser='yes' /gt; lt;graphics type='desktop' fullscreen='yes'/gt; +lt;graphics type='vnc'gt; +lt;listen type='address' address='1.2.3.4'/gt; +lt;/graphicsgt; ...and then a second type='vnc' description (do we support multiple vnc graphics adapters on any existing hypervisor?). How about consolidating this part of the example into just: FYI with virtualbox you can have multiple graphics elements present, though they should be of different types. Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] freebsd: Add gnulib environ module for the commandtest
On Thu, Jul 28, 2011 at 07:56:18AM -0600, Eric Blake wrote: On 07/28/2011 07:52 AM, Matthias Bolte wrote: At least all tests compile on FreeBSD again. But most of the SSH cases in virnetmessagetest are failing and I don't understand why yet. Could it be a PATH vs. exec() issue, where BSD ends up doing a slightly different PATH search and not executing the dummy 'ssh' script from our test directory? Does a ktrace (or truss or strace or however it's spelled) shed any light? NB, if you run ./virnetsockettest it won't work. You have to run PATH=`pwd`:$PATH ./virnetsockettest Or make check TESTS=virnetsockettest to ensure the $PATH is set to find the local fake ssh Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [virt-tools-list] ANNOUNCE: virt-manager 0.9.0 and virtinst 0.600.0 released
On Mon, Aug 01, 2011 at 09:13:07AM -0400, Cole Robinson wrote: On 07/30/2011 01:15 PM, Richard W.M. Jones wrote: On Thu, Jul 28, 2011 at 12:31:10PM -0400, Cole Robinson wrote: - Use libguestfs to show guest packagelist and more (Richard W.M. Jones) I had one complaint that the dependency on python-libguestfs pulls in qemu (from someone using virt-manager to remotely manage systems, so they don't want qemu locally and it wouldn't be any use to them for libguestfs). This is hard to solve, unless RPM grows soft dependencies like dpkg, which unfortunately has been rejected many times upstream even though it's a good feature of dpkg. We might _not_ make it a dependency at all. Virt-manager will still work, just without the enhanced inspection features. Just so you know ... Yeah I had a similar report (or maybe the same one?) Figuring that we go to great pains in virt-manager to avoid having a dep on qemu or libvirtd (since virt-manager doesn't need either to be useful), we can't have a hard dep on libguestfs, so I'll be dropping it for the Fedora packages. Make sure that python-libguestfs is included in the 'Virtualization' group of Fedora comps.xml then. So that when someone does 'yum groupinstall Virtualizaation' or selects Virtualization in Anaconda, they get all the bits by default Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fix bug #611823 prohibit pools with duplicate storage
On Fri, Jul 29, 2011 at 12:34:15PM +0800, Lei Li wrote:
To make sure the unique storage pool defined and created from different
directory to avoid inconsistent version of volume pool created, I add two API
be called by storage driver to check for the probable duplicate pools and
refused the duplicate pool.
virStoragePoolObjFindByPath() provide a method to find pool object by target
path in pool list.
virStoragePoolTargetDuplicate() implement the function to check if there is
duplicate pool.
Add judgement for storagePoolCreatestoragePoolDefine by calling
virStoragePoolTargetDuplicate() to avoid both transient storage pool and
persistent storage pool be created repeatedly in storage driver.
Signed-off-by: Lei Lili...@linux.vnet.ibm.com
---
src/conf/storage_conf.c | 39 +++
src/conf/storage_conf.h |4
src/libvirt_private.syms |2 ++
src/storage/storage_driver.c |6 ++
4 files changed, 51 insertions(+), 0 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 995f9a6..a499e82 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -1317,6 +1317,22 @@ virStoragePoolObjFindByName(virStoragePoolObjListPtr
pools,
return NULL;
}
+virStoragePoolObjPtr
+virStoragePoolObjFindByPath(virStoragePoolObjListPtr pools,
+const char *path)
+{
+unsigned int i;
+
+for (i = 0 ; i pools-count ; i++) {
+virStoragePoolObjLock(pools-objs[i]);
+ if (STREQ(pools-objs[i]-def-target.path, path))
+ return pools-objs[i];
+ virStoragePoolObjUnlock(pools-objs[i]);
+}
+
+return NULL;
+}
+
void
virStoragePoolObjClearVols(virStoragePoolObjPtr pool)
{
@@ -1707,6 +1723,29 @@ cleanup:
return ret;
}
+int virStoragePoolTargetDuplicate(virStoragePoolObjListPtr pools,
+ virStoragePoolDefPtr def)
+{
+int ret = 1;
+virStoragePoolObjPtr pool = NULL;
+
+/*check pool list if defined target path already exist*/
+pool = virStoragePoolObjFindByPath(pools, def-target.path);
+
+if (pool) {
+virStorageReportError(VIR_ERR_OPERATION_FAILED,
+ _(target path '%s' is already in use),
+ pool-def-target.path);
+ret = -1;
+goto cleanup;
+}
This only works for pools which are type=dir|fs|netfs|logical
It will fail for type=iscsi|scsi|disk, because in those cases the
target path has no uniqueness requirement, and will almost always
just be either /dev or /dev/disk/by-path for all pool.
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Libvirt] [PATCH v2] Fix bug #611823 prohibit pools with duplicate storage
On Mon, Aug 01, 2011 at 02:12:51PM -0600, Eric Blake wrote:
On 07/31/2011 10:58 PM, Lei Li wrote:
Make sure the unique storage pool defined and create from different
directory to avoid inconsistent version of volume pool created.
Wrap your commit messages; typically at 70 columns or so (since 'git
log' adds some indentation, but you want the end result to still fit
in 80 columns for legibility).
Signed-off-by: Lei Lili...@linux.vnet.ibm.com
---
src/conf/storage_conf.c | 36
src/conf/storage_conf.h |4
src/libvirt_private.syms |2 ++
src/storage/storage_driver.c |6 ++
4 files changed, 48 insertions(+), 0 deletions(-)
+virStoragePoolObjPtr
+virStoragePoolObjFindByPath(virStoragePoolObjListPtr pools,
+const char *path) {
+unsigned int i;
+
+for (i = 0 ; i pools-count ; i++) {
+virStoragePoolObjLock(pools-objs[i]);
+if (STREQ(pools-objs[i]-def-target.path, path))
+return pools-objs[i];
+virStoragePoolObjUnlock(pools-objs[i]);
+}
+
+return NULL;
+}
This one is good; in fact, we may even want to expose it as a public
API, parallel to other virStoragePoolLookupBy* functions (but not
until after 0.9.4 is released)
No, this API is flawed because def-target.path is not required to
be unique for all types of storage pool.
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Libvirt] [PATCH v3] Fix bug #611823 prohibit pools with duplicate storage
On Tue, Aug 02, 2011 at 10:42:47AM +0800, Lei Li wrote:
Make sure the unique storage pool defined and create from different
directory to avoid inconsistent version of volume pool created.
Signed-off-by: Lei Li li...@linux.vnet.ibm.com
---
src/conf/storage_conf.c | 25 +
src/conf/storage_conf.h |2 ++
src/libvirt_private.syms |1 +
3 files changed, 28 insertions(+), 0 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 995f9a6..1d9fe25 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -1317,6 +1317,21 @@ virStoragePoolObjFindByName(virStoragePoolObjListPtr
pools,
return NULL;
}
+virStoragePoolObjPtr
+virStoragePoolObjFindByPath(virStoragePoolObjListPtr pools,
+const char *path) {
+unsigned int i;
+
+for (i = 0 ; i pools-count ; i++) {
+virStoragePoolObjLock(pools-objs[i]);
+if (STREQ(pools-objs[i]-def-target.path, path))
+return pools-objs[i];
+virStoragePoolObjUnlock(pools-objs[i]);
+}
+
+return NULL;
+}
+
void
virStoragePoolObjClearVols(virStoragePoolObjPtr pool)
{
@@ -1700,6 +1715,16 @@ int
virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools,
}
}
+/* Check the pool list if defined target path already exist */
+pool = virStoragePoolObjFindByPath(pools, def-target.path);
+if (pool) {
+virStorageReportError(VIR_ERR_OPERATION_FAILED,
+ _(target path '%s' is already in use),
+ pool-def-target.path);
+dupPool = -1;
+goto cleanup;
+}
+
ret = dupPool;
cleanup:
if (pool)
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h
index 271441a..9239977 100644
--- a/src/conf/storage_conf.h
+++ b/src/conf/storage_conf.h
@@ -335,6 +335,8 @@ virStoragePoolObjPtr
virStoragePoolObjFindByUUID(virStoragePoolObjListPtr pools,
const unsigned char *uuid);
virStoragePoolObjPtr virStoragePoolObjFindByName(virStoragePoolObjListPtr
pools,
const char *name);
+virStoragePoolObjPtr virStoragePoolObjFindByPath(virStoragePoolObjListPtr
pools,
+ const char *path);
virStorageVolDefPtr virStorageVolDefFindByKey(virStoragePoolObjPtr pool,
const char *key);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 830222b..19f5f92 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -937,6 +937,7 @@ virStoragePoolObjClearVols;
virStoragePoolObjDeleteDef;
virStoragePoolObjFindByName;
virStoragePoolObjFindByUUID;
+virStoragePoolObjFindByPath;
virStoragePoolObjIsDuplicate;
virStoragePoolObjListFree;
virStoragePoolObjLock;
NACK, for the reasons I mentioned in my review of an earlier
version of this patch
Regards,
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3 RFC] macvtap: Implement getPhysfn to support sending a port profile message for a VF to its PF
On Mon, Aug 01, 2011 at 09:16:09PM -0400, Dave Allan wrote: On Mon, Aug 01, 2011 at 04:56:58PM -0700, Roopa Prabhu wrote: This patch tries to fix getPhysFn in macvtap.c to get the physical function(PF) of the direct attach interface, if the interface is a SR-IOV VF. It derives the PF/VF relationship from sysfs. There is some code to do this in node device driver. But it is local to the node device driver . I did not see a clean way to use some of the node device stuff here. The node device driver looks at PCI capabilities to get the same information. We should not have two implementations of this functionality in the code. Either the node device code should be made to use this version or vice versa. We already have a file src/util/pci.c that contains a bunch of helper APIs for dealing with PCI devices. If we need some shared code between macvtap and the node device driver, then we ought to put it in the pci.c file Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] freebsd: Add gnulib environ module for the commandtest
2011/8/2 Daniel P. Berrange berra...@redhat.com: On Thu, Jul 28, 2011 at 07:56:18AM -0600, Eric Blake wrote: On 07/28/2011 07:52 AM, Matthias Bolte wrote: At least all tests compile on FreeBSD again. But most of the SSH cases in virnetmessagetest are failing and I don't understand why yet. Could it be a PATH vs. exec() issue, where BSD ends up doing a slightly different PATH search and not executing the dummy 'ssh' script from our test directory? Does a ktrace (or truss or strace or however it's spelled) shed any light? NB, if you run ./virnetsockettest it won't work. You have to run PATH=`pwd`:$PATH ./virnetsockettest That's true and this way is works on FreeBSD too. Or make check TESTS=virnetsockettest to ensure the $PATH is set to find the local fake ssh This way it doesn't because the makefile extends the path in a non-portable way. Eric fixed this http://libvirt.org/git/?p=libvirt.git;a=commit;h=343ab98229a60126ec75087dd425207392b77754 -- Matthias Bolte http://photron.blogspot.com -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [virt-tools-list] ANNOUNCE: virt-manager 0.9.0 and virtinst 0.600.0 released
On Tue, Aug 02, 2011 at 12:05:19PM +0100, Daniel P. Berrange wrote: On Mon, Aug 01, 2011 at 09:13:07AM -0400, Cole Robinson wrote: On 07/30/2011 01:15 PM, Richard W.M. Jones wrote: On Thu, Jul 28, 2011 at 12:31:10PM -0400, Cole Robinson wrote: - Use libguestfs to show guest packagelist and more (Richard W.M. Jones) I had one complaint that the dependency on python-libguestfs pulls in qemu (from someone using virt-manager to remotely manage systems, so they don't want qemu locally and it wouldn't be any use to them for libguestfs). This is hard to solve, unless RPM grows soft dependencies like dpkg, which unfortunately has been rejected many times upstream even though it's a good feature of dpkg. We might _not_ make it a dependency at all. Virt-manager will still work, just without the enhanced inspection features. Just so you know ... Yeah I had a similar report (or maybe the same one?) Figuring that we go to great pains in virt-manager to avoid having a dep on qemu or libvirtd (since virt-manager doesn't need either to be useful), we can't have a hard dep on libguestfs, so I'll be dropping it for the Fedora packages. Make sure that python-libguestfs is included in the 'Virtualization' group of Fedora comps.xml then. So that when someone does 'yum groupinstall Virtualizaation' or selects Virtualization in Anaconda, they get all the bits by default Done it. I added guestfs-browser as an optional component too. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3 RFC] Add function to get the network interface name of a pci device
On 8/1/11 6:00 PM, Stefan Berger stef...@linux.vnet.ibm.com wrote:
On 08/01/2011 07:57 PM, Roopa Prabhu wrote:
From: Roopa Prabhuropra...@cisco.com
This function looks at sysfs net directory to get the network interface
name associated with the pci device
Signed-off-by: Roopa Prabhuropra...@cisco.com
Signed-off-by: Christian Benvenutibe...@cisco.com
Signed-off-by: David Wangdwa...@cisco.com
---
src/util/pci.c | 42 ++
src/util/pci.h |2 ++
2 files changed, 44 insertions(+), 0 deletions(-)
diff --git a/src/util/pci.c b/src/util/pci.c
index a79c164..d2deeef 100644
--- a/src/util/pci.c
+++ b/src/util/pci.c
@@ -1679,3 +1679,45 @@ int pciDeviceIsAssignable(pciDevice *dev,
return 1;
}
+
+/*
+ * This function returns the network device name
+ * of a pci device
+ */
+int
+pciDeviceGetNetName(char *device_link_sysfs_path, char **netname)
+{
+char *pcidev_sysfs_net_path = NULL;
+int ret = -1;
+DIR *dir = NULL;
+struct dirent *entry = NULL;
+
+if (virBuildPath(pcidev_sysfs_net_path, device_link_sysfs_path,
+net) == -1) {
+virReportOOMError();
+return -1;
+}
+
+dir = opendir(pcidev_sysfs_net_path);
+if (dir == NULL)
+goto out;
+
+while ((entry = readdir(dir))) {
+ if (entry-d_name[0] == '.' ||
The above check makes the following one obsolete. If all entries with
first letter '.' can be skipped, then you could just keep the first one,
otherwise I'd also use !strcmp(entry-d_name, .).
Agree, yes I should use strcmp. Did not think of all cases here because the
net dir normally contains one entry. I will also revisit this check for
cases when there might be more net devices associated with a pci device.
+ !strcmp(entry-d_name, ..))
+ continue;
+
+ /* Assume a single directory entry */
+ *netname = strdup(entry-d_name);
+ ret = 0;
+ break;
+}
+
+if (dir)
+closedir(dir);
Check for 'dir != NULL) is not necessary due to goto above.
Yes will fix. Thanks stefan.
+
+out:
+VIR_FREE(pcidev_sysfs_net_path);
+
+return ret;
+}
diff --git a/src/util/pci.h b/src/util/pci.h
index a351baf..fa25472 100644
--- a/src/util/pci.h
+++ b/src/util/pci.h
@@ -74,4 +74,6 @@ int pciDeviceIsAssignable(pciDevice *dev,
int strict_acs_check);
int pciWaitForDeviceCleanup(pciDevice *dev, const char *matcher);
+int pciDeviceGetNetName(char *device_link_sysfs_path, char **netname);
+
#endif /* __VIR_PCI_H__ */
Stefan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3 RFC] Add functions to get sriov PF/VF relationship of a network interface
On 8/1/11 6:10 PM, Stefan Berger stef...@linux.vnet.ibm.com wrote:
On 08/01/2011 07:57 PM, Roopa Prabhu wrote:
From: Roopa Prabhuropra...@cisco.com
This patch adds helper functions to derive the PF/VF relationship of an sriov
network device
Signed-off-by: Roopa Prabhuropra...@cisco.com
Signed-off-by: Christian Benvenutibe...@cisco.com
Signed-off-by: David Wangdwa...@cisco.com
---
src/util/interface.c | 122
++
src/util/interface.h |8 +++
2 files changed, 130 insertions(+), 0 deletions(-)
diff --git a/src/util/interface.c b/src/util/interface.c
index f5eecfb..5ee5703 100644
--- a/src/util/interface.c
+++ b/src/util/interface.c
@@ -30,6 +30,7 @@
#includesys/ioctl.h
#includefcntl.h
#includenetinet/in.h
+#includedirent.h
#ifdef __linux__
# includelinux/if.h
@@ -45,6 +46,8 @@
#include virfile.h
#include memory.h
#include netlink.h
+#include pci.h
+#include logging.h
#define VIR_FROM_THIS VIR_FROM_NET
@@ -1197,3 +1200,122 @@ ifaceRestoreMacAddress(const char *linkdev,
return rc;
}
+
+int ifaceIsVF(const char *ifname)
+{
+char *if_sysfs_device_link = NULL;
+int ret;
+
+if (virAsprintf(if_sysfs_device_link, NET_SYSFS %s/device/physfn,
+ifname) 0) {
+virReportOOMError();
+return -1;
+}
+
+ret = virFileExists(if_sysfs_device_link);
+
+VIR_FREE(if_sysfs_device_link);
+
+return ret;
+}
+
+int ifaceGetVFIndex(const char *pfname, const char *vfname,
+int *vf_index)
+{
+int ret = -1;
+DIR *dir = NULL;
+struct dirent *entry = NULL;
+char *pf_sysfs_device_link = NULL, *vf_sysfs_device_link = NULL;
+char *vf_sysfs_device = NULL;
+char errbuf[64];
+
+if (virAsprintf(pf_sysfs_device_link, NET_SYSFS %s/device,
+pfname) 0) {
+virReportOOMError();
+return -1;
+}
+
+if (virAsprintf(vf_sysfs_device_link, NET_SYSFS %s/device,
+vfname) 0) {
+VIR_FREE(pf_sysfs_device_link);
+virReportOOMError();
+return -1;
+}
+
+vf_sysfs_device = canonicalize_file_name(vf_sysfs_device_link);
+if (vf_sysfs_device == NULL) {
+memset(errbuf, '\0', sizeof(errbuf));
+VIR_ERROR(_(Failed to resolve device link '%s': '%s'),
+ vf_sysfs_device_link,
+ virStrerror(errno, errbuf, sizeof(errbuf)));
+VIR_FREE(pf_sysfs_device_link);
+VIR_FREE(vf_sysfs_device_link);
+return -1;
+}
+
+dir = opendir(pf_sysfs_device_link);
+if (dir == NULL)
+goto out;
+
+while ((entry = readdir(dir))) {
+if (STRPREFIX(entry-d_name, virtfn)) {
+char *device_link, *device_path;
+
+if (virBuildPath(device_link, pf_sysfs_device_link,
+entry-d_name) == -1) {
+virReportOOMError();
+goto out;
+}
+
+device_path = canonicalize_file_name(device_link);
+if (device_path == NULL) {
+memset(errbuf, '\0', sizeof(errbuf));
+VIR_ERROR(_(Failed to resolve device link '%s': '%s'),
+ device_link, virStrerror(errno, errbuf,
+ sizeof(errbuf)));
+VIR_FREE(device_link);
+goto out;
+}
+
+if (!strcmp(vf_sysfs_device, device_path)) {
+*vf_index = atoi(entry-d_name + strlen(virtfn));
This looks odd. Can you explain?
The PF device sysfs directory has links with the name virtfnvf_index
pointing to the VF's pci device.
Example:
# ls -l virtfn*
lrwxrwxrwx. 1 root root 0 Jul 27 11:09 virtfn0 - ../:1e:00.1
lrwxrwxrwx. 1 root root 0 Jul 27 11:09 virtfn1 - ../:1e:00.2
lrwxrwxrwx. 1 root root 0 Jul 27 11:09 virtfn10 - ../:1e:01.3
lrwxrwxrwx. 1 root root 0 Jul 27 11:09 virtfn11 - ../:1e:01.4
lrwxrwxrwx. 1 root root 0 Jul 27 11:09 virtfn12 - ../:1e:01.5
I see if the virt function link is pointing to the pci device of the VF I
am looking for. If it is, then I try to get the vf index from the virtual
function link name (I just need the number in virtfnvf_index.
This is one way to get the vf index. The other way is to get it by reading
the pci config space of the PF, which the node device driver uses.
+ret = 0;
+}
+
+VIR_FREE(device_link);
+VIR_FREE(device_path);
+
+if ( ret == 0 )
+break;
+}
+}
+
+if (dir)
+closedir(dir);
In this case the above test should be after the 'out:' label not to
cause a memory leak with the goto's above.
Thanks for catching it. Exactly similar lines of code exists in the previous
patch. There was a bug in the out label location in previous patch. When I
fixed the previous patch I accidently made the
Re: [libvirt] [PATCH 0/3 RFC] macvtap: Implement getPhysfn to support sending a port profile message for a VF to its PF
On 8/2/11 4:15 AM, Daniel P. Berrange berra...@redhat.com wrote: On Mon, Aug 01, 2011 at 09:16:09PM -0400, Dave Allan wrote: On Mon, Aug 01, 2011 at 04:56:58PM -0700, Roopa Prabhu wrote: This patch tries to fix getPhysFn in macvtap.c to get the physical function(PF) of the direct attach interface, if the interface is a SR-IOV VF. It derives the PF/VF relationship from sysfs. There is some code to do this in node device driver. But it is local to the node device driver . I did not see a clean way to use some of the node device stuff here. The node device driver looks at PCI capabilities to get the same information. We should not have two implementations of this functionality in the code. Either the node device code should be made to use this version or vice versa. We already have a file src/util/pci.c that contains a bunch of helper APIs for dealing with PCI devices. If we need some shared code between macvtap and the node device driver, then we ought to put it in the pci.c file Yes, I looked at it and saw that I had to move quite a few node_device data structures and code in src/util/pci.c. Wasn't sure if there was a reason for it to not already exist in src/util/pci.c. So decided to post this patch as RFC for specific directions. From the patches that I posted, I still need the stuff that tries to derive the net device from the pci device. I will re-evaluate moving the node device stuff to get PF-VF relationship information into src/util/pci.c and repost the patches. Thanks for the comments. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] libvirt-0.9.1 to 0.9.3-r1: managedsave/save won't start/restore at saved state
The 27/07/11, Nicolas Sebrecht wrote: I'm seeing strange behaviour, here. Any guests saved using both managedsave and save commands from virsh won't restore at saved state. A new full boot sequence happen. - Tested against libvirt v0.9.1, v0.9.2, v0.9.3-r1 (Gentoo) - Confirmed on three different hosts Gentoo amd64 systems. - Tested with gentoo and ubuntu guests. - Nothing relevant in /var/log/libvirt/libvirt.log or /var/log/libvirt/qemu/dom.log The state file /var/lib/libvirt/qemu/save/dom.save exists and is deleted when 'virsh start' is called. The new boot sequence is confirmed by : - VNC console checks - previous screen sessions lost - uptime I've open a bug at https://bugs.gentoo.org/show_bug.cgi?id=376333 but had no answer. I'm stuck! As told before, I have one working (in production) system and others failing Gentoo systems (including the testing machine). I've check the working system against the testing machine and looked for differences. I did remove differences one by one (luckily systems are very near from each other) and couldn't have the testing machine to work. I've check Linux kernel configuration (first) and the whole system for installed packages and compilation options. On each difference found I've done: - compilation and reinstallation of ALL the packages; - reboot; - tests. Now, I have almost two exact same systems behaving differently. Some minor differences remain about installed packages (missing on testing): - lshw - pv - colorgcc - autofs - iperf Hardware isn't the same, though. Main differences are: - Intel(R) Xeon(R) CPU E5420 @ 2.50GHz (cpu family : 6) hardware RAID - Intel(R) Core(TM) i3-2100 CPU @ 3.10GHz (cpu family : 6) software RAID Ouch! -- Nicolas Sebrecht -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] doc: Fix confusing statement about required privileges.
src/libvirt.c: Statement of required hypervisor privileges was probably edited after writing and word may was left in a confusing place. --- src/libvirt.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index 941cb47..78f303d 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -3269,7 +3269,7 @@ error: * Dynamically change the target amount of physical memory allocated to a * domain. If domain is NULL, then this change the amount of memory reserved * to Domain0 i.e. the domain where the application runs. - * This function may requires privileged access to the hypervisor. + * This function requires privileged access to the hypervisor. * * @flags may include VIR_DOMAIN_AFFECT_LIVE or VIR_DOMAIN_AFFECT_CONFIG. * Both flags may be set. If VIR_DOMAIN_AFFECT_LIVE is set, the change affects -- 1.7.6 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [virt-tools-list] ANNOUNCE: virt-manager 0.9.0 and virtinst 0.600.0 released
Hi, I posted Japanese translation po file for virt-manager 0.9.0 via transifex.net. I always read python code for translation work. because virt-manager has so many difficult words about virtualization technology. I take some time. Could you merge po file to virt-manager 0.9.x? Best regards, Taira - 元のメッセージ - 差出人: Cole Robinson crobi...@redhat.com To: virt-tools-list virt-tools-l...@redhat.com Cc: Fedora Virtualization List fedora-v...@redhat.com, libvirt-l...@redhat.com 送信済み: 2011年7月29日, 金曜日 午前 1:31:10 件名: [virt-tools-list] ANNOUNCE: virt-manager 0.9.0 and virtinst 0.600.0 released I'm happy to announce two new releases: virt-manager 0.9.0: virt-manager is a desktop application for managing KVM and Xen virtual machines via libvirt. virtinst 0.600.0: virtinst is a collection of command line tools for provisioning libvirt virtual machines, including virt-install and virt-clone. The releases can be downloaded from: http://virt-manager.org/download.html The direct download links are: http://virt-manager.org/download/sources/virt-manager/virt-manager-0.9.0.tar.gz http://virt-manager.org/download/sources/virtinst/virtinst-0.600.0.tar.gz The virt-manager release includes: - Use a hiding toolbar for fullscreen mode - Use libguestfs to show guest packagelist and more (Richard W.M. Jones) - Basic 'New VM' wizard support for LXC guests - Remote serial console access (with latest libvirt) - Remote URL guest installs (with latest libvirt) - Add Hardware: Support filesystem devices - Add Hardware: Support smartcard devices (Marc-André Lureau) - Enable direct interface selection for qemu/kvm (Gerhard Stenzel) - Allow viewing and changing disk serial number The virtinst release includes: - virt-install: Various improvements to enable LXC/container guests: - New --filesystem option for filesystem devices - New --init option for container init path - New --container option (similar to --paravirt or --hvm) - virt-install: Make --location remotely (with latest libvirt) - virt-install: New --smartcard option for smartcard devices (Marc-André Lureau) - virt-install: New --numatune option for building guest numatune XML - virt-install: option to set --disk error_policy= - virt-install: option to set --disk serial= Thanks to everyone who has contributed to this release through testing, bug reporting, submitting patches, and otherwise sending in feedback! Thanks, Cole ___ virt-tools-list mailing list virt-tools-l...@redhat.com https://www.redhat.com/mailman/listinfo/virt-tools-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3 RFC] Add functions to get sriov PF/VF relationship of a network interface
On 08/02/2011 08:46 AM, Roopa Prabhu wrote:
On 8/1/11 6:10 PM, Stefan Bergerstef...@linux.vnet.ibm.com wrote:
On 08/01/2011 07:57 PM, Roopa Prabhu wrote:
From: Roopa Prabhuropra...@cisco.com
This patch adds helper functions to derive the PF/VF relationship of an sriov
network device
Signed-off-by: Roopa Prabhuropra...@cisco.com
Signed-off-by: Christian Benvenutibe...@cisco.com
Signed-off-by: David Wangdwa...@cisco.com
---
src/util/interface.c | 122
++
src/util/interface.h |8 +++
2 files changed, 130 insertions(+), 0 deletions(-)
diff --git a/src/util/interface.c b/src/util/interface.c
index f5eecfb..5ee5703 100644
--- a/src/util/interface.c
+++ b/src/util/interface.c
@@ -30,6 +30,7 @@
#includesys/ioctl.h
#includefcntl.h
#includenetinet/in.h
+#includedirent.h
#ifdef __linux__
# includelinux/if.h
@@ -45,6 +46,8 @@
#include virfile.h
#include memory.h
#include netlink.h
+#include pci.h
+#include logging.h
#define VIR_FROM_THIS VIR_FROM_NET
@@ -1197,3 +1200,122 @@ ifaceRestoreMacAddress(const char *linkdev,
return rc;
}
+
+int ifaceIsVF(const char *ifname)
+{
+char *if_sysfs_device_link = NULL;
+int ret;
+
+if (virAsprintf(if_sysfs_device_link, NET_SYSFS %s/device/physfn,
+ifname) 0) {
+virReportOOMError();
+return -1;
+}
+
+ret = virFileExists(if_sysfs_device_link);
+
+VIR_FREE(if_sysfs_device_link);
+
+return ret;
+}
+
+int ifaceGetVFIndex(const char *pfname, const char *vfname,
+int *vf_index)
+{
+int ret = -1;
+DIR *dir = NULL;
+struct dirent *entry = NULL;
+char *pf_sysfs_device_link = NULL, *vf_sysfs_device_link = NULL;
+char *vf_sysfs_device = NULL;
+char errbuf[64];
+
+if (virAsprintf(pf_sysfs_device_link, NET_SYSFS %s/device,
+pfname) 0) {
+virReportOOMError();
+return -1;
+}
+
+if (virAsprintf(vf_sysfs_device_link, NET_SYSFS %s/device,
+vfname) 0) {
+VIR_FREE(pf_sysfs_device_link);
+virReportOOMError();
+return -1;
+}
+
+vf_sysfs_device = canonicalize_file_name(vf_sysfs_device_link);
+if (vf_sysfs_device == NULL) {
+memset(errbuf, '\0', sizeof(errbuf));
+VIR_ERROR(_(Failed to resolve device link '%s': '%s'),
+ vf_sysfs_device_link,
+ virStrerror(errno, errbuf, sizeof(errbuf)));
+VIR_FREE(pf_sysfs_device_link);
+VIR_FREE(vf_sysfs_device_link);
+return -1;
+}
+
+dir = opendir(pf_sysfs_device_link);
+if (dir == NULL)
+goto out;
+
+while ((entry = readdir(dir))) {
+if (STRPREFIX(entry-d_name, virtfn)) {
+char *device_link, *device_path;
+
+if (virBuildPath(device_link, pf_sysfs_device_link,
+entry-d_name) == -1) {
+virReportOOMError();
+goto out;
+}
+
+device_path = canonicalize_file_name(device_link);
+if (device_path == NULL) {
+memset(errbuf, '\0', sizeof(errbuf));
+VIR_ERROR(_(Failed to resolve device link '%s': '%s'),
+ device_link, virStrerror(errno, errbuf,
+ sizeof(errbuf)));
+VIR_FREE(device_link);
+goto out;
+}
+
+if (!strcmp(vf_sysfs_device, device_path)) {
+*vf_index = atoi(entry-d_name + strlen(virtfn));
This looks odd. Can you explain?
The PF device sysfs directory has links with the name virtfnvf_index
pointing to the VF's pci device.
Example:
# ls -l virtfn*
lrwxrwxrwx. 1 root root 0 Jul 27 11:09 virtfn0 - ../:1e:00.1
lrwxrwxrwx. 1 root root 0 Jul 27 11:09 virtfn1 - ../:1e:00.2
lrwxrwxrwx. 1 root root 0 Jul 27 11:09 virtfn10 - ../:1e:01.3
lrwxrwxrwx. 1 root root 0 Jul 27 11:09 virtfn11 - ../:1e:01.4
lrwxrwxrwx. 1 root root 0 Jul 27 11:09 virtfn12 - ../:1e:01.5
I see if the virt function link is pointing to the pci device of the VF I
am looking for. If it is, then I try to get the vf index from the virtual
function link name (I just need the number in virtfnvf_index.
This is one way to get the vf index. The other way is to get it by reading
the pci config space of the PF, which the node device driver uses.
In that case I'd suggest to use 'if (sscanf(entry-d_name, virtfn%u,
vf_index) != 1) { error }'
Stefan
+ret = 0;
+}
+
+VIR_FREE(device_link);
+VIR_FREE(device_path);
+
+if ( ret == 0 )
+break;
+}
+}
+
+if (dir)
+closedir(dir);
In this case the above test should be after the 'out:' label not to
cause a memory leak with the goto's above.
Thanks for catching it. Exactly similar lines of code exists in the previous
patch. There was a bug in the out label location in previous patch. When I
fixed
Re: [libvirt] [PATCH] rpc: avoid libvirtd crash on unexpected client close
On 08/02/2011 04:41 AM, Daniel P. Berrange wrote:
On Mon, Aug 01, 2011 at 01:53:19PM -0600, Eric Blake wrote:
Steps to reproduce this problem (vm1 is not running):
for i in `seq 50`; do virsh managedsave vm1 done; killall virsh
Pre-patch, virNetServerClientClose could end up setting client-sock
to NULL prior to other cleanup functions trying to use client-sock.
This fixes things by checking for NULL in more places, and by deferring
the cleanup until after all queued messages have been served.
@@ -570,10 +577,7 @@ void virNetServerClientClose(virNetServerClientPtr client)
virNetTLSSessionFree(client-tls);
client-tls = NULL;
}
-if (client-sock) {
-virNetSocketFree(client-sock);
-client-sock = NULL;
-}
+client-wantClose = true;
while (client-rx) {
virNetMessagePtr msg
@@ -586,6 +590,11 @@ void virNetServerClientClose(virNetServerClientPtr client)
virNetMessageFree(msg);
}
+if (client-sock) {
+virNetSocketFree(client-sock);
+client-sock = NULL;
+}
+
virNetServerClientUnlock(client);
}
I'm curious why you have these last 2 hunks of the patc ? The entire
of the virNetServerClientClose() is executed under the mutex, so AFAICT
moving these 4 lines to later in the function can have no effect. The
assignment to wantClose ought to not be needed at this point either,
since this flag is used by the server to decide to invoke the
virNetServerClientClose method.
I did it because virNetMessageFree calls a callback, and I didn't want
to audit whether that callback might temporarily drop the mutex or
reference client-sock.
That all said, these 2 hunks are at worst harmless, so ACK to the
patch.
Fair enough, so I pushed as-is.
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] doc: Fix confusing statement about required privileges.
On 08/02/2011 07:03 AM, Peter Krempa wrote: src/libvirt.c: Statement of required hypervisor privileges was probably edited after writing and word may was left in a confusing place. --- src/libvirt.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index 941cb47..78f303d 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -3269,7 +3269,7 @@ error: * Dynamically change the target amount of physical memory allocated to a * domain. If domain is NULL, then this change the amount of memory reserved * to Domain0 i.e. the domain where the application runs. - * This function may requires privileged access to the hypervisor. + * This function requires privileged access to the hypervisor. Actually, I'd rather go with 'may require' than 'requires'. And there are multiple culprits, not just this one line. How about this patch instead? diff --git c/src/libvirt.c w/src/libvirt.c index 941cb47..c154c7d 100644 --- c/src/libvirt.c +++ w/src/libvirt.c @@ -1789,7 +1789,7 @@ virDomainGetConnect (virDomainPtr dom) * * Launch a new guest domain, based on an XML description similar * to the one returned by virDomainGetXMLDesc() - * This function may requires privileged access to the hypervisor. + * This function may require privileged access to the hypervisor. * The domain is not persistent, so its definition will disappear when it * is destroyed, or if the host is restarted (see virDomainDefineXML() to * define persistent domains). @@ -2198,7 +2198,7 @@ virDomainRef(virDomainPtr domain) * to CPU resources and I/O but the memory used by the domain at the * hypervisor level will stay allocated. Use virDomainResume() to reactivate * the domain. - * This function may requires privileged access. + * This function may require privileged access. * * Returns 0 in case of success and -1 in case of failure. */ @@ -2244,7 +2244,7 @@ error: * * Resume a suspended domain, the process is restarted from the state where * it was frozen by calling virSuspendDomain(). - * This function may requires privileged access + * This function may require privileged access * * Returns 0 in case of success and -1 in case of failure. */ @@ -3158,7 +3158,7 @@ error: * Dynamically change the maximum amount of physical memory allocated to a * domain. If domain is NULL, then this change the amount of memory reserved * to Domain0 i.e. the domain where the application runs. - * This function requires privileged access to the hypervisor. + * This function may require privileged access to the hypervisor. * * This command is hypervisor-specific for whether active, persistent, * or both configurations are changed; for more control, use @@ -3213,7 +3213,7 @@ error: * Dynamically change the target amount of physical memory allocated to a * domain. If domain is NULL, then this change the amount of memory reserved * to Domain0 i.e. the domain where the application runs. - * This function may requires privileged access to the hypervisor. + * This function may require privileged access to the hypervisor. * * This command only changes the runtime configuration of the domain, * so can only be called on an active domain. @@ -3269,7 +3269,7 @@ error: * Dynamically change the target amount of physical memory allocated to a * domain. If domain is NULL, then this change the amount of memory reserved * to Domain0 i.e. the domain where the application runs. - * This function may requires privileged access to the hypervisor. + * This function may require privileged access to the hypervisor. * * @flags may include VIR_DOMAIN_AFFECT_LIVE or VIR_DOMAIN_AFFECT_CONFIG. * Both flags may be set. If VIR_DOMAIN_AFFECT_LIVE is set, the change affects @@ -3338,7 +3338,7 @@ error: * @flags: bitwise-OR of virDomainModificationImpact * * Change all or a subset of the memory tunables. - * This function requires privileged access to the hypervisor. + * This function may require privileged access to the hypervisor. * * Returns -1 in case of error, 0 in case of success. */ @@ -3412,7 +3412,7 @@ error: * goto error; * } * - * This function requires privileged access to the hypervisor. This function + * This function may require privileged access to the hypervisor. This function * expects the caller to allocate the @params. * * Returns -1 in case of error, 0 in case of success. @@ -3464,7 +3464,7 @@ error: * @flags: an OR'ed set of virDomainModificationImpact * * Change all or a subset of the blkio tunables. - * This function requires privileged access to the hypervisor. + * This function may require privileged access to the hypervisor. * * Returns -1 in case of error, 0 in case of success. */ @@ -3522,7 +3522,7 @@ error: * equal to the number of parameters suggested by @nparams. * See virDomainGetMemoryParameters for an equivalent usage example. * - * This function requires privileged access to
Re: [libvirt] [PATCH] doc: Fix confusing statement about required privileges.
On Tue, Aug 02, 2011 at 03:03:38PM +0200, Peter Krempa wrote: src/libvirt.c: Statement of required hypervisor privileges was probably edited after writing and word may was left in a confusing place. --- src/libvirt.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index 941cb47..78f303d 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -3269,7 +3269,7 @@ error: * Dynamically change the target amount of physical memory allocated to a * domain. If domain is NULL, then this change the amount of memory reserved * to Domain0 i.e. the domain where the application runs. - * This function may requires privileged access to the hypervisor. + * This function requires privileged access to the hypervisor. * * @flags may include VIR_DOMAIN_AFFECT_LIVE or VIR_DOMAIN_AFFECT_CONFIG. * Both flags may be set. If VIR_DOMAIN_AFFECT_LIVE is set, the change affects Hum, no, well there is a superfluous 's' but it's right * This function may require privileged access to the hypervisor. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] doc: Fix confusing statement about required privileges.
On 08/02/2011 03:57 PM, Eric Blake wrote: Actually, I'd rather go with 'may require' than 'requires'. And there are multiple culprits, not just this one line. How about this patch instead? Oh, I compared it to a neighbour comment and asumed it was a typo, so I didn't look further. My bad. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Unlink temporary file on failure
On 08/02/2011 09:36 AM, Michal Privoznik wrote: Although virFDStreamOpenFile will unlink it once opened, when we hit error path, we must unlink it by hand. --- src/qemu/qemu_driver.c |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 09b2791..d860cd2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2930,6 +2930,7 @@ qemuDomainScreenshot(virDomainPtr dom, ret = strdup(image/x-portable-pixmap); endjob: +unlink(tmp); VIR_FORCE_CLOSE(tmp_fd); Swap these two lines. Mingw doesn't like unlinking open files. Also, you MUST check that tmp is not NULL (which is possible on some of the error paths); unlink(NULL) is a no-no. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2] qemu: Unlink temporary file on failure
Although virFDStreamOpenFile will unlink it once opened, when we hit
error path, we must unlink it by hand.
---
src/qemu/qemu_driver.c |5 -
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 09b2791..c78cdb7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2931,7 +2931,10 @@ qemuDomainScreenshot(virDomainPtr dom,
endjob:
VIR_FORCE_CLOSE(tmp_fd);
-VIR_FREE(tmp);
+ if (tmp) {
+unlink(tmp);
+VIR_FREE(tmp);
+ }
if (qemuDomainObjEndJob(driver, vm) == 0)
vm = NULL;
--
1.7.3.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: Unlink temporary file on failure
On 08/02/2011 09:50 AM, Michal Privoznik wrote:
Although virFDStreamOpenFile will unlink it once opened, when we hit
error path, we must unlink it by hand.
---
src/qemu/qemu_driver.c |5 -
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 09b2791..c78cdb7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2931,7 +2931,10 @@ qemuDomainScreenshot(virDomainPtr dom,
endjob:
VIR_FORCE_CLOSE(tmp_fd);
-VIR_FREE(tmp);
+ if (tmp) {
+unlink(tmp);
+VIR_FREE(tmp);
+ }
ACK.
The VIR_FREE(tmp) could be unconditional, but it doesn't hurt to leave
it conditional - at any rate, it won't trip the syntax-check for
useless-if-before-free because we are doing more than just freeing tmp
in the conditional.
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/3] rpc: avoid uninitialized memory use
Spotted by Coverity. Gnutls documents that buffer must be NULL if gnutls_x509_crt_get_key_purpose_oid is to be used to determine the correct size needed for allocating a buffer. * src/rpc/virnettlscontext.c (virNetTLSContextCheckCertKeyPurpose): Initialize buffer. --- src/rpc/virnettlscontext.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index 2a58ede..be08207 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -264,7 +264,7 @@ static int virNetTLSContextCheckCertKeyPurpose(gnutls_x509_crt_t cert, int i; unsigned int purposeCritical; unsigned int critical; -char *buffer; +char *buffer = NULL; size_t size; bool allowClient = false, allowServer = false; -- 1.7.4.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/3] rpc: avoid double close on error
Spotted by coverity. If pipe2 fails, then we attempt to close
uninitialized fds, which may result in a double-close.
* src/rpc/virnetserver.c (virNetServerSignalSetup): Initialize fds.
---
src/rpc/virnetserver.c |2 +-
1 file changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 2dae2ff..4deeca1 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -471,7 +471,7 @@ cleanup:
static int virNetServerSignalSetup(virNetServerPtr srv)
{
-int fds[2];
+int fds[2] = { -1, -1 };
if (srv-sigwrite != -1)
return 0;
--
1.7.4.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/n] Coverity run
I'm doing another coverity cleanup run; Coverity reported as many as 75 errors this time around, but that definitely includes some false positives. I'll add to this thread as I sort through more reports to see which ones are real, but to get things started: Eric Blake (3): rpc: avoid double close on error rpc: avoid uninitialized memory use python: avoid unlikely sign extension bug python/libvirt-override.c |2 +- src/rpc/virnetserver.c |2 +- src/rpc/virnettlscontext.c |2 +- 4 files changed, 4 insertions(+), 4 deletions(-) -- 1.7.4.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/3] python: avoid unlikely sign extension bug
Detected by Coverity; same analysis as for commit f73198df. * python/libvirt-override.c (libvirt_virDomainGetVcpuPinInfo): Use correct type. --- python/libvirt-override.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/python/libvirt-override.c b/python/libvirt-override.c index aa84438..b5650e2 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -1114,7 +1114,7 @@ libvirt_virDomainGetVcpuPinInfo(PyObject *self ATTRIBUTE_UNUSED, virNodeInfo nodeinfo; virDomainInfo dominfo; unsigned char *cpumaps; -int cpumaplen, vcpu, pcpu; +size_t cpumaplen, vcpu, pcpu; unsigned int flags; int i_retval; -- 1.7.4.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3 RFC] Add functions to get sriov PF/VF relationship of a network interface
On 8/2/11 6:46 AM, Stefan Berger stef...@linux.vnet.ibm.com wrote:
On 08/02/2011 08:46 AM, Roopa Prabhu wrote:
On 8/1/11 6:10 PM, Stefan Bergerstef...@linux.vnet.ibm.com wrote:
On 08/01/2011 07:57 PM, Roopa Prabhu wrote:
From: Roopa Prabhuropra...@cisco.com
This patch adds helper functions to derive the PF/VF relationship of an
sriov
network device
Signed-off-by: Roopa Prabhuropra...@cisco.com
Signed-off-by: Christian Benvenutibe...@cisco.com
Signed-off-by: David Wangdwa...@cisco.com
---
src/util/interface.c | 122
++
src/util/interface.h |8 +++
2 files changed, 130 insertions(+), 0 deletions(-)
diff --git a/src/util/interface.c b/src/util/interface.c
index f5eecfb..5ee5703 100644
--- a/src/util/interface.c
+++ b/src/util/interface.c
@@ -30,6 +30,7 @@
#includesys/ioctl.h
#includefcntl.h
#includenetinet/in.h
+#includedirent.h
#ifdef __linux__
# includelinux/if.h
@@ -45,6 +46,8 @@
#include virfile.h
#include memory.h
#include netlink.h
+#include pci.h
+#include logging.h
#define VIR_FROM_THIS VIR_FROM_NET
@@ -1197,3 +1200,122 @@ ifaceRestoreMacAddress(const char *linkdev,
return rc;
}
+
+int ifaceIsVF(const char *ifname)
+{
+char *if_sysfs_device_link = NULL;
+int ret;
+
+if (virAsprintf(if_sysfs_device_link, NET_SYSFS %s/device/physfn,
+ifname) 0) {
+virReportOOMError();
+return -1;
+}
+
+ret = virFileExists(if_sysfs_device_link);
+
+VIR_FREE(if_sysfs_device_link);
+
+return ret;
+}
+
+int ifaceGetVFIndex(const char *pfname, const char *vfname,
+int *vf_index)
+{
+int ret = -1;
+DIR *dir = NULL;
+struct dirent *entry = NULL;
+char *pf_sysfs_device_link = NULL, *vf_sysfs_device_link = NULL;
+char *vf_sysfs_device = NULL;
+char errbuf[64];
+
+if (virAsprintf(pf_sysfs_device_link, NET_SYSFS %s/device,
+pfname) 0) {
+virReportOOMError();
+return -1;
+}
+
+if (virAsprintf(vf_sysfs_device_link, NET_SYSFS %s/device,
+vfname) 0) {
+VIR_FREE(pf_sysfs_device_link);
+virReportOOMError();
+return -1;
+}
+
+vf_sysfs_device = canonicalize_file_name(vf_sysfs_device_link);
+if (vf_sysfs_device == NULL) {
+memset(errbuf, '\0', sizeof(errbuf));
+VIR_ERROR(_(Failed to resolve device link '%s': '%s'),
+ vf_sysfs_device_link,
+ virStrerror(errno, errbuf, sizeof(errbuf)));
+VIR_FREE(pf_sysfs_device_link);
+VIR_FREE(vf_sysfs_device_link);
+return -1;
+}
+
+dir = opendir(pf_sysfs_device_link);
+if (dir == NULL)
+goto out;
+
+while ((entry = readdir(dir))) {
+if (STRPREFIX(entry-d_name, virtfn)) {
+char *device_link, *device_path;
+
+if (virBuildPath(device_link, pf_sysfs_device_link,
+entry-d_name) == -1) {
+virReportOOMError();
+goto out;
+}
+
+device_path = canonicalize_file_name(device_link);
+if (device_path == NULL) {
+memset(errbuf, '\0', sizeof(errbuf));
+VIR_ERROR(_(Failed to resolve device link '%s': '%s'),
+ device_link, virStrerror(errno, errbuf,
+ sizeof(errbuf)));
+VIR_FREE(device_link);
+goto out;
+}
+
+if (!strcmp(vf_sysfs_device, device_path)) {
+*vf_index = atoi(entry-d_name + strlen(virtfn));
This looks odd. Can you explain?
The PF device sysfs directory has links with the name virtfnvf_index
pointing to the VF's pci device.
Example:
# ls -l virtfn*
lrwxrwxrwx. 1 root root 0 Jul 27 11:09 virtfn0 - ../:1e:00.1
lrwxrwxrwx. 1 root root 0 Jul 27 11:09 virtfn1 - ../:1e:00.2
lrwxrwxrwx. 1 root root 0 Jul 27 11:09 virtfn10 - ../:1e:01.3
lrwxrwxrwx. 1 root root 0 Jul 27 11:09 virtfn11 - ../:1e:01.4
lrwxrwxrwx. 1 root root 0 Jul 27 11:09 virtfn12 - ../:1e:01.5
I see if the virt function link is pointing to the pci device of the VF I
am looking for. If it is, then I try to get the vf index from the virtual
function link name (I just need the number in virtfnvf_index.
This is one way to get the vf index. The other way is to get it by reading
the pci config space of the PF, which the node device driver uses.
In that case I'd suggest to use 'if (sscanf(entry-d_name, virtfn%u,
vf_index) != 1) { error }'
Ok sounds good. Thanks.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] doc: Fix confusing statement about required privileges.
On 08/02/2011 08:10 AM, Peter Krempa wrote: On 08/02/2011 03:57 PM, Eric Blake wrote: Actually, I'd rather go with 'may require' than 'requires'. And there are multiple culprits, not just this one line. How about this patch instead? Oh, I compared it to a neighbour comment and asumed it was a typo, so I didn't look further. My bad. At any rate, I've gone ahead and pushed my bigger cleanup patch under the trivial rule, since it is doc only and fixes grammar issues. Those are the sorts of patches nice to include in the 0.9.4 release. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: Unlink temporary file on failure
On 08/02/2011 09:59 AM, Eric Blake wrote:
On 08/02/2011 09:50 AM, Michal Privoznik wrote:
Although virFDStreamOpenFile will unlink it once opened, when we hit
error path, we must unlink it by hand.
---
src/qemu/qemu_driver.c | 5 -
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 09b2791..c78cdb7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2931,7 +2931,10 @@ qemuDomainScreenshot(virDomainPtr dom,
endjob:
VIR_FORCE_CLOSE(tmp_fd);
- VIR_FREE(tmp);
+ if (tmp) {
Wonky spacing.
+ unlink(tmp);
+ VIR_FREE(tmp);
+ }
ACK.
I went ahead and pushed this after fixing the spacing, so that it will
be in 0.9.4.
Meanwhile, I wonder if we have a bigger bug - that is, should
virFDStreamOpenInternal guarantee that the file is deleted when
requested, even on failure paths, so that callers need not do the
unlink()? That is, on the success path, we end up unlink()ing the same
name twice, which is racy if the name is predictable (in the case of
qemuDomainScreenshot, the name is temporary and supposedly safe).
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Libvirt] [PATCH v2] Fix bug #611823 prohibit pools with duplicate storage
On 08/02/2011 07:11 PM, Daniel P. Berrange wrote:
On Mon, Aug 01, 2011 at 02:12:51PM -0600, Eric Blake wrote:
On 07/31/2011 10:58 PM, Lei Li wrote:
Make sure the unique storage pool defined and create from different directory
to avoid inconsistent version of volume pool created.
Wrap your commit messages; typically at 70 columns or so (since 'git
log' adds some indentation, but you want the end result to still fit
in 80 columns for legibility).
Signed-off-by: Lei Lili...@linux.vnet.ibm.com
---
src/conf/storage_conf.c | 36
src/conf/storage_conf.h |4
src/libvirt_private.syms |2 ++
src/storage/storage_driver.c |6 ++
4 files changed, 48 insertions(+), 0 deletions(-)
+virStoragePoolObjPtr
+virStoragePoolObjFindByPath(virStoragePoolObjListPtr pools,
+const char *path) {
+unsigned int i;
+
+for (i = 0 ; i pools-count ; i++) {
+virStoragePoolObjLock(pools-objs[i]);
+if (STREQ(pools-objs[i]-def-target.path, path))
+return pools-objs[i];
+virStoragePoolObjUnlock(pools-objs[i]);
+}
+
+return NULL;
+}
This one is good; in fact, we may even want to expose it as a public
API, parallel to other virStoragePoolLookupBy* functions (but not
until after 0.9.4 is released)
No, this API is flawed because def-target.path is not required to
be unique for all types of storage pool.
Daniel
Yes, in the beginning it seems like target-path is not required to be unique.
But for this bug https://bugzilla.redhat.com/show_bug.cgi?id=611823
you reported, you said that For example, if two directory pools point to the
same directory, and one pool is used to create a volume,
the other pool will remain unaware of the new volume until it is refreshed.
And I have test it when use 'virsh pool-define/create' it will create more
than two pools not two have the same directory. I think maybe you should look
at the description of the bug first.
This API virStoragePoolObjFindByPath() just provide a method to search pool obj
by path and can be use to avoid duplicate target path to fix this bug you
mentioned.
BTW, I found that there are 3 method provide ways to search by
'key','name','path' in storage volume also.
--
Lei
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] screenshot: don't unlink bogus file
The previous qemu patch could end up calling unlink(tmp) before
tmp was the name of a valid file (unlinking a fileXX template
instead), or calling unlink(tmp) twice on success (once here,
and once at the end of the stream). Meanwhile, vbox also suffered
from the same leaked tmp file bug.
* src/qemu/qemu_driver.c (qemuDomainScreenshot): Don't unlink on
success, or on invalid name.
* src/vbox/vbox_tmpl.c (vboxDomainScreenshot): Don't leak temp file.
---
Meanwhile, I wonder if we have a bigger bug - that is, should
virFDStreamOpenInternal guarantee that the file is deleted when
requested, even on failure paths, so that callers need not do the
unlink()? That is, on the success path, we end up unlink()ing the
same name twice, which is racy if the name is predictable (in the case
of qemuDomainScreenshot, the name is temporary and supposedly safe).
I audited all callers, and there were only two that passed delete=true.
Of those two, I found another bug (calling unlink() too soon).
Additionally, remember that the reason we passed delete=true in the
first place was due to libvirt_iohelper doing the unlink in a child
process; where a race condition required that we not do the unlink in
the parent. But now that libvirt_iohelper receives its fd by
inheritance, I think we can revert the delete parameter in the
first place. But that's invasive enough to delay to post-release.
Meanwhile, I still think this is worth applying pre-release.
src/qemu/qemu_driver.c |8
src/vbox/vbox_tmpl.c |5 +
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5e2c903..0297159 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2912,18 +2912,21 @@ qemuDomainScreenshot(virDomainPtr dom,
qemuDomainObjEnterMonitor(driver, vm);
if (qemuMonitorScreendump(priv-mon, tmp) 0) {
qemuDomainObjExitMonitor(driver, vm);
+unlink(tmp);
goto endjob;
}
qemuDomainObjExitMonitor(driver, vm);
if (VIR_CLOSE(tmp_fd) 0) {
virReportSystemError(errno, _(unable to close %s), tmp);
+unlink(tmp);
goto endjob;
}
if (virFDStreamOpenFile(st, tmp, 0, 0, O_RDONLY, true) 0) {
qemuReportError(VIR_ERR_OPERATION_FAILED, %s,
_(unable to open stream));
+unlink(tmp);
goto endjob;
}
@@ -2931,10 +2934,7 @@ qemuDomainScreenshot(virDomainPtr dom,
endjob:
VIR_FORCE_CLOSE(tmp_fd);
-if (tmp) {
-unlink(tmp);
-VIR_FREE(tmp);
-}
+VIR_FREE(tmp);
if (qemuDomainObjEndJob(driver, vm) == 0)
vm = NULL;
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index a7d354e..66a0fe9 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -8713,6 +8713,7 @@ vboxDomainScreenshot(virDomainPtr dom,
if (NS_FAILED(rc) || !width || !height) {
vboxError(VIR_ERR_OPERATION_FAILED, %s,
_(unable to get screen resolution));
+unlink(tmp);
goto endjob;
}
@@ -8723,6 +8724,7 @@ vboxDomainScreenshot(virDomainPtr dom,
if (NS_FAILED(rc)) {
vboxError(VIR_ERR_OPERATION_FAILED, %s,
_(failed to take screenshot));
+unlink(tmp);
goto endjob;
}
@@ -8730,17 +8732,20 @@ vboxDomainScreenshot(virDomainPtr dom,
screenDataSize) 0) {
virReportSystemError(errno, _(unable to write data
to '%s'), tmp);
+unlink(tmp);
goto endjob;
}
if (VIR_CLOSE(tmp_fd) 0) {
virReportSystemError(errno, _(unable to close %s), tmp);
+unlink(tmp);
goto endjob;
}
if (virFDStreamOpenFile(st, tmp, 0, 0, O_RDONLY, true) 0) {
vboxError(VIR_ERR_OPERATION_FAILED, %s,
_(unable to open stream));
+unlink(tmp);
goto endjob;
}
--
1.7.4.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] rpc: avoid uninitialized memory use
On 08/02/2011 12:12 PM, Eric Blake wrote: Spotted by Coverity. Gnutls documents that buffer must be NULL if gnutls_x509_crt_get_key_purpose_oid is to be used to determine the correct size needed for allocating a buffer. * src/rpc/virnettlscontext.c (virNetTLSContextCheckCertKeyPurpose): Initialize buffer. --- src/rpc/virnettlscontext.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index 2a58ede..be08207 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -264,7 +264,7 @@ static int virNetTLSContextCheckCertKeyPurpose(gnutls_x509_crt_t cert, int i; unsigned int purposeCritical; unsigned int critical; -char *buffer; +char *buffer = NULL; size_t size; bool allowClient = false, allowServer = false; ACK. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] rpc: avoid double close on error
On 08/02/2011 12:12 PM, Eric Blake wrote:
Spotted by coverity. If pipe2 fails, then we attempt to close
uninitialized fds, which may result in a double-close.
* src/rpc/virnetserver.c (virNetServerSignalSetup): Initialize fds.
---
src/rpc/virnetserver.c |2 +-
1 file changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 2dae2ff..4deeca1 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -471,7 +471,7 @@ cleanup:
static int virNetServerSignalSetup(virNetServerPtr srv)
{
-int fds[2];
+int fds[2] = { -1, -1 };
if (srv-sigwrite != -1)
return 0;
ACK.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] python: avoid unlikely sign extension bug
On 08/02/2011 12:12 PM, Eric Blake wrote: Detected by Coverity; same analysis as for commit f73198df. * python/libvirt-override.c (libvirt_virDomainGetVcpuPinInfo): Use correct type. --- python/libvirt-override.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/python/libvirt-override.c b/python/libvirt-override.c index aa84438..b5650e2 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -1114,7 +1114,7 @@ libvirt_virDomainGetVcpuPinInfo(PyObject *self ATTRIBUTE_UNUSED, virNodeInfo nodeinfo; virDomainInfo dominfo; unsigned char *cpumaps; -int cpumaplen, vcpu, pcpu; +size_t cpumaplen, vcpu, pcpu; unsigned int flags; int i_retval; ACK. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] fdstream: drop delete argument
Revert 6a1f5f568f8. Now that libvirt_iohelper no longer has a
race where it can open() a file after the parent process has
unlink()d the same file, it makes more sense to make the callers
both create and unlink, rather than the caller create and the
stream unlink.
* src/fdstream.h (virFDStreamOpenFile, virFDStreamCreateFile):
Callers are responsible for deletion.
* src/fdstream.c (virFDStreamOpenFileInternal): Don't leak created
file on failure.
(virFDStreamOpenFile, virFDStreamCreateFile): Drop parameter.
* src/lxc/lxc_driver.c (lxcDomainOpenConsole): Update callers.
* src/qemu/qemu_driver.c (qemuDomainScreenshot)
(qemuDomainOpenConsole): Likewise.
* src/storage/storage_driver.c (storageVolumeDownload)
(storageVolumeUpload): Likewise.
* src/uml/uml_driver.c (umlDomainOpenConsole): Likewise.
* src/vbox/vbox_tmpl.c (vboxDomainScreenshot): Likewise.
* src/xen/xen_driver.c (xenUnifiedDomainOpenConsole): Likewise.
---
This is the bigger patch, which I'm worried may be too invasive
for 0.9.4 this late in the release cycle, but is cleaner overall.
src/fdstream.c | 23 +--
src/fdstream.h |6 ++
src/lxc/lxc_driver.c |2 +-
src/qemu/qemu_driver.c | 11 ++-
src/storage/storage_driver.c |4 ++--
src/uml/uml_driver.c |2 +-
src/vbox/vbox_tmpl.c |8 ++--
src/xen/xen_driver.c |2 +-
8 files changed, 24 insertions(+), 34 deletions(-)
diff --git a/src/fdstream.c b/src/fdstream.c
index 2b7812f..b60162c 100644
--- a/src/fdstream.c
+++ b/src/fdstream.c
@@ -505,8 +505,7 @@ virFDStreamOpenFileInternal(virStreamPtr st,
unsigned long long offset,
unsigned long long length,
int oflags,
-int mode,
-bool delete)
+int mode)
{
int fd = -1;
int fds[2] = { -1, -1 };
@@ -514,8 +513,8 @@ virFDStreamOpenFileInternal(virStreamPtr st,
virCommandPtr cmd = NULL;
int errfd = -1;
-VIR_DEBUG(st=%p path=%s oflags=%x offset=%llu length=%llu mode=%o
delete=%d,
- st, path, oflags, offset, length, mode, delete);
+VIR_DEBUG(st=%p path=%s oflags=%x offset=%llu length=%llu mode=%o,
+ st, path, oflags, offset, length, mode);
if (oflags O_CREAT)
fd = open(path, oflags, mode);
@@ -593,9 +592,6 @@ virFDStreamOpenFileInternal(virStreamPtr st,
if (virFDStreamOpenInternal(st, fd, cmd, errfd, length) 0)
goto error;
-if (delete)
-unlink(path);
-
return 0;
error:
@@ -603,6 +599,8 @@ error:
VIR_FORCE_CLOSE(fds[0]);
VIR_FORCE_CLOSE(fds[1]);
VIR_FORCE_CLOSE(fd);
+if (oflags O_CREAT)
+unlink(path);
return -1;
}
@@ -610,8 +608,7 @@ int virFDStreamOpenFile(virStreamPtr st,
const char *path,
unsigned long long offset,
unsigned long long length,
-int oflags,
-bool delete)
+int oflags)
{
if (oflags O_CREAT) {
streamsReportError(VIR_ERR_INTERNAL_ERROR,
@@ -621,7 +618,7 @@ int virFDStreamOpenFile(virStreamPtr st,
}
return virFDStreamOpenFileInternal(st, path,
offset, length,
- oflags, 0, delete);
+ oflags, 0);
}
int virFDStreamCreateFile(virStreamPtr st,
@@ -629,11 +626,9 @@ int virFDStreamCreateFile(virStreamPtr st,
unsigned long long offset,
unsigned long long length,
int oflags,
- mode_t mode,
- bool delete)
+ mode_t mode)
{
return virFDStreamOpenFileInternal(st, path,
offset, length,
- oflags | O_CREAT,
- mode, delete);
+ oflags | O_CREAT, mode);
}
diff --git a/src/fdstream.h b/src/fdstream.h
index 76f0cd0..f9edb23 100644
--- a/src/fdstream.h
+++ b/src/fdstream.h
@@ -37,14 +37,12 @@ int virFDStreamOpenFile(virStreamPtr st,
const char *path,
unsigned long long offset,
unsigned long long length,
-int oflags,
-bool delete);
+int oflags);
int virFDStreamCreateFile(virStreamPtr st,
const char *path,
unsigned long long offset,
unsigned long long length,
int oflags,
- mode_t mode,
- bool delete);
+ mode_t mode);
Re: [libvirt] [Libvirt] [PATCH v2] Fix bug #611823 prohibit pools with duplicate storage
On 08/02/2011 07:11 PM, Daniel P. Berrange wrote:
On Mon, Aug 01, 2011 at 02:12:51PM -0600, Eric Blake wrote:
On 07/31/2011 10:58 PM, Lei Li wrote:
Make sure the unique storage pool defined and create from different directory
to avoid inconsistent version of volume pool created.
Wrap your commit messages; typically at 70 columns or so (since 'git
log' adds some indentation, but you want the end result to still fit
in 80 columns for legibility).
Signed-off-by: Lei Lili...@linux.vnet.ibm.com
---
src/conf/storage_conf.c | 36
src/conf/storage_conf.h |4
src/libvirt_private.syms |2 ++
src/storage/storage_driver.c |6 ++
4 files changed, 48 insertions(+), 0 deletions(-)
+virStoragePoolObjPtr
+virStoragePoolObjFindByPath(virStoragePoolObjListPtr pools,
+const char *path) {
+unsigned int i;
+
+for (i = 0 ; i pools-count ; i++) {
+virStoragePoolObjLock(pools-objs[i]);
+if (STREQ(pools-objs[i]-def-target.path, path))
+return pools-objs[i];
+virStoragePoolObjUnlock(pools-objs[i]);
+}
+
+return NULL;
+}
This one is good; in fact, we may even want to expose it as a public
API, parallel to other virStoragePoolLookupBy* functions (but not
until after 0.9.4 is released)
No, this API is flawed because def-target.path is not required to
be unique for all types of storage pool.
Daniel
And you said in the bug description that The simplest example is two directory
pools that point to the same directory, but iSCSI and other pool types behave
similarly.
Based on your description, step to reproduce and expected results, I look at
the code about process of storage pool, I agree with your conclusion. But now I
was confused for
your comment target.path is not required to be unique for all types of storage
pool.
--
Lei
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] python: avoid unlikely sign extension bug
On 08/02/2011 11:28 AM, Laine Stump wrote: On 08/02/2011 12:12 PM, Eric Blake wrote: Detected by Coverity; same analysis as for commit f73198df. * python/libvirt-override.c (libvirt_virDomainGetVcpuPinInfo): Use correct type. --- python/libvirt-override.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/python/libvirt-override.c b/python/libvirt-override.c index aa84438..b5650e2 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -1114,7 +1114,7 @@ libvirt_virDomainGetVcpuPinInfo(PyObject *self ATTRIBUTE_UNUSED, virNodeInfo nodeinfo; virDomainInfo dominfo; unsigned char *cpumaps; - int cpumaplen, vcpu, pcpu; + size_t cpumaplen, vcpu, pcpu; unsigned int flags; int i_retval; ACK. 1-3 pushed, and I'll have some more shortly. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/3] build: allow caching the input to STATIC_ANALYSIS
Right now, every re-run of configure re-evaluates whether a
static analysis tool is in use. But if you run configure under
coverity, make a tweak, and then do an incremental rebuild with
gcc but not coverity to test the tweak, then rerun a build under
coverity, then configure does not get rerun, and static analysis
ends up with lots of false positives.
This patch caches the static analysis result, and also makes it
easier to force static analysis even if the existing checks are
insufficient to detect newer versions of the static analyzer tools.
* configure.ac (lv_cv_static_analysis): New cache variable.
---
I _thought_ some of those false positives were looking familiar
from my last coverity run. Turns out I inadvertantly managed
to lose my STATIC_ANALYSIS define.
configure.ac | 11 +--
1 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/configure.ac b/configure.ac
index 7c4fb32..10487e5 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2404,9 +2404,16 @@ cp -f COPYING.LIB COPYING
# Detect when running under the clang static analyzer's scan-build driver
# or Coverity-prevent's cov-build. Define STATIC_ANALYSIS accordingly.
+AC_CACHE_CHECK([whether this build is done by a static analysis tool],
+ [lv_cv_static_analysis], [
+lv_cv_static_analysis=no
+if test -n ${CCC_ANALYZER_ANALYSIS+set} || \
+ test -n $COVERITY_BUILD_COMMAND$COVERITY_LD_PRELOAD; then
+ lv_cv_static_analysis=yes
+fi
+ ])
t=0
-test -n ${CCC_ANALYZER_ANALYSIS+set} t=1
-test -n $COVERITY_BUILD_COMMAND$COVERITY_LD_PRELOAD t=1
+test x$lv_cv_static_analysis = xyes t=1
AC_DEFINE_UNQUOTED([STATIC_ANALYSIS], [$t],
[Define to 1 when performing static analysis.])
--
1.7.4.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] screenshot: don't unlink bogus file
On 08/02/2011 01:11 PM, Eric Blake wrote:
The previous qemu patch could end up calling unlink(tmp) before
tmp was the name of a valid file (unlinking a fileXX template
instead), or calling unlink(tmp) twice on success (once here,
and once at the end of the stream). Meanwhile, vbox also suffered
from the same leaked tmp file bug.
ACK.
* src/qemu/qemu_driver.c (qemuDomainScreenshot): Don't unlink on
success, or on invalid name.
* src/vbox/vbox_tmpl.c (vboxDomainScreenshot): Don't leak temp file.
---
Meanwhile, I wonder if we have a bigger bug - that is, should
virFDStreamOpenInternal guarantee that the file is deleted when
requested, even on failure paths, so that callers need not do the
unlink()? That is, on the success path, we end up unlink()ing the
same name twice, which is racy if the name is predictable (in the case
of qemuDomainScreenshot, the name is temporary and supposedly safe).
I audited all callers, and there were only two that passed delete=true.
Of those two, I found another bug (calling unlink() too soon).
Additionally, remember that the reason we passed delete=true in the
first place was due to libvirt_iohelper doing the unlink in a child
process; where a race condition required that we not do the unlink in
the parent. But now that libvirt_iohelper receives its fd by
inheritance, I think we can revert the delete parameter in the
first place. But that's invasive enough to delay to post-release.
Meanwhile, I still think this is worth applying pre-release.
src/qemu/qemu_driver.c |8
src/vbox/vbox_tmpl.c |5 +
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5e2c903..0297159 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2912,18 +2912,21 @@ qemuDomainScreenshot(virDomainPtr dom,
qemuDomainObjEnterMonitor(driver, vm);
if (qemuMonitorScreendump(priv-mon, tmp) 0) {
qemuDomainObjExitMonitor(driver, vm);
+unlink(tmp);
goto endjob;
}
qemuDomainObjExitMonitor(driver, vm);
if (VIR_CLOSE(tmp_fd) 0) {
virReportSystemError(errno, _(unable to close %s), tmp);
+unlink(tmp);
goto endjob;
}
if (virFDStreamOpenFile(st, tmp, 0, 0, O_RDONLY, true) 0) {
qemuReportError(VIR_ERR_OPERATION_FAILED, %s,
_(unable to open stream));
+unlink(tmp);
goto endjob;
}
@@ -2931,10 +2934,7 @@ qemuDomainScreenshot(virDomainPtr dom,
endjob:
VIR_FORCE_CLOSE(tmp_fd);
-if (tmp) {
-unlink(tmp);
-VIR_FREE(tmp);
-}
+VIR_FREE(tmp);
if (qemuDomainObjEndJob(driver, vm) == 0)
vm = NULL;
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index a7d354e..66a0fe9 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -8713,6 +8713,7 @@ vboxDomainScreenshot(virDomainPtr dom,
if (NS_FAILED(rc) || !width || !height) {
vboxError(VIR_ERR_OPERATION_FAILED, %s,
_(unable to get screen resolution));
+unlink(tmp);
goto endjob;
}
@@ -8723,6 +8724,7 @@ vboxDomainScreenshot(virDomainPtr dom,
if (NS_FAILED(rc)) {
vboxError(VIR_ERR_OPERATION_FAILED, %s,
_(failed to take screenshot));
+unlink(tmp);
goto endjob;
}
@@ -8730,17 +8732,20 @@ vboxDomainScreenshot(virDomainPtr dom,
screenDataSize) 0) {
virReportSystemError(errno, _(unable to write data
to '%s'), tmp);
+unlink(tmp);
goto endjob;
}
if (VIR_CLOSE(tmp_fd) 0) {
virReportSystemError(errno, _(unable to close %s), tmp);
+unlink(tmp);
goto endjob;
}
if (virFDStreamOpenFile(st, tmp, 0, 0, O_RDONLY, true) 0) {
vboxError(VIR_ERR_OPERATION_FAILED, %s,
_(unable to open stream));
+unlink(tmp);
goto endjob;
}
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 5/3] rpc: correctly process sasl whitelist globs
Detected by Coverity. We want to compare the result of fnmatch 'rv',
not our pre-set return value 'ret'.
* src/rpc/virnetsaslcontext.c (virNetSASLContextCheckIdentity):
Check correct variable.
---
src/rpc/virnetsaslcontext.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/rpc/virnetsaslcontext.c b/src/rpc/virnetsaslcontext.c
index a0752dd..ef36e2c 100644
--- a/src/rpc/virnetsaslcontext.c
+++ b/src/rpc/virnetsaslcontext.c
@@ -130,11 +130,11 @@ int virNetSASLContextCheckIdentity(virNetSASLContextPtr
ctxt,
int rv = fnmatch (*wildcards, identity, 0);
if (rv == 0) {
ret = 1;
goto cleanup; /* Succesful match */
}
-if (ret != FNM_NOMATCH) {
+if (rv != FNM_NOMATCH) {
virNetError(VIR_ERR_INTERNAL_ERROR,
_(Malformed TLS whitelist regular expression '%s'),
*wildcards);
goto cleanup;
}
--
1.7.4.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] fdstream: drop delete argument
On 08/02/2011 01:31 PM, Eric Blake wrote:
Revert 6a1f5f568f8. Now that libvirt_iohelper no longer has a
race where it can open() a file after the parent process has
unlink()d the same file, it makes more sense to make the callers
both create and unlink, rather than the caller create and the
stream unlink.
I wasn't paying attention to the messages/patches related to the race
condition you reference, but this (caller creates and unlinks)
definitely seems cleaner than the other way. Beyond that, the patch
seems to be correct. ACK.
* src/fdstream.h (virFDStreamOpenFile, virFDStreamCreateFile):
Callers are responsible for deletion.
* src/fdstream.c (virFDStreamOpenFileInternal): Don't leak created
file on failure.
(virFDStreamOpenFile, virFDStreamCreateFile): Drop parameter.
* src/lxc/lxc_driver.c (lxcDomainOpenConsole): Update callers.
* src/qemu/qemu_driver.c (qemuDomainScreenshot)
(qemuDomainOpenConsole): Likewise.
* src/storage/storage_driver.c (storageVolumeDownload)
(storageVolumeUpload): Likewise.
* src/uml/uml_driver.c (umlDomainOpenConsole): Likewise.
* src/vbox/vbox_tmpl.c (vboxDomainScreenshot): Likewise.
* src/xen/xen_driver.c (xenUnifiedDomainOpenConsole): Likewise.
---
This is the bigger patch, which I'm worried may be too invasive
for 0.9.4 this late in the release cycle, but is cleaner overall.
src/fdstream.c | 23 +--
src/fdstream.h |6 ++
src/lxc/lxc_driver.c |2 +-
src/qemu/qemu_driver.c | 11 ++-
src/storage/storage_driver.c |4 ++--
src/uml/uml_driver.c |2 +-
src/vbox/vbox_tmpl.c |8 ++--
src/xen/xen_driver.c |2 +-
8 files changed, 24 insertions(+), 34 deletions(-)
diff --git a/src/fdstream.c b/src/fdstream.c
index 2b7812f..b60162c 100644
--- a/src/fdstream.c
+++ b/src/fdstream.c
@@ -505,8 +505,7 @@ virFDStreamOpenFileInternal(virStreamPtr st,
unsigned long long offset,
unsigned long long length,
int oflags,
-int mode,
-bool delete)
+int mode)
{
int fd = -1;
int fds[2] = { -1, -1 };
@@ -514,8 +513,8 @@ virFDStreamOpenFileInternal(virStreamPtr st,
virCommandPtr cmd = NULL;
int errfd = -1;
-VIR_DEBUG(st=%p path=%s oflags=%x offset=%llu length=%llu mode=%o
delete=%d,
- st, path, oflags, offset, length, mode, delete);
+VIR_DEBUG(st=%p path=%s oflags=%x offset=%llu length=%llu mode=%o,
+ st, path, oflags, offset, length, mode);
if (oflags O_CREAT)
fd = open(path, oflags, mode);
@@ -593,9 +592,6 @@ virFDStreamOpenFileInternal(virStreamPtr st,
if (virFDStreamOpenInternal(st, fd, cmd, errfd, length) 0)
goto error;
-if (delete)
-unlink(path);
-
return 0;
error:
@@ -603,6 +599,8 @@ error:
VIR_FORCE_CLOSE(fds[0]);
VIR_FORCE_CLOSE(fds[1]);
VIR_FORCE_CLOSE(fd);
+if (oflags O_CREAT)
+unlink(path);
return -1;
}
@@ -610,8 +608,7 @@ int virFDStreamOpenFile(virStreamPtr st,
const char *path,
unsigned long long offset,
unsigned long long length,
-int oflags,
-bool delete)
+int oflags)
{
if (oflags O_CREAT) {
streamsReportError(VIR_ERR_INTERNAL_ERROR,
@@ -621,7 +618,7 @@ int virFDStreamOpenFile(virStreamPtr st,
}
return virFDStreamOpenFileInternal(st, path,
offset, length,
- oflags, 0, delete);
+ oflags, 0);
}
int virFDStreamCreateFile(virStreamPtr st,
@@ -629,11 +626,9 @@ int virFDStreamCreateFile(virStreamPtr st,
unsigned long long offset,
unsigned long long length,
int oflags,
- mode_t mode,
- bool delete)
+ mode_t mode)
{
return virFDStreamOpenFileInternal(st, path,
offset, length,
- oflags | O_CREAT,
- mode, delete);
+ oflags | O_CREAT, mode);
}
diff --git a/src/fdstream.h b/src/fdstream.h
index 76f0cd0..f9edb23 100644
--- a/src/fdstream.h
+++ b/src/fdstream.h
@@ -37,14 +37,12 @@ int virFDStreamOpenFile(virStreamPtr st,
const char *path,
unsigned long long offset,
unsigned long long length,
-int oflags,
-bool delete);
+int oflags);
int
Re: [libvirt] [PATCH 5/3] rpc: correctly process sasl whitelist globs
On 08/02/2011 03:06 PM, Eric Blake wrote:
Detected by Coverity. We want to compare the result of fnmatch 'rv',
not our pre-set return value 'ret'.
* src/rpc/virnetsaslcontext.c (virNetSASLContextCheckIdentity):
Check correct variable.
---
src/rpc/virnetsaslcontext.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/rpc/virnetsaslcontext.c b/src/rpc/virnetsaslcontext.c
index a0752dd..ef36e2c 100644
--- a/src/rpc/virnetsaslcontext.c
+++ b/src/rpc/virnetsaslcontext.c
@@ -130,11 +130,11 @@ int virNetSASLContextCheckIdentity(virNetSASLContextPtr
ctxt,
int rv = fnmatch (*wildcards, identity, 0);
if (rv == 0) {
ret = 1;
goto cleanup; /* Succesful match */
}
-if (ret != FNM_NOMATCH) {
+if (rv != FNM_NOMATCH) {
virNetError(VIR_ERR_INTERNAL_ERROR,
_(Malformed TLS whitelist regular expression '%s'),
*wildcards);
goto cleanup;
}
ACK.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/3] rpc: correctly process sasl whitelist globs
On Tue, Aug 02, 2011 at 13:06:44 -0600, Eric Blake wrote: Detected by Coverity. We want to compare the result of fnmatch 'rv', not our pre-set return value 'ret'. * src/rpc/virnetsaslcontext.c (virNetSASLContextCheckIdentity): Check correct variable. --- src/rpc/virnetsaslcontext.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) ACK Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 6/3] qemu: avoid null deref on block pull error
Coverity detected that 5 of 6 callers of virJSONValueArrayGet checked
for a NULL return; and that by not checking we risk a null deref
during an error. The error is unlikely since the prior call to
virJSONValueArraySize would probably have already caught any botched
JSON array parse, but better safe than sorry.
* src/qemu/qemu_monitor_json.c (qemuMonitorJSONGetBlockJobInfo):
Check for NULL.
(qemuMonitorJSONExtractPtyPaths): Fix typo.
---
src/qemu/qemu_monitor_json.c |9 +++--
1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index b7a6a12..2a9a078 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -1018,7 +1018,7 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr reply,
int thread;
if (!entry) {
qemuReportError(VIR_ERR_INTERNAL_ERROR, %s,
-_(character device information was missing aray
element));
+_(character device information was missing array
element));
goto cleanup;
}
@@ -2266,7 +2266,7 @@ static int qemuMonitorJSONExtractPtyPaths(virJSONValuePtr
reply,
const char *id;
if (!entry) {
qemuReportError(VIR_ERR_INTERNAL_ERROR, %s,
-_(character device information was missing aray
element));
+_(character device information was missing array
element));
goto cleanup;
}
@@ -2855,6 +2855,11 @@ static int
qemuMonitorJSONGetBlockJobInfo(virJSONValuePtr reply,
for (i = 0; i nr_results; i++) {
virJSONValuePtr entry = virJSONValueArrayGet(data, i);
+if (!entry) {
+qemuReportError(VIR_ERR_INTERNAL_ERROR, %s,
+_(missing array element));
+return -1;
+}
if (qemuMonitorJSONGetBlockJobInfoOne(entry, device, info) == 0)
return 1;
}
--
1.7.4.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/3] build: allow caching the input to STATIC_ANALYSIS
On 08/02/2011 02:31 PM, Eric Blake wrote:
Right now, every re-run of configure re-evaluates whether a
static analysis tool is in use. But if you run configure under
coverity, make a tweak, and then do an incremental rebuild with
gcc but not coverity to test the tweak, then rerun a build under
coverity, then configure does not get rerun, and static analysis
ends up with lots of false positives.
This patch caches the static analysis result, and also makes it
easier to force static analysis even if the existing checks are
insufficient to detect newer versions of the static analyzer tools.
* configure.ac (lv_cv_static_analysis): New cache variable.
---
I _thought_ some of those false positives were looking familiar
from my last coverity run. Turns out I inadvertantly managed
to lose my STATIC_ANALYSIS define.
configure.ac | 11 +--
1 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/configure.ac b/configure.ac
index 7c4fb32..10487e5 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2404,9 +2404,16 @@ cp -f COPYING.LIB COPYING
# Detect when running under the clang static analyzer's scan-build driver
# or Coverity-prevent's cov-build. Define STATIC_ANALYSIS accordingly.
+AC_CACHE_CHECK([whether this build is done by a static analysis tool],
+ [lv_cv_static_analysis], [
+lv_cv_static_analysis=no
+if test -n ${CCC_ANALYZER_ANALYSIS+set} || \
+ test -n $COVERITY_BUILD_COMMAND$COVERITY_LD_PRELOAD; then
+ lv_cv_static_analysis=yes
+fi
+ ])
t=0
-test -n ${CCC_ANALYZER_ANALYSIS+set} t=1
-test -n $COVERITY_BUILD_COMMAND$COVERITY_LD_PRELOAD t=1
+test x$lv_cv_static_analysis = xyes t=1
AC_DEFINE_UNQUOTED([STATIC_ANALYSIS], [$t],
[Define to 1 when performing static analysis.])
ACK.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] fdstream: drop delete argument
On 08/02/2011 01:18 PM, Laine Stump wrote: On 08/02/2011 01:31 PM, Eric Blake wrote: Revert 6a1f5f568f8. Now that libvirt_iohelper no longer has a race where it can open() a file after the parent process has unlink()d the same file, it makes more sense to make the callers both create and unlink, rather than the caller create and the stream unlink. I wasn't paying attention to the messages/patches related to the race condition you reference, Commit 1eb66479 plugged the race; commit 6a1f5f5 introduced the race in the first place. The problem was that if we use libvirt_iohelper, and the child process calls open(), but the parent process calls unlink() before the child process gets to run very far, then the child process will fail to open(). But by changing fdstream to pass the fd to libvirt-iohelper by fd inheritance instead of by name-wise open() calls, there is no longer an open() race, so we can once again unlink() in the parent. but this (caller creates and unlinks) definitely seems cleaner than the other way. Beyond that, the patch seems to be correct. ACK. Should this go in for 0.9.4, or am I correct in deferring it until after the release? -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 7/3] rpc: avoid crash on error
Detected by Coverity. Freeing the wrong variable results in both
a memory leak and the likelihood of the caller dereferencing through
a freed pointer.
* src/rpc/virnettlscontext.c (virNetTLSSessionNew): Free correct
variable.
---
src/rpc/virnettlscontext.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c
index be08207..eeffe54 100644
--- a/src/rpc/virnettlscontext.c
+++ b/src/rpc/virnettlscontext.c
@@ -1164,17 +1164,17 @@ virNetTLSSessionPtr
virNetTLSSessionNew(virNetTLSContextPtr ctxt,
if (VIR_ALLOC(sess) 0) {
virReportOOMError();
return NULL;
}
if (virMutexInit(sess-lock) 0) {
virNetError(VIR_ERR_INTERNAL_ERROR, %s,
_(Failed to initialized mutex));
-VIR_FREE(ctxt);
+VIR_FREE(sess);
return NULL;
}
sess-refs = 1;
if (hostname
!(sess-hostname = strdup(hostname))) {
virReportOOMError();
goto error;
--
1.7.4.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 8/3] conf: avoid memory leak
Detected by Coverity. Introduced in commit 85aa40e.
* src/conf/domain_conf.c (virDomainDiskDefForeachPath): Plug leak.
---
src/conf/domain_conf.c |6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e182cd6..010ce57 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -11315,7 +11315,7 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr
disk,
virDomainDiskDefPathIterator iter,
void *opaque)
{
-virHashTablePtr paths;
+virHashTablePtr paths = NULL;
int format;
int ret = -1;
size_t depth = 0;
@@ -11339,7 +11339,7 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr
disk,
virDomainReportError(VIR_ERR_INTERNAL_ERROR,
_(unknown disk format '%s' for %s),
disk-driverType, disk-src);
-return -1;
+goto cleanup;
}
} else {
if (allowProbing) {
@@ -11348,7 +11348,7 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr
disk,
virDomainReportError(VIR_ERR_INTERNAL_ERROR,
_(no disk format for %s and probing is
disabled),
disk-src);
-return -1;
+goto cleanup;
}
}
--
1.7.4.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: avoid memory leaks
Quite a few leaks detected by coverity. For chr, the leaks were
close enough to the allocations to plug in place; for disk, the
leaks were separated from the allocation by enough other lines with
intermediate failure cases that I refactored the cleanup instead.
* src/qemu/qemu_command.c (qemuParseCommandLine): Plug leaks.
---
src/qemu/qemu_command.c | 23 ---
1 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 6a2e2ae..c638420 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5938,6 +5938,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
int video = VIR_DOMAIN_VIDEO_TYPE_CIRRUS;
int nvirtiodisk = 0;
qemuDomainCmdlineDefPtr cmd = NULL;
+virDomainDiskDefPtr disk = NULL;
if (pidfile)
*pidfile = NULL;
@@ -6124,7 +6125,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
STRPREFIX(arg, -fd) ||
STREQ(arg, -cdrom)) {
WANT_VALUE();
-virDomainDiskDefPtr disk;
if (VIR_ALLOC(disk) 0)
goto no_memory;
@@ -6239,6 +6239,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
goto no_memory;
}
def-disks[def-ndisks++] = disk;
+disk = NULL;
} else if (STREQ(arg, -no-acpi)) {
def-features = ~(1 VIR_DOMAIN_FEATURE_ACPI);
} else if (STREQ(arg, -no-reboot)) {
@@ -6307,8 +6308,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
if (!(chr = virDomainChrDefNew()))
goto error;
-if (qemuParseCommandLineChr(chr-source, val) 0)
+if (qemuParseCommandLineChr(chr-source, val) 0) {
+virDomainChrDefFree(chr);
goto error;
+}
if (VIR_REALLOC_N(def-serials, def-nserials+1) 0) {
virDomainChrDefFree(chr);
goto no_memory;
@@ -6325,8 +6328,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
if (!(chr = virDomainChrDefNew()))
goto error;
-if (qemuParseCommandLineChr(chr-source, val) 0)
+if (qemuParseCommandLineChr(chr-source, val) 0) {
+virDomainChrDefFree(chr);
goto error;
+}
if (VIR_REALLOC_N(def-parallels, def-nparallels+1) 0) {
virDomainChrDefFree(chr);
goto no_memory;
@@ -6353,7 +6358,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
}
def-inputs[def-ninputs++] = input;
} else if (STRPREFIX(val, disk:)) {
-virDomainDiskDefPtr disk;
if (VIR_ALLOC(disk) 0)
goto no_memory;
disk-src = strdup(val + strlen(disk:));
@@ -6376,6 +6380,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
goto no_memory;
}
def-disks[def-ndisks++] = disk;
+disk = NULL;
} else {
virDomainHostdevDefPtr hostdev;
if (!(hostdev = qemuParseCommandLineUSB(val)))
@@ -6399,7 +6404,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
def-nets[def-nnets++] = net;
}
} else if (STREQ(arg, -drive)) {
-virDomainDiskDefPtr disk;
WANT_VALUE();
if (!(disk = qemuParseCommandLineDisk(caps, val, nvirtiodisk)))
goto error;
@@ -6408,6 +6412,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
goto no_memory;
}
def-disks[def-ndisks++] = disk;
+disk = NULL;
if (disk-bus == VIR_DOMAIN_DISK_BUS_VIRTIO)
nvirtiodisk++;
@@ -6516,8 +6521,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
if (VIR_ALLOC(chr) 0)
goto no_memory;
-if (qemuParseCommandLineChr(chr, val) 0)
+if (qemuParseCommandLineChr(chr, val) 0) {
+VIR_FREE(chr);
goto error;
+}
*monConfig = chr;
}
@@ -6545,10 +6552,11 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
char *hosts, *port, *saveptr = NULL, *token;
virDomainDiskDefPtr first_rbd_disk = NULL;
for (i = 0 ; i def-ndisks ; i++) {
-virDomainDiskDefPtr disk = def-disks[i];
+disk = def-disks[i];
if (disk-type == VIR_DOMAIN_DISK_TYPE_NETWORK
disk-protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) {
first_rbd_disk = disk;
+disk = NULL;
break;
}
}
@@
[libvirt] [PATCH 10/3] qemu: remove dead code
Warning detected by Coverity. No need for the NULL check, and
removing it silences the warning without any semantic change.
* src/qemu/qemu_migration.c (qemuMigrationFinish): All entries to
endjob had non-NULL vm.
---
Oops - I forgot to label the subject for 9/3 properly.
src/qemu/qemu_migration.c | 12 +---
1 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 8bdbcaf..7aeea69 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2569,13 +2569,11 @@ qemuMigrationFinish(struct qemud_driver *driver,
VIR_WARN(Unable to encode migration cookie);
endjob:
-if (vm) {
-if (qemuMigrationJobFinish(driver, vm) == 0) {
-vm = NULL;
-} else if (!vm-persistent !virDomainObjIsActive(vm)) {
-virDomainRemoveInactive(driver-domains, vm);
-vm = NULL;
-}
+if (qemuMigrationJobFinish(driver, vm) == 0) {
+vm = NULL;
+} else if (!vm-persistent !virDomainObjIsActive(vm)) {
+virDomainRemoveInactive(driver-domains, vm);
+vm = NULL;
}
cleanup:
--
1.7.4.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 11/3] qemu: plug child process leak on domain core dump
Detected by Coverity. Leak introduced by typo in commit 58e668d2. * src/qemu/qemu_driver.c (doCoreDump): Use correct function. --- src/qemu/qemu_driver.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0297159..ea24df8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2717,7 +2717,7 @@ doCoreDump(struct qemud_driver *driver, cleanup: VIR_FORCE_CLOSE(fd); -virFileDirectFdClose(directFd); +virFileDirectFdFree(directFd); if (ret != 0) unlink(path); return ret; -- 1.7.4.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 10/3] qemu: remove dead code
On 08/02/2011 04:20 PM, Eric Blake wrote: Warning detected by Coverity. No need for the NULL check, and removing it silences the warning without any semantic change. True. The first function called in qemuMigrationFinish dereferences vm, so we would have crashed long before if it was NULL. ACK. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: avoid memory leaks
On 08/02/2011 04:10 PM, Eric Blake wrote:
Quite a few leaks detected by coverity. For chr, the leaks were
close enough to the allocations to plug in place; for disk, the
leaks were separated from the allocation by enough other lines with
intermediate failure cases that I refactored the cleanup instead.
* src/qemu/qemu_command.c (qemuParseCommandLine): Plug leaks.
---
src/qemu/qemu_command.c | 23 ---
1 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 6a2e2ae..c638420 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5938,6 +5938,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
int video = VIR_DOMAIN_VIDEO_TYPE_CIRRUS;
int nvirtiodisk = 0;
qemuDomainCmdlineDefPtr cmd = NULL;
+virDomainDiskDefPtr disk = NULL;
if (pidfile)
*pidfile = NULL;
@@ -6124,7 +6125,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
STRPREFIX(arg, -fd) ||
STREQ(arg, -cdrom)) {
WANT_VALUE();
-virDomainDiskDefPtr disk;
if (VIR_ALLOC(disk) 0)
goto no_memory;
@@ -6239,6 +6239,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
goto no_memory;
}
def-disks[def-ndisks++] = disk;
+disk = NULL;
} else if (STREQ(arg, -no-acpi)) {
def-features= ~(1 VIR_DOMAIN_FEATURE_ACPI);
} else if (STREQ(arg, -no-reboot)) {
@@ -6307,8 +6308,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
if (!(chr = virDomainChrDefNew()))
goto error;
-if (qemuParseCommandLineChr(chr-source, val) 0)
+if (qemuParseCommandLineChr(chr-source, val) 0) {
+virDomainChrDefFree(chr);
goto error;
+}
if (VIR_REALLOC_N(def-serials, def-nserials+1) 0) {
virDomainChrDefFree(chr);
goto no_memory;
@@ -6325,8 +6328,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
if (!(chr = virDomainChrDefNew()))
goto error;
-if (qemuParseCommandLineChr(chr-source, val) 0)
+if (qemuParseCommandLineChr(chr-source, val) 0) {
+virDomainChrDefFree(chr);
goto error;
+}
if (VIR_REALLOC_N(def-parallels, def-nparallels+1) 0) {
virDomainChrDefFree(chr);
goto no_memory;
@@ -6353,7 +6358,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
}
def-inputs[def-ninputs++] = input;
} else if (STRPREFIX(val, disk:)) {
-virDomainDiskDefPtr disk;
if (VIR_ALLOC(disk) 0)
goto no_memory;
disk-src = strdup(val + strlen(disk:));
@@ -6376,6 +6380,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
goto no_memory;
}
def-disks[def-ndisks++] = disk;
+disk = NULL;
} else {
virDomainHostdevDefPtr hostdev;
if (!(hostdev = qemuParseCommandLineUSB(val)))
@@ -6399,7 +6404,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
def-nets[def-nnets++] = net;
}
} else if (STREQ(arg, -drive)) {
-virDomainDiskDefPtr disk;
WANT_VALUE();
if (!(disk = qemuParseCommandLineDisk(caps, val, nvirtiodisk)))
goto error;
@@ -6408,6 +6412,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
goto no_memory;
}
def-disks[def-ndisks++] = disk;
+disk = NULL;
if (disk-bus == VIR_DOMAIN_DISK_BUS_VIRTIO)
nvirtiodisk++;
@@ -6516,8 +6521,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
if (VIR_ALLOC(chr) 0)
goto no_memory;
-if (qemuParseCommandLineChr(chr, val) 0)
+if (qemuParseCommandLineChr(chr, val) 0) {
+VIR_FREE(chr);
Shouldn't this be virDomainChrDefFree(chr)?
goto error;
+}
*monConfig = chr;
}
@@ -6545,10 +6552,11 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
char *hosts, *port, *saveptr = NULL, *token;
virDomainDiskDefPtr first_rbd_disk = NULL;
for (i = 0 ; i def-ndisks ; i++) {
-virDomainDiskDefPtr disk = def-disks[i];
+disk = def-disks[i];
if (disk-type == VIR_DOMAIN_DISK_TYPE_NETWORK
disk-protocol ==
Re: [libvirt] [PATCH 8/3] conf: avoid memory leak
On 08/02/2011 03:50 PM, Eric Blake wrote:
Detected by Coverity. Introduced in commit 85aa40e.
leak of meta - important point, since the thing being leaked doesn't
show up anywhere in the diff.
ACK
* src/conf/domain_conf.c (virDomainDiskDefForeachPath): Plug leak.
---
src/conf/domain_conf.c |6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e182cd6..010ce57 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -11315,7 +11315,7 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr
disk,
virDomainDiskDefPathIterator iter,
void *opaque)
{
-virHashTablePtr paths;
+virHashTablePtr paths = NULL;
int format;
int ret = -1;
size_t depth = 0;
@@ -11339,7 +11339,7 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr
disk,
virDomainReportError(VIR_ERR_INTERNAL_ERROR,
_(unknown disk format '%s' for %s),
disk-driverType, disk-src);
-return -1;
+goto cleanup;
}
} else {
if (allowProbing) {
@@ -11348,7 +11348,7 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr
disk,
virDomainReportError(VIR_ERR_INTERNAL_ERROR,
_(no disk format for %s and probing is
disabled),
disk-src);
-return -1;
+goto cleanup;
}
}
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 7/3] rpc: avoid crash on error
On 08/02/2011 03:38 PM, Eric Blake wrote:
Detected by Coverity. Freeing the wrong variable results in both
a memory leak and the likelihood of the caller dereferencing through
a freed pointer.
* src/rpc/virnettlscontext.c (virNetTLSSessionNew): Free correct
variable.
---
src/rpc/virnettlscontext.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c
index be08207..eeffe54 100644
--- a/src/rpc/virnettlscontext.c
+++ b/src/rpc/virnettlscontext.c
@@ -1164,17 +1164,17 @@ virNetTLSSessionPtr
virNetTLSSessionNew(virNetTLSContextPtr ctxt,
if (VIR_ALLOC(sess) 0) {
virReportOOMError();
return NULL;
}
if (virMutexInit(sess-lock) 0) {
virNetError(VIR_ERR_INTERNAL_ERROR, %s,
_(Failed to initialized mutex));
-VIR_FREE(ctxt);
+VIR_FREE(sess);
return NULL;
You could just as well replace this with a goto error, but ACK anyway.
}
sess-refs = 1;
if (hostname
!(sess-hostname = strdup(hostname))) {
virReportOOMError();
goto error;
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/3] qemu: avoid null deref on block pull error
On 08/02/2011 03:21 PM, Eric Blake wrote: Coverity detected that 5 of 6 callers of virJSONValueArrayGet checked for a NULL return; and that by not checking we risk a null deref during an error. The error is unlikely since the prior call to virJSONValueArraySize would probably have already caught any botched JSON array parse, but better safe than sorry. ACK. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 11/3] qemu: plug child process leak on domain core dump
On 08/02/2011 04:38 PM, Eric Blake wrote: Detected by Coverity. Leak introduced by typo in commit 58e668d2. * src/qemu/qemu_driver.c (doCoreDump): Use correct function. --- src/qemu/qemu_driver.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0297159..ea24df8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2717,7 +2717,7 @@ doCoreDump(struct qemud_driver *driver, cleanup: VIR_FORCE_CLOSE(fd); -virFileDirectFdClose(directFd); +virFileDirectFdFree(directFd); if (ret != 0) unlink(path); return ret; ACK. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] fdstream: drop delete argument
On 08/02/2011 01:29 PM, Eric Blake wrote: On 08/02/2011 01:18 PM, Laine Stump wrote: On 08/02/2011 01:31 PM, Eric Blake wrote: Revert 6a1f5f568f8. Now that libvirt_iohelper no longer has a race where it can open() a file after the parent process has unlink()d the same file, it makes more sense to make the callers both create and unlink, rather than the caller create and the stream unlink. I wasn't paying attention to the messages/patches related to the race condition you reference, Commit 1eb66479 plugged the race; commit 6a1f5f5 introduced the race in the first place. The problem was that if we use libvirt_iohelper, and the child process calls open(), but the parent process calls unlink() before the child process gets to run very far, then the child process will fail to open(). But by changing fdstream to pass the fd to libvirt-iohelper by fd inheritance instead of by name-wise open() calls, there is no longer an open() race, so we can once again unlink() in the parent. but this (caller creates and unlinks) definitely seems cleaner than the other way. Beyond that, the patch seems to be correct. ACK. Should this go in for 0.9.4, or am I correct in deferring it until after the release? Laine and I chatted some more on IRC: eblake laine: should I push both patches now (or even squash together), or delay the second patch till after the release? laine eblake: I don't see a problem with the second patch, I think I like it better. Not knowing the history of the race you mentioned and whether or not this code still touches it, I wouldn't single-handledly say to squash the two together, but if you're comfortable with it, then it looks good to me. eblake I'll update the commit message of the second; the race was first solved in 6a1f5f5 by making the parent not unlink() if libvirt_iohelper will also be open()ing the file eblake but solving it in that manner required passing delete=true all the way through the fdstream code eblake later, I pushed commit 1eb66479, which taught libvirt_iostream to operate on an inherited fd instead of calling open() eblake and in so doing, the child no longer needs to unlink() eblake so we no longer have the unlink() in parent prior to open()/unlink() in the child, and can get rid of the extra parameter Given that, I went ahead and updated the commit comment, then pushed both patches separately, to make it into 0.9.4. Revert 6a1f5f568f8. Now that libvirt_iohelper takes fds by inheritance rather than by open() (commit 1eb66479), there is no longer a race where the parent can unlink() a file prior to the iohelper open()ing the same file. From there, it makes more sense to have the callers both create and unlink, rather than the caller create and the stream unlink, since the latter was only needed when iohelper had to do the unlink. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 8/3] conf: avoid memory leak
On 08/02/2011 02:53 PM, Laine Stump wrote: On 08/02/2011 03:50 PM, Eric Blake wrote: Detected by Coverity. Introduced in commit 85aa40e. leak of meta - important point, since the thing being leaked doesn't show up anywhere in the diff. ACK Good point. I've now pushed 4-8, with this commit comment for 8: conf: avoid memory leak on disk operations Detected by Coverity. Leak on meta introduced in commit 85aa40e. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 11/3] qemu: plug child process leak on domain core dump
On 08/02/2011 02:59 PM, Laine Stump wrote: On 08/02/2011 04:38 PM, Eric Blake wrote: Detected by Coverity. Leak introduced by typo in commit 58e668d2. * src/qemu/qemu_driver.c (doCoreDump): Use correct function. --- src/qemu/qemu_driver.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0297159..ea24df8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2717,7 +2717,7 @@ doCoreDump(struct qemud_driver *driver, cleanup: VIR_FORCE_CLOSE(fd); - virFileDirectFdClose(directFd); + virFileDirectFdFree(directFd); if (ret != 0) unlink(path); return ret; ACK. I've pushed 10 and 11; I'll respin 9 to address your comments. More still probably on the way... -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Libvirt] [PATCH v2] Fix bug #611823 prohibit pools with duplicate storage
On Wed, Aug 03, 2011 at 12:52:50AM +0800, Lei Li wrote:
On 08/02/2011 07:11 PM, Daniel P. Berrange wrote:
On Mon, Aug 01, 2011 at 02:12:51PM -0600, Eric Blake wrote:
On 07/31/2011 10:58 PM, Lei Li wrote:
Make sure the unique storage pool defined and create from different
directory to avoid inconsistent version of volume pool created.
Wrap your commit messages; typically at 70 columns or so (since 'git
log' adds some indentation, but you want the end result to still fit
in 80 columns for legibility).
Signed-off-by: Lei Lili...@linux.vnet.ibm.com
---
src/conf/storage_conf.c | 36
src/conf/storage_conf.h |4
src/libvirt_private.syms |2 ++
src/storage/storage_driver.c |6 ++
4 files changed, 48 insertions(+), 0 deletions(-)
+virStoragePoolObjPtr
+virStoragePoolObjFindByPath(virStoragePoolObjListPtr pools,
+const char *path) {
+unsigned int i;
+
+for (i = 0 ; i pools-count ; i++) {
+virStoragePoolObjLock(pools-objs[i]);
+if (STREQ(pools-objs[i]-def-target.path, path))
+return pools-objs[i];
+virStoragePoolObjUnlock(pools-objs[i]);
+}
+
+return NULL;
+}
This one is good; in fact, we may even want to expose it as a public
API, parallel to other virStoragePoolLookupBy* functions (but not
until after 0.9.4 is released)
No, this API is flawed because def-target.path is not required to
be unique for all types of storage pool.
Daniel
Yes, in the beginning it seems like target-path is not required to be
unique. But for this bug https://bugzilla.redhat.com/show_bug.cgi?id=611823
you reported, you said that For example, if two directory pools point to the
same directory, and one pool is used to create a volume,
the other pool will remain unaware of the new volume until it is refreshed.
And I have test it when use 'virsh pool-define/create' it will create more
than two pools not two have the same directory. I think maybe you should look
at the description of the bug first.
This API virStoragePoolObjFindByPath() just provide a method to search pool
obj by path and can be use to avoid duplicate target path to fix this bug you
mentioned.
Ah a slight misunderstanding here. There are quite a few different pieces
of metadata with storage pools, and in some cases they are the same.
- name/uuid - usual unique metadata for a storage pool
- source - the data for the source varies according to storage pool type
- hostname
- directory path
- adaptor
- device path
- source name
- initiator IQN
- target - a path that controls how storage volume paths are formed
I think your misunderstanding is because for 'directory' storage pools,
the source directory path is actually the same as the target path.
So if you want to do uniqueness checking for storage pools, you need
todo it based on the source information, rather than the target path.
The checks will of course need to be slightly different for each
storage pool type.
Regards,
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: avoid memory leaks
On 08/02/2011 02:48 PM, Laine Stump wrote:
On 08/02/2011 04:10 PM, Eric Blake wrote:
Quite a few leaks detected by coverity. For chr, the leaks were
close enough to the allocations to plug in place; for disk, the
leaks were separated from the allocation by enough other lines with
intermediate failure cases that I refactored the cleanup instead.
- if (qemuParseCommandLineChr(chr, val) 0)
+ if (qemuParseCommandLineChr(chr, val) 0) {
+ VIR_FREE(chr);
Shouldn't this be virDomainChrDefFree(chr)?
virDomainDiskDefPtr first_rbd_disk = NULL;
for (i = 0 ; i def-ndisks ; i++) {
- virDomainDiskDefPtr disk = def-disks[i];
+ disk = def-disks[i];
if (disk-type == VIR_DOMAIN_DISK_TYPE_NETWORK
disk-protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) {
first_rbd_disk = disk;
+ disk = NULL;
break;
}
If you never hit the if condition, disk will be left pointing to one of
the disks in def, and will be freed during cleanup. I think you want to
set disk = NULL after this look is finished as well, don't you? (Either
that, or just use a different variable).
Both good points. I'm squashing this in to create v2.
diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c
index c638420..194f3ff 100644
--- i/src/qemu/qemu_command.c
+++ w/src/qemu/qemu_command.c
@@ -6522,7 +6522,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
goto no_memory;
if (qemuParseCommandLineChr(chr, val) 0) {
-VIR_FREE(chr);
+virDomainChrSourceDefFree(chr);
goto error;
}
@@ -6552,11 +6552,9 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
char *hosts, *port, *saveptr = NULL, *token;
virDomainDiskDefPtr first_rbd_disk = NULL;
for (i = 0 ; i def-ndisks ; i++) {
-disk = def-disks[i];
-if (disk-type == VIR_DOMAIN_DISK_TYPE_NETWORK
-disk-protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) {
-first_rbd_disk = disk;
-disk = NULL;
+if (def-disks[i]-type == VIR_DOMAIN_DISK_TYPE_NETWORK
+def-disks[i]-protocol ==
VIR_DOMAIN_DISK_PROTOCOL_RBD) {
+first_rbd_disk = def-disks[i];
break;
}
}
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2 9/3] qemu: avoid memory leaks
Quite a few leaks detected by coverity. For chr, the leaks were
close enough to the allocations to plug in place; for disk, the
leaks were separated from the allocation by enough other lines with
intermediate failure cases that I refactored the cleanup instead.
* src/qemu/qemu_command.c (qemuParseCommandLine): Plug leaks.
---
v2: address review comments from v1
src/qemu/qemu_command.c | 27 +--
1 files changed, 17 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 6a2e2ae..194f3ff 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5938,6 +5938,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
int video = VIR_DOMAIN_VIDEO_TYPE_CIRRUS;
int nvirtiodisk = 0;
qemuDomainCmdlineDefPtr cmd = NULL;
+virDomainDiskDefPtr disk = NULL;
if (pidfile)
*pidfile = NULL;
@@ -6124,7 +6125,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
STRPREFIX(arg, -fd) ||
STREQ(arg, -cdrom)) {
WANT_VALUE();
-virDomainDiskDefPtr disk;
if (VIR_ALLOC(disk) 0)
goto no_memory;
@@ -6239,6 +6239,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
goto no_memory;
}
def-disks[def-ndisks++] = disk;
+disk = NULL;
} else if (STREQ(arg, -no-acpi)) {
def-features = ~(1 VIR_DOMAIN_FEATURE_ACPI);
} else if (STREQ(arg, -no-reboot)) {
@@ -6307,8 +6308,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
if (!(chr = virDomainChrDefNew()))
goto error;
-if (qemuParseCommandLineChr(chr-source, val) 0)
+if (qemuParseCommandLineChr(chr-source, val) 0) {
+virDomainChrDefFree(chr);
goto error;
+}
if (VIR_REALLOC_N(def-serials, def-nserials+1) 0) {
virDomainChrDefFree(chr);
goto no_memory;
@@ -6325,8 +6328,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
if (!(chr = virDomainChrDefNew()))
goto error;
-if (qemuParseCommandLineChr(chr-source, val) 0)
+if (qemuParseCommandLineChr(chr-source, val) 0) {
+virDomainChrDefFree(chr);
goto error;
+}
if (VIR_REALLOC_N(def-parallels, def-nparallels+1) 0) {
virDomainChrDefFree(chr);
goto no_memory;
@@ -6353,7 +6358,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
}
def-inputs[def-ninputs++] = input;
} else if (STRPREFIX(val, disk:)) {
-virDomainDiskDefPtr disk;
if (VIR_ALLOC(disk) 0)
goto no_memory;
disk-src = strdup(val + strlen(disk:));
@@ -6376,6 +6380,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
goto no_memory;
}
def-disks[def-ndisks++] = disk;
+disk = NULL;
} else {
virDomainHostdevDefPtr hostdev;
if (!(hostdev = qemuParseCommandLineUSB(val)))
@@ -6399,7 +6404,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
def-nets[def-nnets++] = net;
}
} else if (STREQ(arg, -drive)) {
-virDomainDiskDefPtr disk;
WANT_VALUE();
if (!(disk = qemuParseCommandLineDisk(caps, val, nvirtiodisk)))
goto error;
@@ -6408,6 +6412,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
goto no_memory;
}
def-disks[def-ndisks++] = disk;
+disk = NULL;
if (disk-bus == VIR_DOMAIN_DISK_BUS_VIRTIO)
nvirtiodisk++;
@@ -6516,8 +6521,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
if (VIR_ALLOC(chr) 0)
goto no_memory;
-if (qemuParseCommandLineChr(chr, val) 0)
+if (qemuParseCommandLineChr(chr, val) 0) {
+virDomainChrSourceDefFree(chr);
goto error;
+}
*monConfig = chr;
}
@@ -6545,10 +6552,9 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
char *hosts, *port, *saveptr = NULL, *token;
virDomainDiskDefPtr first_rbd_disk = NULL;
for (i = 0 ; i def-ndisks ; i++) {
-virDomainDiskDefPtr disk = def-disks[i];
-if (disk-type == VIR_DOMAIN_DISK_TYPE_NETWORK
-disk-protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) {
-first_rbd_disk = disk;
+if (def-disks[i]-type == VIR_DOMAIN_DISK_TYPE_NETWORK
+
Re: [libvirt] [PATCHv2 9/3] qemu: avoid memory leaks
On 08/02/2011 03:46 PM, Eric Blake wrote:
Quite a few leaks detected by coverity. For chr, the leaks were
close enough to the allocations to plug in place; for disk, the
leaks were separated from the allocation by enough other lines with
intermediate failure cases that I refactored the cleanup instead.
* src/qemu/qemu_command.c (qemuParseCommandLine): Plug leaks.
---
v2: address review comments from v1
squash this in too (serves me right for hitting send prior to 'make
check' completing).
diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c
index 194f3ff..067e53e 100644
--- i/src/qemu/qemu_command.c
+++ w/src/qemu/qemu_command.c
@@ -6411,11 +6411,11 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr
caps,
virDomainDiskDefFree(disk);
goto no_memory;
}
-def-disks[def-ndisks++] = disk;
-disk = NULL;
-
if (disk-bus == VIR_DOMAIN_DISK_BUS_VIRTIO)
nvirtiodisk++;
+
+def-disks[def-ndisks++] = disk;
+disk = NULL;
} else if (STREQ(arg, -pcidevice)) {
virDomainHostdevDefPtr hostdev;
WANT_VALUE();
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 12/3] build: silence coverity false positives
Coverity complained that 395 out of 409 virAsprintf calls are
checked, and therefore assumed that the remaining cases are bugs
waiting to happen. But in each of these cases, a failed virAsprintf
will properly set the target string to NULL, and pass on that
failure to the caller, without wasting efforts to check the call.
Adding the ignore_value silences Coverity.
* src/conf/domain_audit.c (virDomainAuditGetRdev): Ignore
virAsprintf return value, when it behaves like we need.
* src/network/bridge_driver.c (networkDnsmasqLeaseFileNameDefault)
(networkRadvdConfigFileName, networkBridgeDummyNicName)
(networkRadvdPidfileBasename): Likewise.
* src/util/storage_file.c (absolutePathFromBaseFile): Likewise.
* src/openvz/openvz_driver.c (openvzGenerateContainerVethName):
Likewise.
* src/util/command.c (virCommandTranslateStatus): Likewise.
---
src/conf/domain_audit.c |2 +-
src/network/bridge_driver.c | 22 --
src/openvz/openvz_driver.c |2 +-
src/util/command.c |8 +---
src/util/storage_file.c |2 +-
5 files changed, 20 insertions(+), 16 deletions(-)
diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c
index 963eecb..9d89c94 100644
--- a/src/conf/domain_audit.c
+++ b/src/conf/domain_audit.c
@@ -46,7 +46,7 @@ virDomainAuditGetRdev(const char *path)
(S_ISCHR(sb.st_mode) || S_ISBLK(sb.st_mode))) {
int maj = major(sb.st_rdev);
int min = minor(sb.st_rdev);
-virAsprintf(ret, %02X:%02X, maj, min);
+ignore_value(virAsprintf(ret, %02X:%02X, maj, min));
}
return ret;
}
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 0a60bb8..c7d2dfd 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -60,6 +60,7 @@
#include dnsmasq.h
#include util/network.h
#include configmake.h
+#include ignore-value.h
#define NETWORK_PID_DIR LOCALSTATEDIR /run/libvirt/network
#define NETWORK_STATE_DIR LOCALSTATEDIR /lib/libvirt/network
@@ -125,8 +126,8 @@ networkDnsmasqLeaseFileNameDefault(const char *netname)
{
char *leasefile;
-virAsprintf(leasefile, DNSMASQ_STATE_DIR /%s.leases,
-netname);
+ignore_value(virAsprintf(leasefile, DNSMASQ_STATE_DIR /%s.leases,
+ netname));
return leasefile;
}
@@ -139,7 +140,7 @@ networkRadvdPidfileBasename(const char *netname)
/* this is simple but we want to be sure it's consistently done */
char *pidfilebase;
-virAsprintf(pidfilebase, %s-radvd, netname);
+ignore_value(virAsprintf(pidfilebase, %s-radvd, netname));
return pidfilebase;
}
@@ -148,8 +149,8 @@ networkRadvdConfigFileName(const char *netname)
{
char *configfile;
-virAsprintf(configfile, RADVD_STATE_DIR /%s-radvd.conf,
-netname);
+ignore_value(virAsprintf(configfile, RADVD_STATE_DIR /%s-radvd.conf,
+ netname));
return configfile;
}
@@ -166,12 +167,13 @@ networkBridgeDummyNicName(const char *brname)
* a possible numeric ending (eg virbr0, virbr1, etc), we grab
* the first 8 and last 3 characters of the string.
*/
- virAsprintf(nicname, %.*s%s%s,
- /* space for last 3 chars + -nic + NULL */
- (int)(IFNAMSIZ - (3 + sizeof(dummyNicSuffix))),
- brname, brname + strlen(brname) - 3, dummyNicSuffix);
+ignore_value(virAsprintf(nicname, %.*s%s%s,
+ /* space for last 3 chars + -nic + NULL */
+ (int)(IFNAMSIZ - (3 +
sizeof(dummyNicSuffix))),
+ brname, brname + strlen(brname) - 3,
+ dummyNicSuffix));
} else {
- virAsprintf(nicname, %s%s, brname, dummyNicSuffix);
+ignore_value(virAsprintf(nicname, %s%s, brname, dummyNicSuffix));
}
return nicname;
}
diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
index df2079e..b9dc712 100644
--- a/src/openvz/openvz_driver.c
+++ b/src/openvz/openvz_driver.c
@@ -710,7 +710,7 @@ openvzGenerateContainerVethName(int veid)
}
/* set new name */
-virAsprintf(name, eth%d, max + 1);
+ignore_value(virAsprintf(name, eth%d, max + 1));
}
VIR_FREE(temp);
diff --git a/src/util/command.c b/src/util/command.c
index 475eb62..26fcb28 100644
--- a/src/util/command.c
+++ b/src/util/command.c
@@ -1543,11 +1543,13 @@ virCommandTranslateStatus(int status)
{
char *buf;
if (WIFEXITED(status)) {
-virAsprintf(buf, _(exit status %d), WEXITSTATUS(status));
+ignore_value(virAsprintf(buf, _(exit status %d),
+ WEXITSTATUS(status)));
} else if (WIFSIGNALED(status)) {
-virAsprintf(buf, _(fatal signal %d), WTERMSIG(status));
+ignore_value(virAsprintf(buf, _(fatal signal %d),
+ WTERMSIG(status)));
}
Re: [libvirt] [PATCH] qemu: avoid memory leaks
On 08/02/2011 05:46 PM, Eric Blake wrote:
On 08/02/2011 02:48 PM, Laine Stump wrote:
On 08/02/2011 04:10 PM, Eric Blake wrote:
Quite a few leaks detected by coverity. For chr, the leaks were
close enough to the allocations to plug in place; for disk, the
leaks were separated from the allocation by enough other lines with
intermediate failure cases that I refactored the cleanup instead.
- if (qemuParseCommandLineChr(chr, val) 0)
+ if (qemuParseCommandLineChr(chr, val) 0) {
+ VIR_FREE(chr);
Shouldn't this be virDomainChrDefFree(chr)?
virDomainDiskDefPtr first_rbd_disk = NULL;
for (i = 0 ; i def-ndisks ; i++) {
- virDomainDiskDefPtr disk = def-disks[i];
+ disk = def-disks[i];
if (disk-type == VIR_DOMAIN_DISK_TYPE_NETWORK
disk-protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) {
first_rbd_disk = disk;
+ disk = NULL;
break;
}
If you never hit the if condition, disk will be left pointing to one of
the disks in def, and will be freed during cleanup. I think you want to
set disk = NULL after this look is finished as well, don't you? (Either
that, or just use a different variable).
Both good points. I'm squashing this in to create v2.
diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c
index c638420..194f3ff 100644
--- i/src/qemu/qemu_command.c
+++ w/src/qemu/qemu_command.c
@@ -6522,7 +6522,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr
caps,
goto no_memory;
if (qemuParseCommandLineChr(chr, val) 0) {
-VIR_FREE(chr);
+virDomainChrSourceDefFree(chr);
goto error;
}
@@ -6552,11 +6552,9 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr
caps,
char *hosts, *port, *saveptr = NULL, *token;
virDomainDiskDefPtr first_rbd_disk = NULL;
for (i = 0 ; i def-ndisks ; i++) {
-disk = def-disks[i];
-if (disk-type == VIR_DOMAIN_DISK_TYPE_NETWORK
-disk-protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) {
-first_rbd_disk = disk;
-disk = NULL;
+if (def-disks[i]-type ==
VIR_DOMAIN_DISK_TYPE_NETWORK
+def-disks[i]-protocol ==
VIR_DOMAIN_DISK_PROTOCOL_RBD) {
+first_rbd_disk = def-disks[i];
break;
}
}
ACK with those changes.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 13/3] rpc: avoid null deref
Detected by Coverity.
* src/rpc/virnetserverclient.c (virNetServerClientDispatchRead):
Avoid null deref on OOM.
---
src/rpc/virnetserverclient.c |5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index 2f6c040..e246fa5 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -786,9 +786,10 @@ readmore:
if (client-nrequests client-nrequests_max) {
if (!(client-rx = virNetMessageNew())) {
client-wantClose = true;
+} else {
+client-rx-bufferLength = VIR_NET_MESSAGE_LEN_MAX;
+client-nrequests++;
}
-client-rx-bufferLength = VIR_NET_MESSAGE_LEN_MAX;
-client-nrequests++;
}
virNetServerClientUpdateEvent(client);
}
--
1.7.4.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 14/3] rpc: silence coverity false positives
In virNetServerNew, Coverity didn't realize that srv-mdsnGroupName
can only be non-NULL if mdsnGroupName was non-NULL.
In virNetServerRun, Coverity didn't realize that the array is non-NULL
if the array count is non-zero.
* src/rpc/virnetserver.c (virNetServerNew): Use alternate pointer.
(virNetServerRun): Give coverity a hint.
---
src/rpc/virnetserver.c |6 +-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 4deeca1..5e4826b 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -308,7 +308,8 @@ virNetServerPtr virNetServerNew(size_t min_workers,
if (srv-mdnsGroupName) {
if (!(srv-mdns = virNetServerMDNSNew()))
goto error;
-if (!(srv-mdnsGroup = virNetServerMDNSAddGroup(srv-mdns,
mdnsGroupName)))
+if (!(srv-mdnsGroup = virNetServerMDNSAddGroup(srv-mdns,
+srv-mdnsGroupName)))
goto error;
}
#endif
@@ -702,6 +703,9 @@ void virNetServerRun(virNetServerPtr srv)
reprocess:
for (i = 0 ; i srv-nclients ; i++) {
+/* Coverity 5.3.0 couldn't see that srv-clients is non-NULL
+ * if srv-nclients is non-zero. */
+sa_assert(srv-clients);
if (virNetServerClientWantClose(srv-clients[i]))
virNetServerClientClose(srv-clients[i]);
if (virNetServerClientIsClosed(srv-clients[i])) {
--
1.7.4.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 15/3] interface: drop dead code
Coverity detected that ifaceGetNthParent had already dereferenced 'nth' prior to the conditional; all callers already complied with passing a non-NULL pointer so make this part of the contract. * src/util/interface.h (ifaceGetNthParent): Add annotations. * src/util/interface.c (ifaceGetNthParent): Drop useless null check. --- src/util/interface.c |3 +-- src/util/interface.h |5 - 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/util/interface.c b/src/util/interface.c index f5eecfb..8b4522b 100644 --- a/src/util/interface.c +++ b/src/util/interface.c @@ -1060,8 +1060,7 @@ ifaceGetNthParent(int ifindex, const char *ifname, unsigned int nthParent, i++; } -if (nth) -*nth = i - 1; +*nth = i - 1; return rc; } diff --git a/src/util/interface.h b/src/util/interface.h index 9647653..47c0eb0 100644 --- a/src/util/interface.h +++ b/src/util/interface.h @@ -1,6 +1,7 @@ /* * interface.h: interface helper APIs for libvirt * + * Copyright (C) 2011 Red Hat, Inc. * Copyright (C) 2010 IBM Corporation, Inc. * * See COPYING.LIB for the License of this software @@ -67,7 +68,9 @@ int ifaceMacvtapLinkDump(bool nltarget_kernel, const char *ifname, int ifindex, int ifaceGetNthParent(int ifindex, const char *ifname, unsigned int nthParent, int *parent_ifindex, char *parent_ifname, - unsigned int *nth); + unsigned int *nth) +ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) +ATTRIBUTE_NONNULL(6); int ifaceReplaceMacAddress(const unsigned char *macaddress, const char *linkdev, -- 1.7.4.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 16/3] util: plug memory leak
Leak detected by Coverity; only possible on unlikely ptsname_r
failure. Additionally, the man page for ptsname_r states that
failure is merely non-zero, not necessarily -1.
* src/util/util.c (virFileOpenTtyAt): Avoid leak on ptsname_r
failure.
---
src/util/util.c |4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c
index 03a9e1a..2e2a6a0 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -1126,8 +1126,10 @@ int virFileOpenTtyAt(const char *ptmx,
goto cleanup;
}
-if (ptsname_r(*ttymaster, *ttyName, PATH_MAX) 0)
+if (ptsname_r(*ttymaster, *ttyName, PATH_MAX) != 0) {
+VIR_FREE(*ttyName);
goto cleanup;
+}
}
rc = 0;
--
1.7.4.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 9/3] qemu: avoid memory leaks
On 08/02/2011 05:46 PM, Eric Blake wrote:
Quite a few leaks detected by coverity. For chr, the leaks were
close enough to the allocations to plug in place; for disk, the
leaks were separated from the allocation by enough other lines with
intermediate failure cases that I refactored the cleanup instead.
Sorry, two more problems I didn't notice before:
1) instead of VIR_FREE(disk) down at the bottom, you should be calling
virDomainDiskDefFree()
2) There are some places in that qemuParseCommandLine where
virDomainDiskDefFree() is called right before goto no_memory (or goto
error), but since virDomainDiskDefFree() doesn't/can't set disk to NULL,
you'll end up with a double free.
* src/qemu/qemu_command.c (qemuParseCommandLine): Plug leaks.
---
v2: address review comments from v1
src/qemu/qemu_command.c | 27 +--
1 files changed, 17 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 6a2e2ae..194f3ff 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5938,6 +5938,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
int video = VIR_DOMAIN_VIDEO_TYPE_CIRRUS;
int nvirtiodisk = 0;
qemuDomainCmdlineDefPtr cmd = NULL;
+virDomainDiskDefPtr disk = NULL;
if (pidfile)
*pidfile = NULL;
@@ -6124,7 +6125,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
STRPREFIX(arg, -fd) ||
STREQ(arg, -cdrom)) {
WANT_VALUE();
-virDomainDiskDefPtr disk;
if (VIR_ALLOC(disk) 0)
goto no_memory;
@@ -6239,6 +6239,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
goto no_memory;
}
def-disks[def-ndisks++] = disk;
+disk = NULL;
} else if (STREQ(arg, -no-acpi)) {
def-features= ~(1 VIR_DOMAIN_FEATURE_ACPI);
} else if (STREQ(arg, -no-reboot)) {
@@ -6307,8 +6308,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
if (!(chr = virDomainChrDefNew()))
goto error;
-if (qemuParseCommandLineChr(chr-source, val) 0)
+if (qemuParseCommandLineChr(chr-source, val) 0) {
+virDomainChrDefFree(chr);
goto error;
+}
if (VIR_REALLOC_N(def-serials, def-nserials+1) 0) {
virDomainChrDefFree(chr);
goto no_memory;
@@ -6325,8 +6328,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
if (!(chr = virDomainChrDefNew()))
goto error;
-if (qemuParseCommandLineChr(chr-source, val) 0)
+if (qemuParseCommandLineChr(chr-source, val) 0) {
+virDomainChrDefFree(chr);
goto error;
+}
if (VIR_REALLOC_N(def-parallels, def-nparallels+1) 0) {
virDomainChrDefFree(chr);
goto no_memory;
@@ -6353,7 +6358,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
}
def-inputs[def-ninputs++] = input;
} else if (STRPREFIX(val, disk:)) {
-virDomainDiskDefPtr disk;
if (VIR_ALLOC(disk) 0)
goto no_memory;
disk-src = strdup(val + strlen(disk:));
@@ -6376,6 +6380,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
goto no_memory;
}
def-disks[def-ndisks++] = disk;
+disk = NULL;
} else {
virDomainHostdevDefPtr hostdev;
if (!(hostdev = qemuParseCommandLineUSB(val)))
@@ -6399,7 +6404,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
def-nets[def-nnets++] = net;
}
} else if (STREQ(arg, -drive)) {
-virDomainDiskDefPtr disk;
WANT_VALUE();
if (!(disk = qemuParseCommandLineDisk(caps, val, nvirtiodisk)))
goto error;
@@ -6408,6 +6412,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
goto no_memory;
}
def-disks[def-ndisks++] = disk;
+disk = NULL;
if (disk-bus == VIR_DOMAIN_DISK_BUS_VIRTIO)
nvirtiodisk++;
@@ -6516,8 +6521,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
if (VIR_ALLOC(chr) 0)
goto no_memory;
-if (qemuParseCommandLineChr(chr, val) 0)
+if (qemuParseCommandLineChr(chr, val) 0) {
+virDomainChrSourceDefFree(chr);
goto error;
+}
*monConfig = chr;
}
@@ -6545,10 +6552,9 @@ virDomainDefPtr
Re: [libvirt] [PATCH 12/3] build: silence coverity false positives
On 08/02/2011 05:53 PM, Eric Blake wrote: Coverity complained that 395 out of 409 virAsprintf calls are checked, and therefore assumed that the remaining cases are bugs waiting to happen. But in each of these cases, a failed virAsprintf will properly set the target string to NULL, and pass on that failure to the caller, without wasting efforts to check the call. Adding the ignore_value silences Coverity. ACK. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 13/3] rpc: avoid null deref
On 08/02/2011 05:58 PM, Eric Blake wrote:
Detected by Coverity.
* src/rpc/virnetserverclient.c (virNetServerClientDispatchRead):
Avoid null deref on OOM.
---
src/rpc/virnetserverclient.c |5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index 2f6c040..e246fa5 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -786,9 +786,10 @@ readmore:
if (client-nrequests client-nrequests_max) {
if (!(client-rx = virNetMessageNew())) {
client-wantClose = true;
+} else {
+client-rx-bufferLength = VIR_NET_MESSAGE_LEN_MAX;
+client-nrequests++;
}
-client-rx-bufferLength = VIR_NET_MESSAGE_LEN_MAX;
-client-nrequests++;
}
virNetServerClientUpdateEvent(client);
}
ACK.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 9/3] qemu: avoid memory leaks
On 08/02/2011 03:50 PM, Eric Blake wrote:
On 08/02/2011 03:46 PM, Eric Blake wrote:
Quite a few leaks detected by coverity. For chr, the leaks were
close enough to the allocations to plug in place; for disk, the
leaks were separated from the allocation by enough other lines with
intermediate failure cases that I refactored the cleanup instead.
* src/qemu/qemu_command.c (qemuParseCommandLine): Plug leaks.
---
v2: address review comments from v1
squash this in too (serves me right for hitting send prior to 'make
check' completing).
Yet more to squash in.
diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c
index 067e53e..76df0aa 100644
--- i/src/qemu/qemu_command.c
+++ w/src/qemu/qemu_command.c
@@ -6226,18 +6226,14 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr
caps,
}
if (!(disk-src || disk-nhosts 0) ||
-!disk-dst) {
-virDomainDiskDefFree(disk);
+!disk-dst)
goto no_memory;
-}
if (virDomainDiskDefAssignAddress(caps, disk) 0)
goto error;
-if (VIR_REALLOC_N(def-disks, def-ndisks+1) 0) {
-virDomainDiskDefFree(disk);
+if (VIR_REALLOC_N(def-disks, def-ndisks+1) 0)
goto no_memory;
-}
def-disks[def-ndisks++] = disk;
disk = NULL;
} else if (STREQ(arg, -no-acpi)) {
@@ -6361,24 +6357,17 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr
caps,
if (VIR_ALLOC(disk) 0)
goto no_memory;
disk-src = strdup(val + strlen(disk:));
-if (!disk-src) {
-virDomainDiskDefFree(disk);
+if (!disk-src)
goto no_memory;
-}
if (STRPREFIX(disk-src, /dev/))
disk-type = VIR_DOMAIN_DISK_TYPE_BLOCK;
else
disk-type = VIR_DOMAIN_DISK_TYPE_FILE;
disk-device = VIR_DOMAIN_DISK_DEVICE_DISK;
disk-bus = VIR_DOMAIN_DISK_BUS_USB;
-if (!(disk-dst = strdup(sda))) {
-virDomainDiskDefFree(disk);
+if (!(disk-dst = strdup(sda)) ||
+VIR_REALLOC_N(def-disks, def-ndisks+1) 0)
goto no_memory;
-}
-if (VIR_REALLOC_N(def-disks, def-ndisks+1) 0) {
-virDomainDiskDefFree(disk);
-goto no_memory;
-}
def-disks[def-ndisks++] = disk;
disk = NULL;
} else {
@@ -6407,10 +6396,8 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
WANT_VALUE();
if (!(disk = qemuParseCommandLineDisk(caps, val,
nvirtiodisk)))
goto error;
-if (VIR_REALLOC_N(def-disks, def-ndisks+1) 0) {
-virDomainDiskDefFree(disk);
+if (VIR_REALLOC_N(def-disks, def-ndisks+1) 0)
goto no_memory;
-}
if (disk-bus == VIR_DOMAIN_DISK_BUS_VIRTIO)
nvirtiodisk++;
@@ -6682,7 +6669,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
no_memory:
virReportOOMError();
error:
-VIR_FREE(disk);
+virDomainDiskDefFree(disk);
VIR_FREE(cmd);
virDomainDefFree(def);
VIR_FREE(nics);
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv3 9/3] qemu: avoid memory leaks
Quite a few leaks detected by coverity. For chr, the leaks were
close enough to the allocations to plug in place; for disk, the
leaks were separated from the allocation by enough other lines with
intermediate failure cases that I refactored the cleanup instead.
* src/qemu/qemu_command.c (qemuParseCommandLine): Plug leaks.
---
v3: squash in more fixes: free 'disk' correctly, and only once; don't
reference disk after it is set to NULL
src/qemu/qemu_command.c | 56 +--
1 files changed, 25 insertions(+), 31 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 6a2e2ae..76df0aa 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5938,6 +5938,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
int video = VIR_DOMAIN_VIDEO_TYPE_CIRRUS;
int nvirtiodisk = 0;
qemuDomainCmdlineDefPtr cmd = NULL;
+virDomainDiskDefPtr disk = NULL;
if (pidfile)
*pidfile = NULL;
@@ -6124,7 +6125,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
STRPREFIX(arg, -fd) ||
STREQ(arg, -cdrom)) {
WANT_VALUE();
-virDomainDiskDefPtr disk;
if (VIR_ALLOC(disk) 0)
goto no_memory;
@@ -6226,19 +6226,16 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
}
if (!(disk-src || disk-nhosts 0) ||
-!disk-dst) {
-virDomainDiskDefFree(disk);
+!disk-dst)
goto no_memory;
-}
if (virDomainDiskDefAssignAddress(caps, disk) 0)
goto error;
-if (VIR_REALLOC_N(def-disks, def-ndisks+1) 0) {
-virDomainDiskDefFree(disk);
+if (VIR_REALLOC_N(def-disks, def-ndisks+1) 0)
goto no_memory;
-}
def-disks[def-ndisks++] = disk;
+disk = NULL;
} else if (STREQ(arg, -no-acpi)) {
def-features = ~(1 VIR_DOMAIN_FEATURE_ACPI);
} else if (STREQ(arg, -no-reboot)) {
@@ -6307,8 +6304,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
if (!(chr = virDomainChrDefNew()))
goto error;
-if (qemuParseCommandLineChr(chr-source, val) 0)
+if (qemuParseCommandLineChr(chr-source, val) 0) {
+virDomainChrDefFree(chr);
goto error;
+}
if (VIR_REALLOC_N(def-serials, def-nserials+1) 0) {
virDomainChrDefFree(chr);
goto no_memory;
@@ -6325,8 +6324,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
if (!(chr = virDomainChrDefNew()))
goto error;
-if (qemuParseCommandLineChr(chr-source, val) 0)
+if (qemuParseCommandLineChr(chr-source, val) 0) {
+virDomainChrDefFree(chr);
goto error;
+}
if (VIR_REALLOC_N(def-parallels, def-nparallels+1) 0) {
virDomainChrDefFree(chr);
goto no_memory;
@@ -6353,29 +6354,22 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
}
def-inputs[def-ninputs++] = input;
} else if (STRPREFIX(val, disk:)) {
-virDomainDiskDefPtr disk;
if (VIR_ALLOC(disk) 0)
goto no_memory;
disk-src = strdup(val + strlen(disk:));
-if (!disk-src) {
-virDomainDiskDefFree(disk);
+if (!disk-src)
goto no_memory;
-}
if (STRPREFIX(disk-src, /dev/))
disk-type = VIR_DOMAIN_DISK_TYPE_BLOCK;
else
disk-type = VIR_DOMAIN_DISK_TYPE_FILE;
disk-device = VIR_DOMAIN_DISK_DEVICE_DISK;
disk-bus = VIR_DOMAIN_DISK_BUS_USB;
-if (!(disk-dst = strdup(sda))) {
-virDomainDiskDefFree(disk);
-goto no_memory;
-}
-if (VIR_REALLOC_N(def-disks, def-ndisks+1) 0) {
-virDomainDiskDefFree(disk);
+if (!(disk-dst = strdup(sda)) ||
+VIR_REALLOC_N(def-disks, def-ndisks+1) 0)
goto no_memory;
-}
def-disks[def-ndisks++] = disk;
+disk = NULL;
} else {
virDomainHostdevDefPtr hostdev;
if (!(hostdev = qemuParseCommandLineUSB(val)))
@@ -6399,18 +6393,16 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
def-nets[def-nnets++] = net;
}
} else if (STREQ(arg, -drive)) {
-virDomainDiskDefPtr disk;
WANT_VALUE();
if
Re: [libvirt] [PATCH 14/3] rpc: silence coverity false positives
On 08/02/2011 06:04 PM, Eric Blake wrote: In virNetServerNew, Coverity didn't realize that srv-mdsnGroupName can only be non-NULL if mdsnGroupName was non-NULL. In virNetServerRun, Coverity didn't realize that the array is non-NULL if the array count is non-zero. ACK. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 15/3] interface: drop dead code
On 08/02/2011 06:09 PM, Eric Blake wrote: Coverity detected that ifaceGetNthParent had already dereferenced 'nth' prior to the conditional; all callers already complied with passing a non-NULL pointer so make this part of the contract. ACK. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv3 9/3] qemu: avoid memory leaks
On 08/02/2011 06:34 PM, Eric Blake wrote: Quite a few leaks detected by coverity. For chr, the leaks were close enough to the allocations to plug in place; for disk, the leaks were separated from the allocation by enough other lines with intermediate failure cases that I refactored the cleanup instead. Okay, this time I think it's done. ACK. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 16/3] util: plug memory leak
On 08/02/2011 06:27 PM, Eric Blake wrote: Leak detected by Coverity; only possible on unlikely ptsname_r failure. Additionally, the man page for ptsname_r states that failure is merely non-zero, not necessarily -1. Double ACK. Two bugs in one! -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 17/3] qemu: silence coverity false positives
Coverity gets confused by our logic. Add some hints to silence
false positives.
* src/qemu/qemu_driver.c (qemudDomainGetVcpuPinInfo): Add hint.
(qemuDomainGetMemoryParameters): Likewise.
---
src/qemu/qemu_driver.c |6 ++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2044e23..ce19be7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3432,6 +3432,9 @@ qemudDomainGetVcpuPinInfo(virDomainPtr dom,
goto cleanup;
}
+/* Coverity didn't realize that targetDef must be set if we got here. */
+sa_assert(targetDef);
+
if (nodeGetInfo(dom-conn, nodeinfo) 0)
goto cleanup;
hostcpus = VIR_NODEINFO_MAXCPUS(nodeinfo);
@@ -6139,6 +6142,9 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom,
param-value.ul = 0;
param-type = VIR_TYPED_PARAM_ULLONG;
+/* Coverity does not realize that if we get here, group is set. */
+sa_assert(group);
+
switch (i) {
case 0: /* fill memory hard limit here */
rc = virCgroupGetMemoryHardLimit(group, val);
--
1.7.4.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 17/3] qemu: silence coverity false positives
On 08/02/2011 06:47 PM, Eric Blake wrote: Coverity gets confused by our logic. Add some hints to silence false positives. ACK. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 17/3] qemu: silence coverity false positives
On 08/02/2011 04:52 PM, Laine Stump wrote: On 08/02/2011 06:47 PM, Eric Blake wrote: Coverity gets confused by our logic. Add some hints to silence false positives. ACK. I've pushed through 17, and run out of time for any more today. Hopefully I picked out the juicy bugs, as evidenced by the fact that my later reports were merely silencing false positives. Any further analysis on my part will be post-0.9.4. Thanks for the speedy reviews today (it helps that the patches were mostly small). -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 17/3] qemu: silence coverity false positives
On Tue, Aug 02, 2011 at 05:00:19PM -0600, Eric Blake wrote: On 08/02/2011 04:52 PM, Laine Stump wrote: On 08/02/2011 06:47 PM, Eric Blake wrote: Coverity gets confused by our logic. Add some hints to silence false positives. ACK. I've pushed through 17, and run out of time for any more today. Hopefully I picked out the juicy bugs, as evidenced by the fact that my later reports were merely silencing false positives. Any further analysis on my part will be post-0.9.4. Thanks for the speedy reviews today (it helps that the patches were mostly small). Thanks a lot for doing this Eric, this definitely squashed a couple of real but yet undetected bug waiting for 0.9.4 !!! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Release of libvirt-0.9.4
With an extra day of delay but allowing to pick all coverity checks related patches from Eric, the release is out, I hope it's solid :-) available as usual at: ftp://libvirt.org/libvirt/libvirt-0.9.4.tar.gz along with signed RPMs This is again a fairly big release with more than 400 commits included, both new API, improvement and a large amount of fixes: Features: - bandwidth QoS control (Michal Privoznik) - Add new API virDomainBlockPull* (Adam Litke) - save: new API to manipulate save file images (Eric Blake) - CPU bandwidth limits support (Wen Congyang) - allow to send NMI and key event to guests (Lai Jiangshan) - new API virDomainUndefineFlags (Osier Yang) - Implement code to attach to external QEMU instances. (Daniel P. Berrange) - various missing python binding (Hu Tao and Lai Jiangshan) - bios: Add support for SGA (Michal Privoznik) Documentation: - doc: fix confusing statement about required privileges (Eric Blake) - doc: fix incorrect option in blockjob (Alex Jia) - Correct the default value of lock_manager in qemu.conf (Guannan Ren) - libvirt.c: Update outdated description of flags (Michal Privoznik) - qemu: Improve docs for virsh dump format (Osier Yang) - qemu: improve thread documentation (Eric Blake) - doc: Add doc for blockpull and blockjob commands (Osier Yang) - Fix incorrect implication about list options (Dave Allan) - Fix typos in virsh.pod file (Daniel P. Berrange) - network: Fix typo (Osier Yang) - Break up 'Basic Resources' XML section (Cole Robinson) - driver.h: Fix two driver documentation mistakes (Wieland Hoffmann) - doc: Add documentation for new cputune elements period and quota (Wen Congyang) - doc: Correct documents for iface commands (Osier Yang) - improve virsh man page synopses (Eric Blake) - Fix spice documentation typo (Michal Privoznik) - document dxml argument to migrate2 (Eric Blake) - website: Point main page links to libvirt driver pages (Dave Allan) - maint: fix typos (Eric Blake) - mention EMOTIVE as a libvirt-using app (Eric Blake) - virsh: Update virsh man page (Supriya Kannery) - Fix virsh inject-nmi man page (KAMEZAWA Hiroyuki) - virsh: make destroy sound less scary (Eric Blake) - minor whitespace cleanups (Eric Blake) - Add documentation for the seclabel XML element (Daniel P. Berrange) Portability: - build: fix include path for cygwin (Eric Blake) - build: avoid non-portable shell in test setup (Eric Blake) - tests: Don't use bash if we don't have to (Matthias Bolte) - freebsd: Fix build problem due to picking up the wrong libvirt.h (Matthias Bolte) - freebsd: Avoid /bin/true in commandtest (Matthias Bolte) - freebsd: Add gnulib environ module for the commandtest (Matthias Bolte) - build: support warnings on RHEL 5 (Eric Blake) - Build: fix build if HAVE_AVAHI is not defined (Stefan Berger) - sysinfo: Don't try to run dmidecode on archs missing it (Michal Privoznik) - udev: Don't try to dump DMI on non-intel archs (Michal Privoznik) - Fix build when using polkit0 (Jim Fehlig) - Fix rpm build with sanlock and without QEmu (Daniel Veillard) - Skip some xen tests if xend is not running (Jim Fehlig) - build: fix virBufferVasprintf on mingw (Eric Blake) - Fix compilation of statstest.c during make check (Jim Fehlig) - Fix compilation error when SASL support is disabled (Jean-Baptiste Rouault) - tests: Disable networkxml2argvtest when configured without network (Matthias Bolte) Bug Fixes: - util: plug memory leak (Eric Blake) - rpc: avoid null deref (Eric Blake) - qemu: avoid memory leaks (Eric Blake) - qemu: plug child process leak on domain core dump (Eric Blake) - conf: avoid memory leak on disk operations (Eric Blake) - rpc: avoid crash on error (Eric Blake) - qemu: avoid null deref on block pull error (Eric Blake) - rpc: correctly process sasl whitelist globs (Eric Blake) - screenshot: don't unlink bogus file (Eric Blake) - python: avoid unlikely sign extension bug (Eric Blake) - rpc: avoid uninitialized memory use (Eric Blake) - rpc: avoid double close on error (Eric Blake) - qemu: Unlink temporary file on failure (Michal Privoznik) - rpc: avoid libvirtd crash on unexpected client close (Eric Blake) - virsh: fix memory leak in cmdNetworkInfo (Alex Jia) - openvz: detect when a domain was shut down from the inside (Jean-Baptiste Rouault) - qemu: fix crash when mixing sync and async monitor jobs (Eric Blake) - qemu: fix return value issue (Alex Jia) - qemu: fix return value issue in qemuDomainSetMemoryParameters (Alex Jia) - virsh: Fix vol-name and vol-pool commands (Matthias Bolte) - qemu: Fix a regression of domjobabort (Osier Yang) - conf: Don't leak the virtual port profile in virNetworkDefFree (Matthias Bolte) - network: don't forward DNS requests from isolated networks (Laine Stump) - virsh: avoid missing zero value judgement in cmdBlkiotune (Alex Jia) - qemu: fix nested job with driver lock held (Eric Blake) - qemu: Fix memory leak on metadata fetching (Michal Privoznik) - virsh: fix memory leak in cmdVolPath code (Alex Jia) - rpc: Fix
[libvirt] qemu-kvm=0.14 is unable to boot
Hello, I'm wondering what might be behind qemu-kvm=0.14 is unable to boot except from PXE and perhaps virtio HDD. I mean, is somebody running qemu-kvm=0.14 and libvirt-0.9.3/0.9.4 around here, thus is my setup broken? Thanks, Zdenek -- Zdenek Styblik email: sty...@turnovfree.net jabber: sty...@jabber.turnovfree.net -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Release of libvirt-0.9.4
With an extra day of delay but allowing to pick all coverity checks related patches from Eric, the release is out, I hope it's solid :-) available as usual at: ftp://libvirt.org/libvirt/libvirt-0.9.4.tar.gz along with signed RPMs This is again a fairly big release with more than 400 commits included, both new API, improvement and a large amount of fixes: Thanks for your help during the building process. Here is a patch for the update for FreeBSD. http://www.freebsd.org/cgi/query-pr.cgi?pr=159403 -jgh -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
