[libvirt] [PATCH] qemu_monitor_json.c: avoid an unconditional leak
Calling virJSONValueFree does not free the memory allocated via this: cmd = qemuMonitorJSONMakeCommand(drive_add, s:pci_addr, dev, s:opts, drivestr, NULL); so I added the VIR_FREE: From 1d272e070b66fc16e2fcc23002879ee268a58a5f Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Wed, 27 Jan 2010 09:58:12 +0100 Subject: [PATCH] qemu_monitor_json.c: avoid an unconditional leak * src/qemu/qemu_monitor_json.c (qemuMonitorJSONAttachDrive): Don't leak the buffer behind a virJSONValuePtr. --- src/qemu/qemu_monitor_json.c |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 8e88c7e..0f970bb 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1773,6 +1773,7 @@ int qemuMonitorJSONAttachDrive(qemuMonitorPtr mon, ret = -1; virJSONValueFree(cmd); +VIR_FREE(cmd); virJSONValueFree(reply); return ret; } -- 1.7.0.rc0.140.gfbe7 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu_monitor_json.c: avoid many unconditional leaks
Actually, the preceding patch fixed only the one leak that had been introduced in the last month or so. Looking at the many other functions that do the same sort of thing (call qemuMonitorJSONMakeCommand, and later virJSONValueFree), I saw that they all had exactly the same leak. So this amended patch fixes all of them: From 28f820354dcae9950cad042ea78a893fd9475830 Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Wed, 27 Jan 2010 09:58:12 +0100 Subject: [PATCH] qemu_monitor_json.c: avoid many unconditional leaks * src/qemu/qemu_monitor_json.c (qemuMonitorJSONAttachDrive): Don't leak the buffer behind a virJSONValuePtr. (qemuMonitorJSONStartCPUs): Likewise. (qemuMonitorJSONStopCPUs, qemuMonitorJSONSystemPowerdown): Likewise. (qemuMonitorJSONGetCPUInfo, qemuMonitorJSONGetBalloonInfo): Likewise. (qemuMonitorJSONGetBlockStatsInfo): Likewise. (qemuMonitorJSONSetVNCPassword): Likewise. (qemuMonitorJSONSetBalloon, qemuMonitorJSONEjectMedia): Likewise. (qemuMonitorJSONChangeMedia, qemuMonitorJSONSaveMemory): Likewise. (qemuMonitorJSONSetMigrationSpeed): Likewise. (qemuMonitorJSONGetMigrationStatus, qemuMonitorJSONMigrate): Likewise. (qemuMonitorJSONMigrateCancel, qemuMonitorJSONAddUSB): Likewise. (qemuMonitorJSONAddPCIHostDevice, qemuMonitorJSONAddPCIDisk): Likewise. (qemuMonitorJSONAddPCINetwork, qemuMonitorJSONRemovePCIDevice): Likewise. (qemuMonitorJSONSendFileHandle): Likewise. (qemuMonitorJSONCloseFileHandle): Likewise. (qemuMonitorJSONAddHostNetwork): Likewise. (qemuMonitorJSONRemoveHostNetwork): Likewise. (qemuMonitorJSONGetPtyPaths): Likewise. (qemuMonitorJSONAttachPCIDiskController): Likewise. --- src/qemu/qemu_monitor_json.c | 27 +++ 1 files changed, 27 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 8e88c7e..b6c3449 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -492,6 +492,7 @@ qemuMonitorJSONStartCPUs(qemuMonitorPtr mon, ret = qemuMonitorJSONCheckError(cmd, reply); virJSONValueFree(cmd); +VIR_FREE(cmd); virJSONValueFree(reply); return ret; } @@ -512,6 +513,7 @@ qemuMonitorJSONStopCPUs(qemuMonitorPtr mon) ret = qemuMonitorJSONCheckError(cmd, reply); virJSONValueFree(cmd); +VIR_FREE(cmd); virJSONValueFree(reply); return ret; } @@ -531,6 +533,7 @@ int qemuMonitorJSONSystemPowerdown(qemuMonitorPtr mon) ret = qemuMonitorJSONCheckError(cmd, reply); virJSONValueFree(cmd); +VIR_FREE(cmd); virJSONValueFree(reply); return ret; } @@ -637,6 +640,7 @@ int qemuMonitorJSONGetCPUInfo(qemuMonitorPtr mon, ret = qemuMonitorJSONExtractCPUInfo(reply, pids); virJSONValueFree(cmd); +VIR_FREE(cmd); virJSONValueFree(reply); return ret; } @@ -696,6 +700,7 @@ int qemuMonitorJSONGetBalloonInfo(qemuMonitorPtr mon, cleanup: virJSONValueFree(cmd); +VIR_FREE(cmd); virJSONValueFree(reply); return ret; } @@ -800,6 +805,7 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon, cleanup: virJSONValueFree(cmd); +VIR_FREE(cmd); virJSONValueFree(reply); return ret; } @@ -824,6 +830,7 @@ int qemuMonitorJSONSetVNCPassword(qemuMonitorPtr mon, ret = qemuMonitorJSONCheckError(cmd, reply); virJSONValueFree(cmd); +VIR_FREE(cmd); virJSONValueFree(reply); return ret; } @@ -861,6 +868,7 @@ int qemuMonitorJSONSetBalloon(qemuMonitorPtr mon, cleanup: virJSONValueFree(cmd); +VIR_FREE(cmd); virJSONValueFree(reply); return ret; } @@ -884,6 +892,7 @@ int qemuMonitorJSONEjectMedia(qemuMonitorPtr mon, ret = qemuMonitorJSONCheckError(cmd, reply); virJSONValueFree(cmd); +VIR_FREE(cmd); virJSONValueFree(reply); return ret; } @@ -918,6 +927,7 @@ int qemuMonitorJSONChangeMedia(qemuMonitorPtr mon, ret = qemuMonitorJSONCheckError(cmd, reply); virJSONValueFree(cmd); +VIR_FREE(cmd); virJSONValueFree(reply); return ret; } @@ -945,6 +955,7 @@ static int qemuMonitorJSONSaveMemory(qemuMonitorPtr mon, ret = qemuMonitorJSONCheckError(cmd, reply); virJSONValueFree(cmd); +VIR_FREE(cmd); virJSONValueFree(reply); return ret; } @@ -991,6 +1002,7 @@ int qemuMonitorJSONSetMigrationSpeed(qemuMonitorPtr mon, ret = qemuMonitorJSONCheckError(cmd, reply); virJSONValueFree(cmd); +VIR_FREE(cmd); virJSONValueFree(reply); return ret; } @@ -1084,6 +1096,7 @@ int qemuMonitorJSONGetMigrationStatus(qemuMonitorPtr mon, ret = -1; virJSONValueFree(cmd); +VIR_FREE(cmd); virJSONValueFree(reply); return ret; } @@ -1109,6 +1122,7 @@ static int qemuMonitorJSONMigrate(qemuMonitorPtr mon, ret = qemuMonitorJSONCheckError(cmd, reply); virJSONValueFree(cmd); +VIR_FREE(cmd); virJSONValueFree(reply); return ret; } @@ -1206,6 +1220,7 @@ int
Re: [libvirt] [PATCH] qemu_monitor_json.c: avoid many unconditional leaks
On Wed, Jan 27, 2010 at 10:38:55AM +0100, Jim Meyering wrote: Actually, the preceding patch fixed only the one leak that had been introduced in the last month or so. Looking at the many other functions that do the same sort of thing (call qemuMonitorJSONMakeCommand, and later virJSONValueFree), I saw that they all had exactly the same leak. So this amended patch fixes all of them: From 28f820354dcae9950cad042ea78a893fd9475830 Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Wed, 27 Jan 2010 09:58:12 +0100 Subject: [PATCH] qemu_monitor_json.c: avoid many unconditional leaks The real bug is the virJSONValueFree() itself which is missing the final VIR_FREE(value) call. By doing the free in the caller, we still leak data for compound array/hash values. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu_monitor_json.c: avoid many unconditional leaks
Daniel P. Berrange wrote: On Wed, Jan 27, 2010 at 10:38:55AM +0100, Jim Meyering wrote: Actually, the preceding patch fixed only the one leak that had been introduced in the last month or so. Looking at the many other functions that do the same sort of thing (call qemuMonitorJSONMakeCommand, and later virJSONValueFree), I saw that they all had exactly the same leak. So this amended patch fixes all of them: From 28f820354dcae9950cad042ea78a893fd9475830 Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Wed, 27 Jan 2010 09:58:12 +0100 Subject: [PATCH] qemu_monitor_json.c: avoid many unconditional leaks The real bug is the virJSONValueFree() itself which is missing the final VIR_FREE(value) call. By doing the free in the caller, we still leak data for compound array/hash values. Putting the VIR_FREE in virJSONValueFree was my first reflex, too, but since coverity detected no leak for the adjacent virJSONValueFree(reply); use, I assumed that doing so would cause a problem: virJSONValueFree(cmd); VIR_FREE(cmd); virJSONValueFree(reply); -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu_monitor_json.c: avoid many unconditional leaks
On Wed, Jan 27, 2010 at 11:17:32AM +0100, Jim Meyering wrote: Daniel P. Berrange wrote: On Wed, Jan 27, 2010 at 10:38:55AM +0100, Jim Meyering wrote: Actually, the preceding patch fixed only the one leak that had been introduced in the last month or so. Looking at the many other functions that do the same sort of thing (call qemuMonitorJSONMakeCommand, and later virJSONValueFree), I saw that they all had exactly the same leak. So this amended patch fixes all of them: From 28f820354dcae9950cad042ea78a893fd9475830 Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Wed, 27 Jan 2010 09:58:12 +0100 Subject: [PATCH] qemu_monitor_json.c: avoid many unconditional leaks The real bug is the virJSONValueFree() itself which is missing the final VIR_FREE(value) call. By doing the free in the caller, we still leak data for compound array/hash values. Putting the VIR_FREE in virJSONValueFree was my first reflex, too, but since coverity detected no leak for the adjacent virJSONValueFree(reply); use, I assumed that doing so would cause a problem: virJSONValueFree(cmd); VIR_FREE(cmd); virJSONValueFree(reply); I think coverity must be confused then :-) The virJSONValueFree() method is definitely intended to free any memory allocated during one of the virJSONValueNew() methods. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu_monitor_json.c: avoid many unconditional leaks
Daniel P. Berrange wrote: On Wed, Jan 27, 2010 at 11:17:32AM +0100, Jim Meyering wrote: Daniel P. Berrange wrote: On Wed, Jan 27, 2010 at 10:38:55AM +0100, Jim Meyering wrote: Actually, the preceding patch fixed only the one leak that had been introduced in the last month or so. Looking at the many other functions that do the same sort of thing (call qemuMonitorJSONMakeCommand, and later virJSONValueFree), I saw that they all had exactly the same leak. So this amended patch fixes all of them: From 28f820354dcae9950cad042ea78a893fd9475830 Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Wed, 27 Jan 2010 09:58:12 +0100 Subject: [PATCH] qemu_monitor_json.c: avoid many unconditional leaks The real bug is the virJSONValueFree() itself which is missing the final VIR_FREE(value) call. By doing the free in the caller, we still leak data for compound array/hash values. Putting the VIR_FREE in virJSONValueFree was my first reflex, too, but since coverity detected no leak for the adjacent virJSONValueFree(reply); use, I assumed that doing so would cause a problem: virJSONValueFree(cmd); VIR_FREE(cmd); virJSONValueFree(reply); I think coverity must be confused then :-) The virJSONValueFree() method is definitely intended to free any memory allocated during one of the virJSONValueNew() methods. :-) In that case, here's a replacement patch: From 96196ca0b0cd71ae6b7f2dd7668432db95678e70 Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Wed, 27 Jan 2010 09:58:12 +0100 Subject: [PATCH] json.c: avoid an unconditional leak from most qemuMonitorJSON* functions * src/util/json.c (virJSONValueFree): Free the value pointer, too. --- src/util/json.c |5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/util/json.c b/src/util/json.c index a292e1b..1b3c359 100644 --- a/src/util/json.c +++ b/src/util/json.c @@ -2,7 +2,7 @@ * json.c: JSON object parsing/formatting * * Copyright (C) 2009 Daniel P. Berrange - * Copyright (C) 2009 Red Hat, Inc. + * Copyright (C) 2009-2010 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -82,8 +82,9 @@ void virJSONValueFree(virJSONValuePtr value) case VIR_JSON_TYPE_NUMBER: VIR_FREE(value-data.number); break; - } + +VIR_FREE(value); } -- 1.7.0.rc0.140.gfbe7 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] support XEN_SYSCTL_INTERFACE_VERSION
On Tue, Jan 26, 2010 at 11:34:28PM -0700, Jim Fehlig wrote:
xen-unstable c/s 20762 bumped XEN_SYSCTL_INTERFACE_VERSION to 7. I
don't see how the interface change affects libvirt, other than failing
xenHypervisorInit() since version 7 is not tried.
The attached patch accommodates the upcoming Xen 4.0 release by checking
for XEN_SYSCTL_INTERFACE_VERSION 7. If found, it sets
XEN_DOMCTL_INTERFACE_VERSION to 6, which is also new to Xen 4.0.
Regards,
Jim
Index: libvirt-0.7.4/src/xen/xen_hypervisor.c
===
--- libvirt-0.7.4.orig/src/xen/xen_hypervisor.c
+++ libvirt-0.7.4/src/xen/xen_hypervisor.c
@@ -2100,12 +2100,14 @@ xenHypervisorInit(void)
DEBUG0(Using hypervisor call v2, sys ver6 dom ver5\n);
goto done;
}
-/* Xen 4.0 */
+}
+
+/* Xen 4.0 */
+sys_interface_version = 7; /* XEN_SYSCTL_INTERFACE_VERSION */
+if (virXen_getdomaininfo(fd, 0, info) == 1) {
dom_interface_version = 6; /* XEN_DOMCTL_INTERFACE_VERSION */
-if (virXen_getvcpusinfo(fd, 0, 0, ipt, NULL, 0) == 0){
-DEBUG0(Using hypervisor call v2, sys ver6 dom ver6\n);
-goto done;
-}
+DEBUG0(Using hypervisor call v2, sys ver7 dom ver6\n);
+goto done;
}
hypervisor_version = 1;
ACK
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] xen_hypervisor.c: avoid NULL deref for NULL domain argument
On Tue, Jan 26, 2010 at 08:24:25PM +0100, Jim Meyering wrote:
When domain is NULL, don't deref NULL. Instead, just return -1,
as in many other functions in this file, and as this function did
up until a month ago.
An alternative (taken 3 times in this file) is to do this:
virXenErrorFunc (NULL, VIR_ERR_INTERNAL_ERROR, __FUNCTION__,
domain or conn is NULL, 0);
return -1;
I could go either way.
From 177556167775b806a29bcb1af7ba4294d1909912 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 26 Jan 2010 20:17:07 +0100
Subject: [PATCH] xen_hypervisor.c: avoid NULL deref for NULL domain argument
* src/xen/xen_hypervisor.c (xenHypervisorGetVcpus): Don't attempt
to diagnose an unlikely NULL-domain or NULL-domain-conn error.
---
src/xen/xen_hypervisor.c |7 ++-
1 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c
index 6d8accc..0257be2 100644
--- a/src/xen/xen_hypervisor.c
+++ b/src/xen/xen_hypervisor.c
@@ -1,7 +1,7 @@
/*
* xen_internal.c: direct access to Xen hypervisor level
*
- * Copyright (C) 2005, 2006, 2007, 2008, 2009 Red Hat, Inc.
+ * Copyright (C) 2005-2010 Red Hat, Inc.
*
* See COPYING.LIB for the License of this software
*
@@ -3475,11 +3475,8 @@ xenHypervisorGetVcpus(virDomainPtr domain,
virVcpuInfoPtr info, int maxinfo,
virVcpuInfoPtr ipt;
int nbinfo, i;
-if (domain == NULL || domain-conn == NULL) {
-virXenErrorFunc (domain-conn, VIR_ERR_INVALID_ARG, __FUNCTION__,
-invalid argument, 0);
+if (domain == NULL || domain-conn == NULL)
return -1;
-}
I'd rather we just got rid of that check completely - its a left
over from a time when Xen was the only driver these entry points
needed to do full checking. Now all mandatory parameters are checked
in the previous libvirt.c layer.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] cpu_x86.c: avoid NULL-deref for invalid arguments
On Tue, Jan 26, 2010 at 08:06:03PM +0100, Jim Meyering wrote:
Passing a NULL models pointer along with a
contradictory nmodels = 1 would cause a NULL-dereference.
An alternative to the fix below would be simply to guard
the NULL-derferencing strcmp with if (models ...,
but that wouldn't tell the caller that they're passing
bogus arguments.
From f57bd1fbe7a41b1b9d8ba1be61790e95b5060ddc Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 26 Jan 2010 19:58:48 +0100
Subject: [PATCH] cpu_x86.c: avoid NULL-deref for invalid arguments
* src/cpu/cpu_x86.c (x86Decode): Do not dereference NULL
when models is NULL and nmodels is 1 or greater.
---
src/cpu/cpu_x86.c |5 -
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
index dae7c90..47dc400 100644
--- a/src/cpu/cpu_x86.c
+++ b/src/cpu/cpu_x86.c
@@ -1,7 +1,7 @@
/*
* cpu_x86.c: CPU driver for CPUs with x86 compatible CPUID instruction
*
- * Copyright (C) 2009 Red Hat, Inc.
+ * Copyright (C) 2009-2010 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -954,6 +954,9 @@ x86Decode(virCPUDefPtr cpu,
if (data == NULL || (map = x86LoadMap()) == NULL)
return -1;
+if (models == NULL nmodels != 0)
+return -1;
+
candidate = map-models;
while (candidate != NULL) {
bool allowed = (models == NULL);
ACK
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] portability to non-glibc: don't use realpath(..., NULL)
On Tue, Jan 26, 2010 at 05:48:59PM +0100, Jim Meyering wrote:
Using realpath like that is not portable, and providing
a buffer instead of NULL is wasteful and harder to code properly.
Instead, use gnulib's canonicalize_file_name, which does
the same job portably:
From 4afea6b59e2be6c28b45ef59a6a2f896eed44dba Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 26 Jan 2010 17:13:45 +0100
Subject: [PATCH] portability to non-glibc: don't use realpath(..., NULL)
it causes a NULL-dereference on some systems like Solaris 10.
* src/node_device/node_device_linux_sysfs.c. Include stdlib.h.
(get_sriov_function): Use canonicalize_filen_name, not realpath.
* bootstrap (modules): Add canonicalize-lgpl.
---
bootstrap |1 +
src/node_device/node_device_linux_sysfs.c |6 --
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/bootstrap b/bootstrap
index aec5d05..cc3c6ef 100755
--- a/bootstrap
+++ b/bootstrap
@@ -68,6 +68,7 @@ modules='
areadlink
base64
c-ctype
+canonicalize-lgpl
close
connect
getaddrinfo
diff --git a/src/node_device/node_device_linux_sysfs.c
b/src/node_device/node_device_linux_sysfs.c
index 33e658d..c1fce5d 100644
--- a/src/node_device/node_device_linux_sysfs.c
+++ b/src/node_device/node_device_linux_sysfs.c
@@ -24,6 +24,7 @@
#include fcntl.h
#include sys/stat.h
+#include stdlib.h
#include node_device_driver.h
#include node_device_hal.h
@@ -242,7 +243,8 @@ out:
static int get_sriov_function(const char *device_link,
struct pci_config_address **bdf)
{
-char *device_path = NULL, *config_address = NULL;
+char *config_address = NULL;
+char *device_path = NULL;
char errbuf[64];
int ret = SRIOV_ERROR;
@@ -259,7 +261,7 @@ static int get_sriov_function(const char *device_link,
}
-device_path = realpath(device_link, device_path);
+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,
ACK
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/9] Restrict virDomain{Attach, Detach}Device to active domains
On Mon, Jan 25, 2010 at 04:16:59PM -0700, Jim Fehlig wrote:
Daniel P. Berrange wrote:
On Thu, Jan 14, 2010 at 10:42:38AM -0700, Jim Fehlig wrote:
virDomain{Attach,Detach}Device is now only permitted on active
domains. Explicitly state this restriction in the API
documentation and enforce it in libvirt frontend.
---
src/libvirt.c | 14 --
1 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c
index d973907..1145561 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -5121,7 +5121,8 @@ error:
* @domain: pointer to domain object
* @xml: pointer to XML description of one device
*
- * Create a virtual device attachment to backend.
+ * Create a virtual device attachment to backend. This function,
+ * having hotplug semantics, is only allowed on an active domain.
*
* Returns 0 in case of success, -1 in case of failure.
*/
@@ -5142,6 +5143,10 @@ virDomainAttachDevice(virDomainPtr domain, const
char *xml)
virLibDomainError(domain, VIR_ERR_OPERATION_DENIED, __FUNCTION__);
goto error;
}
+if (!virDomainIsActive(domain)) {
+virLibDomainError(domain, VIR_ERR_OPERATION_INVALID,
__FUNCTION__);
+goto error;
+}
conn = domain-conn;
if (conn-driver-domainAttachDevice) {
@@ -5164,7 +5169,8 @@ error:
* @domain: pointer to domain object
* @xml: pointer to XML description of one device
*
- * Destroy a virtual device attachment to backend.
+ * Destroy a virtual device attachment to backend. This function,
+ * having hot-unplug semantics, is only allowed on an active domain.
*
* Returns 0 in case of success, -1 in case of failure.
*/
@@ -5185,6 +5191,10 @@ virDomainDetachDevice(virDomainPtr domain, const
char *xml)
virLibDomainError(domain, VIR_ERR_OPERATION_DENIED, __FUNCTION__);
goto error;
}
+if (!virDomainIsActive(domain)) {
+virLibDomainError(domain, VIR_ERR_OPERATION_INVALID,
__FUNCTION__);
+goto error;
+}
conn = domain-conn;
if (conn-driver-domainDetachDevice) {
--
Agreed with the added doc comments, but I think I prefer that we do the
check for active in the drivers themselves, at time of use. Doing the
check at the top level is open to race conditions, since no locks are
held on the objects in question at this point. Drivers will thus have
to do extra checks themselves, making this top level one redundant.
So I think we should just add the docs here.
Ok. Modified patch below.
Thanks,
Jim
commit d8ec244c6513b7c44956a547e56c228a4c38fbbe
Author: Jim Fehlig jfeh...@novell.com
Date: Wed Jan 13 18:24:51 2010 -0700
doc: restrict virDomain{Attach,Detach}Device to active domains
virDomain{Attach,Detach}Device is now only permitted on active
domains. Explicitly state this restriction in the API
documentation.
V2: Only change doc, dropping the hunk that forced the restriction
in libvirt frontend.
diff --git a/src/libvirt.c b/src/libvirt.c
index d973907..188b991 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -5121,7 +5121,8 @@ error:
* @domain: pointer to domain object
* @xml: pointer to XML description of one device
*
- * Create a virtual device attachment to backend.
+ * Create a virtual device attachment to backend. This function,
+ * having hotplug semantics, is only allowed on an active domain.
*
* Returns 0 in case of success, -1 in case of failure.
*/
@@ -5164,7 +5165,8 @@ error:
* @domain: pointer to domain object
* @xml: pointer to XML description of one device
*
- * Destroy a virtual device attachment to backend.
+ * Destroy a virtual device attachment to backend. This function,
+ * having hot-unplug semantics, is only allowed on an active domain.
*
* Returns 0 in case of success, -1 in case of failure.
*/
ACK
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 9/9] Modify virsh commands
On Mon, Jan 25, 2010 at 04:30:45PM -0700, Jim Fehlig wrote:
Daniel P. Berrange wrote:
On Thu, Jan 14, 2010 at 10:42:46AM -0700, Jim Fehlig wrote:
Change all virsh commands that invoke virDomain{Attach,Detach}Device()
to use virDomain{Attach,Detach}DeviceFlags() instead.
Add a --persistent flag to these virsh commands, allowing user to
specify that the domain persisted config be modified as well.
---
tools/virsh.c | 55
+--
1 files changed, 49 insertions(+), 6 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index 1fae5e6..a082b89 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -6285,6 +6285,7 @@ static const vshCmdInfo info_attach_device[] = {
static const vshCmdOptDef opts_attach_device[] = {
{domain, VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop(domain name, id
or uuid)},
{file, VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop(XML file)},
+{persistent, VSH_OT_BOOL, 0, gettext_noop(persist device
attachment)},
{NULL, 0, 0, NULL}
};
@@ -6296,6 +6297,7 @@ cmdAttachDevice(vshControl *ctl, const vshCmd *cmd)
char *buffer;
int ret = TRUE;
int found;
+int flags = VIR_DOMAIN_DEVICE_MODIFY_CURRENT;
if (!vshConnectionUsability(ctl, ctl-conn, TRUE))
return FALSE;
@@ -6309,13 +6311,18 @@ cmdAttachDevice(vshControl *ctl, const vshCmd *cmd)
virDomainFree(dom);
return FALSE;
}
+if (vshCommandOptBool(cmd, persistent))
+flags |= VIR_DOMAIN_DEVICE_MODIFY_CONFIG;
if (virFileReadAll(from, VIRSH_MAX_XML_FILE, buffer) 0) {
virDomainFree(dom);
return FALSE;
}
-ret = virDomainAttachDevice(dom, buffer);
+if (virDomainIsActive(dom))
+ flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE;
+
+ret = virDomainAttachDeviceFlags(dom, buffer, flags);
VIR_FREE(buffer);
if (ret 0) {
This has the same subtle compatability problem that the public API
entry point suffers from. New virsh won't be able to modify guests
from an existing libvirtd. I think that if flags == 0, then we should
use the existing API method, and only use the new virDomainAttachDeviceFlags
if flags != 0. I think we probably want to default to 0, and only set
the VIR_DOMAIN_DEVICE_MODIFY_LIVE flag if a '--live' flag is given.
Basically we need to try ensure compatability with existing operation
if at all possible
The existing behavior is essentially VIR_DOMAIN_DEVICE_MODIFY_LIVE.
qemu fails the operation if domain is inactive. Attach works on
inactive Xen domains, but detach does not. vbox has an impl for
inactive domains, but I haven't tested it.
I kept the existing behavior by only calling
vir{Attach,Detach}DeviceFlags() only when the new virsh flag
persistent is specified. Revised patch below.
Thanks,
Jim
commit 4e52e0d49d96958facab3e99b6b6f8519d2d9c76
Author: Jim Fehlig jfeh...@novell.com
Date: Wed Jan 13 18:54:58 2010 -0700
Modify virsh commands
Change all virsh commands that invoke virDomain{Attach,Detach}Device()
to use virDomain{Attach,Detach}DeviceFlags() instead.
Add a --persistent flag to these virsh commands, allowing user to
specify that the domain persisted config be modified as well.
V2: Only invoke virDomain{Attach,Detach}DeviceFlags() if
--persistent flag is specified. Otherwise invoke
virDomain{Attach,Detach}Device() to retain current behavior.
diff --git a/tools/virsh.c b/tools/virsh.c
index 1fae5e6..0763dcc 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -6285,6 +6285,7 @@ static const vshCmdInfo info_attach_device[] = {
static const vshCmdOptDef opts_attach_device[] = {
{domain, VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop(domain name, id or
uuid)},
{file, VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop(XML file)},
+{persistent, VSH_OT_BOOL, 0, gettext_noop(persist device
attachment)},
{NULL, 0, 0, NULL}
};
@@ -6296,6 +6297,7 @@ cmdAttachDevice(vshControl *ctl, const vshCmd *cmd)
char *buffer;
int ret = TRUE;
int found;
+unsigned int flags;
if (!vshConnectionUsability(ctl, ctl-conn, TRUE))
return FALSE;
@@ -6315,7 +6317,14 @@ cmdAttachDevice(vshControl *ctl, const vshCmd *cmd)
return FALSE;
}
-ret = virDomainAttachDevice(dom, buffer);
+if (vshCommandOptBool(cmd, persistent)) {
+flags = VIR_DOMAIN_DEVICE_MODIFY_CONFIG;
+if (virDomainIsActive(dom) == 1)
+ flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE;
+ret = virDomainAttachDeviceFlags(dom, buffer, flags);
+} else {
+ret = virDomainAttachDevice(dom, buffer);
+}
VIR_FREE(buffer);
if (ret 0) {
@@ -6343,6 +6352,7 @@ static const vshCmdInfo info_detach_device[] = {
static const vshCmdOptDef
Re: [libvirt] [PATCH 4/9] Public API Implementation
On Mon, Jan 25, 2010 at 04:22:12PM -0700, Jim Fehlig wrote:
Daniel P. Berrange wrote:
This looks safe, but there's a subtle problem with changing the existing
virDomainAttachDevice() entry point to call virDomainAttachDeviceFlags().
It will break compatability with old libvirtd, even though the old
libvirtd supports the virDomainAttachDevice() code.
Ah, yes - good catch. Thanks.
So we need to keep
the distinct paths in the public API driver definitions. The eventual
low level hypervisor drivers can of course just turn their existing
impl into a thin wrapper to the new method..
There's one other option actually - we could put compatability code in
the remote driver client instead. Either, make it always invoke the old
RPC call if flags == VIR_DOMAIN_DEVICE_MODIFY_LIVE, or have it invoke
the new RPC call fallback to the old RPC call if it gets an error
indicating the new one doesn't exist.
Do you prefer the latter option? After a quick look, I didn't spot any
existing compatibility code in the remote driver client. The first
option might be slightly better wrt maintenance.
Yes, the first option is certainly simpler / clearer code todo, so lets
just do that first. We can easily revisit adding compat code in the
remote driver client at a later date if we find it to be neccessary.
Revised patch below.
Thanks,
Jim
commit 487b2434403d520027957ed623354b398984af31
Author: Jim Fehlig jfeh...@novell.com
Date: Wed Jan 13 18:34:23 2010 -0700
Public API Implementation
Implementation of public API for virDomain{Attach,Detach}DeviceFlags.
V2: Don't break remote compatibility with older libvirtd
diff --git a/src/libvirt.c b/src/libvirt.c
index 188b991..de802c4 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -5146,14 +5146,68 @@ virDomainAttachDevice(virDomainPtr domain, const char
*xml)
conn = domain-conn;
if (conn-driver-domainAttachDevice) {
+ int ret;
+ ret = conn-driver-domainAttachDevice (domain, xml);
+ if (ret 0)
+ goto error;
+ return ret;
+}
+
+virLibConnError (conn, VIR_ERR_NO_SUPPORT, __FUNCTION__);
+
+error:
+virDispatchError(domain-conn);
+return -1;
+}
+
+/**
+ * virDomainAttachDeviceFlags:
+ * @domain: pointer to domain object
+ * @xml: pointer to XML description of one device
+ * @flags: an OR'ed set of virDomainDeviceModifyFlags
+ *
+ * Attach a virtual device to a domain, using the flags parameter
+ * to control how the device is attached. VIR_DOMAIN_DEVICE_MODIFY_CURRENT
+ * specifies that the device allocation is made based on current domain
+ * state. VIR_DOMAIN_DEVICE_MODIFY_LIVE specifies that the device shall be
+ * allocated to the active domain instance only and is not added to the
+ * persisted domain configuration. VIR_DOMAIN_DEVICE_MODIFY_CONFIG
+ * specifies that the device shall be allocated to the persisted domain
+ * configuration only. Note that the target hypervisor must return an
+ * error if unable to satisfy flags. E.g. the hypervisor driver will
+ * return failure if LIVE is specified but it only supports modifying the
+ * persisted device allocation.
+ *
+ * Returns 0 in case of success, -1 in case of failure.
+ */
+int
+virDomainAttachDeviceFlags(virDomainPtr domain,
+ const char *xml, unsigned int flags)
+{
+virConnectPtr conn;
+DEBUG(domain=%p, xml=%s, flags=%d, domain, xml, flags);
+
+virResetLastError();
+
+if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
+virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
+return (-1);
+}
+if (domain-conn-flags VIR_CONNECT_RO) {
+virLibDomainError(domain, VIR_ERR_OPERATION_DENIED, __FUNCTION__);
+goto error;
+}
+conn = domain-conn;
+
+if (conn-driver-domainAttachDeviceFlags) {
int ret;
-ret = conn-driver-domainAttachDevice (domain, xml);
+ret = conn-driver-domainAttachDeviceFlags(domain, xml, flags);
if (ret 0)
goto error;
return ret;
}
-virLibConnError (conn, VIR_ERR_NO_SUPPORT, __FUNCTION__);
+virLibConnError(conn, VIR_ERR_NO_SUPPORT, __FUNCTION__);
error:
virDispatchError(domain-conn);
@@ -5192,12 +5246,66 @@ virDomainDetachDevice(virDomainPtr domain, const char
*xml)
if (conn-driver-domainDetachDevice) {
int ret;
ret = conn-driver-domainDetachDevice (domain, xml);
+ if (ret 0)
+ goto error;
+ return ret;
+ }
+
+virLibConnError (conn, VIR_ERR_NO_SUPPORT, __FUNCTION__);
+
+error:
+virDispatchError(domain-conn);
+return -1;
+}
+
+/**
+ * virDomainDetachDeviceFlags:
+ * @domain: pointer to domain object
+ * @xml: pointer to XML description of one device
+ * @flags: an OR'ed set of
[libvirt] [PATCH] Add missing sata controller type to domain.rng
* docs/schemas/domain.rng: Add sata controller type --- docs/schemas/domain.rng |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng index 49b57eb..827ff6f 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domain.rng @@ -529,6 +529,7 @@ valuefdc/value valueide/value valuescsi/value +valuesata/value /choice /attribute /optional -- 1.6.6 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC] [Patch] Support for Linux macvtap device
On Tue, Jan 26, 2010 at 05:22:05PM -0500, Stefan Berger wrote: Daniel P. Berrange berra...@redhat.com wrote on 01/26/2010 04:21:56 libvir-list, gerhard.stenzel, Vivek Kashyap, arndb Please respond to Daniel P. Berrange On Mon, Jan 25, 2010 at 12:47:17PM -0500, Stefan Berger wrote: Hello! The attached patch provides support for the Linux macvtap device for Qemu by passing a file descriptor to Qemu command line similar to how it is done with a regular tap device. I have modified the network XML code to understand a definition as the following one here: network namevepanet/name uuid4ebd5168-6321-4757-8397-f6e83484f402/uuid extbridge mode='vepa' dev='eth0'/ /network I don't think this is the correct place to be adding this kind of configuration / functionality. The virNetworkPtr / network XML is describing a virtual network capability which is *not* directly connected to the LAN. It may be configured to route from the virtual network to the LAN, with optional NAT applied. So while the implementation may use a bridge device, this bridge is not connected to any physical device. Since VEPA is about directly connecting VMs to the LAN, this doesn't really fit here. Yes, I have re-purposed the network XML to describe an external bride. There's the following advantage to this: - you can migrate a VM between machines that have different types of connectivity, i.e, tap and macvtap - pushing the eth0 into referenced XML makes it independent of the local configuration of the host, i.e, on the one host it may be eth0 and on the other eth1. eth0 in the above XML could be a physical adapter, or an SR-IOV physical adapter or virtual function of an SR-IOV adapter. I agree that those are both good advantages, but I'm still not liking the idea of re-purposing the network XML model for this. Unfortunately I don't yet have a clear alternative that satisfies those goals. I rather regret that the current stuff uses the name 'network' since it is somewhat misleading as to its purpose :-) The best idea I can come up with so far is to imagine a new switch object which would basically use the syntax you are suggesting as extension for the 'network object, but without all the existing bits todo with NAT/routing/DHCP. A 'switch' object might be something that is also useful for the parallel work being done in firewall filters in libvirt. I don't think we neccessarily need to consider this mutually exclusive wrt the direct syntax I suggest for VMs. We could start with the direct syntax in VMs since that's pretty quick easy to implement, and then introduce the idea of a 'switch' object later to give us an alternate host-independant config. In the context of bridging a guest to a plain ethernet device, these fit together as follows 1. The virNodeDevPtr APIs are used to discover what physical network devices exist, 'eth0' 2. The virInterfacePtr APIs are used to create a bridge on the host br0, containing the physical device 'eth0' Yes, I suppose this is all done via 'virsh iface-*' commands. Yes, that's correct. So unless I'm missing something major in my reasoning here I think in the domain XML we end up with two possible configs for guest network interfaces 1. The current one using plain Linux software bridging, which we can't change in an incompatible way interface type='bridge'/ source bridge='br0'/ target dev='vnet0'/ /interface Here, the source device is a bridge previously setup to have a physical device enslaved (regular or SR-IOV) The target device is the plain TAP device plain TAP device - no need for change here. 2. A new one using hardware bridging, which we can freely define for our new needs interface type='direct'/ source dev='eth0' mode='vepa|pepa|bridge'/ target dev='vnet0'/ /interface In contrast to the ACLs ( :-) ), where I would regard the ACLs as VM-attached data that ideally would migrate along when the VM migrates between hosts, in the case of this network attachment I'd not put host-specific information in the domain XML as is the case here with the 'eth0'. Who knows, maybe it's going to be the SR-IOV virtual adapter eth10 on the destination side? With the redirection into the network XML (or similar) one could define a network XML per VM, create that with host-specific information on the destination, i.e., eth10, and then migrate the VM previously linked to eth0 via macvtap that then connected via eth10. It's more work for upper layers, but if there is a need for optimization for throughput, then maybe that's the only way that optimizations can be done. Otherwise if all VMs in the data center are created with above XML and eth0 then they will all need to stay on eth0 I suppose. In this context, how will the virtual functions
Re: [libvirt] [PATCH] xen_hypervisor.c: avoid NULL deref for NULL domain argument
Daniel P. Berrange wrote:
On Tue, Jan 26, 2010 at 08:24:25PM +0100, Jim Meyering wrote:
When domain is NULL, don't deref NULL. Instead, just return -1,
as in many other functions in this file, and as this function did
up until a month ago.
An alternative (taken 3 times in this file) is to do this:
virXenErrorFunc (NULL, VIR_ERR_INTERNAL_ERROR, __FUNCTION__,
domain or conn is NULL, 0);
return -1;
I could go either way.
From 177556167775b806a29bcb1af7ba4294d1909912 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 26 Jan 2010 20:17:07 +0100
Subject: [PATCH] xen_hypervisor.c: avoid NULL deref for NULL domain argument
* src/xen/xen_hypervisor.c (xenHypervisorGetVcpus): Don't attempt
to diagnose an unlikely NULL-domain or NULL-domain-conn error.
---
src/xen/xen_hypervisor.c |7 ++-
1 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c
index 6d8accc..0257be2 100644
--- a/src/xen/xen_hypervisor.c
+++ b/src/xen/xen_hypervisor.c
@@ -1,7 +1,7 @@
/*
* xen_internal.c: direct access to Xen hypervisor level
*
- * Copyright (C) 2005, 2006, 2007, 2008, 2009 Red Hat, Inc.
+ * Copyright (C) 2005-2010 Red Hat, Inc.
*
* See COPYING.LIB for the License of this software
*
@@ -3475,11 +3475,8 @@ xenHypervisorGetVcpus(virDomainPtr domain,
virVcpuInfoPtr info, int maxinfo,
virVcpuInfoPtr ipt;
int nbinfo, i;
-if (domain == NULL || domain-conn == NULL) {
-virXenErrorFunc (domain-conn, VIR_ERR_INVALID_ARG, __FUNCTION__,
-invalid argument, 0);
+if (domain == NULL || domain-conn == NULL)
return -1;
-}
I'd rather we just got rid of that check completely - its a left
over from a time when Xen was the only driver these entry points
needed to do full checking. Now all mandatory parameters are checked
in the previous libvirt.c layer.
Here's an additional patch, to eliminate all of the domain == NULL
tests. I want to keep this global clean-up patch separate from
the above bug-fixing one.
I'm less confident about removing the daomin-conn tests,
and would be inclined to leave them as-is, or use an assert, if you
want to remove them. If we also remove the daomin-conn == NULL tests,
an added assert is the best way to keep clang/coverity from
complaining about a possible NULL-dereference.
From 9e6f7ca7a0dfa6b220e598d04ca40d33e67feb22 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Wed, 27 Jan 2010 13:34:03 +0100
Subject: [PATCH] xen_hypervisor.c: remove all domain == NULL tests, ...
* src/xen/xen_hypervisor.c: Remove all domain == NULL tests.
* src/xen/xen_hypervisor.h: Instead, use ATTRIBUTE_NONNULL to
mark each domain parameter as known always to be non-NULL.
---
src/xen/xen_hypervisor.c | 28 ++--
src/xen/xen_hypervisor.h | 44 +---
2 files changed, 43 insertions(+), 29 deletions(-)
diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c
index 0257be2..994f5ef 100644
--- a/src/xen/xen_hypervisor.c
+++ b/src/xen/xen_hypervisor.c
@@ -1130,7 +1130,7 @@ xenHypervisorGetSchedulerType(virDomainPtr domain, int
*nparams)
char *schedulertype = NULL;
xenUnifiedPrivatePtr priv;
-if ((domain == NULL) || (domain-conn == NULL)) {
+if (domain-conn == NULL) {
virXenErrorFunc(NULL, VIR_ERR_INTERNAL_ERROR, __FUNCTION__,
domain or conn is NULL, 0);
return NULL;
@@ -1214,7 +1214,7 @@ xenHypervisorGetSchedulerParameters(virDomainPtr domain,
{
xenUnifiedPrivatePtr priv;
-if ((domain == NULL) || (domain-conn == NULL)) {
+if (domain-conn == NULL) {
virXenErrorFunc(NULL, VIR_ERR_INTERNAL_ERROR, __FUNCTION__,
domain or conn is NULL, 0);
return -1;
@@ -1317,7 +1317,7 @@ xenHypervisorSetSchedulerParameters(virDomainPtr domain,
xenUnifiedPrivatePtr priv;
char buf[256];
-if ((domain == NULL) || (domain-conn == NULL)) {
+if (domain-conn == NULL) {
virXenErrorFunc (NULL, VIR_ERR_INTERNAL_ERROR, __FUNCTION__,
domain or conn is NULL, 0);
return -1;
@@ -3062,12 +3062,12 @@ xenHypervisorGetDomMaxMemory(virConnectPtr conn, int id)
*
* Returns the memory size in kilobytes or 0 in case of error.
*/
-static unsigned long
+static unsigned long ATTRIBUTE_NONNULL (1)
xenHypervisorGetMaxMemory(virDomainPtr domain)
{
xenUnifiedPrivatePtr priv;
-if ((domain == NULL) || (domain-conn == NULL))
+if (domain-conn == NULL)
return 0;
priv = (xenUnifiedPrivatePtr) domain-conn-privateData;
@@ -3176,7 +3176,7 @@ xenHypervisorGetDomainInfo(virDomainPtr domain,
virDomainInfoPtr info)
{
xenUnifiedPrivatePtr priv;
-if ((domain == NULL) || (domain-conn == NULL))
+if (domain-conn == NULL)
return
Re: [libvirt] [PATCH] portability to non-glibc: don't use realpath(..., NULL)
ACK Pushed. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] cpu_x86.c: avoid NULL-deref for invalid arguments
ACK Pushed. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Add missing sata controller type to domain.rng
On Wed, Jan 27, 2010 at 12:00:26PM +, Matthew Booth wrote: * docs/schemas/domain.rng: Add sata controller type --- docs/schemas/domain.rng |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng index 49b57eb..827ff6f 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domain.rng @@ -529,6 +529,7 @@ valuefdc/value valueide/value valuescsi/value +valuesata/value /choice /attribute /optional ACK, 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] maint: fix spelling error in hacking
* HACKING: STRCASEEQ is case insensitive. * docs/hacking.html.in: Likewise. --- HACKING |4 ++-- docs/hacking.html.in |4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/HACKING b/HACKING index 0c65dad..3844e50 100644 --- a/HACKING +++ b/HACKING @@ -222,7 +222,7 @@ one of the following semantically named macros STREQ(a,b) STRNEQ(a,b) - - For case sensitive equality: + - For case insensitive equality: STRCASEEQ(a,b) STRCASENEQ(a,b) @@ -231,7 +231,7 @@ one of the following semantically named macros STREQLEN(a,b,n) STRNEQLEN(a,b,n) - - For case sensitive equality of a substring: + - For case insensitive equality of a substring: STRCASEEQLEN(a,b,n) STRCASENEQLEN(a,b,n) diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 43a79f7..96f6657 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -256,7 +256,7 @@ /pre /li - lipFor case sensitive equality:/p + lipFor case insensitive equality:/p pre STRCASEEQ(a,b) STRCASENEQ(a,b) @@ -271,7 +271,7 @@ /pre /li - lipFor case sensitive equality of a substring:/p + lipFor case insensitive equality of a substring:/p pre STRCASEEQLEN(a,b,n) -- 1.6.6 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Supporting vhost-net and macvtap in libvirt for QEMU
On Tue, Jan 26, 2010 at 07:15:05PM -0800, Vivek Kashyap wrote:
On Mon, 25 Jan 2010, Anthony Liguori wrote:
Describing the bridge and modes:
So, we can define the bridge function using a new type or maybe extend
the bridge.xml itself.
interface type='bridge' name='br0'
bridge
type='hypervisor|embedded|ethernet'/ //hypervisor is default
mode='all|VEPA|PEPA|VEB'/ // 'all' is default if supported.
interface type='ethernet' name='eth0'/
/bridge
/interface
Does this really map to how VEPA works?
For a physical bridge, you create a br0 network interface that also has
eth0 as a component.
Right. So a bridge has at least one 'uplink'. In this case the bridge is
an abstract concept. It still has an 'uplink' which is the device (eth0
in this instance).
With VEPA and macv{lan,tap}, you do not create a single br0 interface.
Instead, for the given physical port, you create interfaces for each tap
device and hand them over. IOW, while something like:
interface type='bridge' name='br0'
bridge
interface type='ethernet' name='eth0'/
interface type='ethernet' name='eth1'/
/bridge
/interface
The above is not in the domain xml but was proposed in the bridge xml.
The advantage of using the bridge concept is that it appears the same
for macvlan and the virtual Linux host bridge. The 'macvlan' interface
itself can support 'bridge' mode in addition to the 'vepa' mode.
Therefore, one is creating the bridge, attaching it to the physical
device. This device is the one which provides the 'uplink' i.e. is
either the sr-iov card or is the device associated with the macvlan driver.
The domain xml can now point to the above bridge. For the interfaces it
creates it can associate target names.
The main issue with this, is that when using VEPA/macvlan there's
no actual host device being created as there is when using the
linux software bridge. The interface descriptions here are mapped
straight into the /etc/sysconfig/networking-scripts/ifcfg-XXX files
that trigger creation setup of the physical, bridge, bonding vlan
interfaces. Since there is no actual bridge interface, there's no
ifcfg-XXX to map onto in the VEPA case.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Log flags in virConnectCompareCPU
Signed-off-by: Jiri Denemark jdene...@redhat.com
---
src/libvirt.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c
index 32f9fc4..8a9ee20 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -10951,7 +10951,7 @@ virConnectCompareCPU(virConnectPtr conn,
const char *xmlDesc,
unsigned int flags)
{
-VIR_DEBUG(conn=%p, xmlDesc=%s, conn, xmlDesc);
+VIR_DEBUG(conn=%p, xmlDesc=%s, flags=%u, conn, xmlDesc, flags);
virResetLastError();
--
1.6.6.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] maint: avoid excess parens in STREQ
* src/internal.h (STREQ, STRCASEEQ, STRNEQ, STRCASENEQ, STREQLEN)
(STRCASEEQLEN, STRNEQLEN, STRCASENEQLEN, STRPREFIX): Avoid
redundant parenthesis.
* examples/domain-events/events-c/event-test.c (STREQ): Likewise.
* src/storage/parthelper.c (STREQ): Likewise.
---
These macros were originally inspired by Jim Meyering, who has since made
this same cleanup elsewhere. For example:
http://lists.gnu.org/archive/html/bug-gnulib/2010-01/msg00293.html
examples/domain-events/events-c/event-test.c |2 +-
src/internal.h | 18 +-
src/storage/parthelper.c |4 ++--
3 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/examples/domain-events/events-c/event-test.c
b/examples/domain-events/events-c/event-test.c
index b2eb1d5..e8f5505 100644
--- a/examples/domain-events/events-c/event-test.c
+++ b/examples/domain-events/events-c/event-test.c
@@ -14,7 +14,7 @@
__func__, __LINE__)
#define DEBUG(fmt, ...) printf(%s:%d: fmt \n, \
__func__, __LINE__, __VA_ARGS__)
-#define STREQ(a,b) (strcmp((a),(b)) == 0)
+#define STREQ(a,b) (strcmp(a,b) == 0)
#ifndef ATTRIBUTE_UNUSED
#define ATTRIBUTE_UNUSED __attribute__((__unused__))
diff --git a/src/internal.h b/src/internal.h
index 5ca1fa3..ec8a49f 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -48,15 +48,15 @@
#define N_(str) dgettext(GETTEXT_PACKAGE, (str))
/* String equality tests, suggested by Jim Meyering. */
-#define STREQ(a,b) (strcmp((a),(b)) == 0)
-#define STRCASEEQ(a,b) (strcasecmp((a),(b)) == 0)
-#define STRNEQ(a,b) (strcmp((a),(b)) != 0)
-#define STRCASENEQ(a,b) (strcasecmp((a),(b)) != 0)
-#define STREQLEN(a,b,n) (strncmp((a),(b),(n)) == 0)
-#define STRCASEEQLEN(a,b,n) (strncasecmp((a),(b),(n)) == 0)
-#define STRNEQLEN(a,b,n) (strncmp((a),(b),(n)) != 0)
-#define STRCASENEQLEN(a,b,n) (strncasecmp((a),(b),(n)) != 0)
-#define STRPREFIX(a,b) (strncmp((a),(b),strlen((b))) == 0)
+#define STREQ(a,b) (strcmp(a,b) == 0)
+#define STRCASEEQ(a,b) (strcasecmp(a,b) == 0)
+#define STRNEQ(a,b) (strcmp(a,b) != 0)
+#define STRCASENEQ(a,b) (strcasecmp(a,b) != 0)
+#define STREQLEN(a,b,n) (strncmp(a,b,n) == 0)
+#define STRCASEEQLEN(a,b,n) (strncasecmp(a,b,n) == 0)
+#define STRNEQLEN(a,b,n) (strncmp(a,b,n) != 0)
+#define STRCASENEQLEN(a,b,n) (strncasecmp(a,b,n) != 0)
+#define STRPREFIX(a,b) (strncmp(a,b,strlen(b)) == 0)
#define NUL_TERMINATE(buf) do { (buf)[sizeof(buf)-1] = '\0'; } while (0)
#define ARRAY_CARDINALITY(Array) (sizeof (Array) / sizeof *(Array))
diff --git a/src/storage/parthelper.c b/src/storage/parthelper.c
index ab04842..5626cd2 100644
--- a/src/storage/parthelper.c
+++ b/src/storage/parthelper.c
@@ -10,7 +10,7 @@
* in a reliable fashion if merely after a list of partitions sizes,
* though it is fine for creating partitions.
*
- * Copyright (C) 2007-2008 Red Hat, Inc.
+ * Copyright (C) 2007-2008, 2010 Red Hat, Inc.
* Copyright (C) 2007-2008 Daniel P. Berrange
*
* This library is free software; you can redistribute it and/or
@@ -37,7 +37,7 @@
#include string.h
/* we don't need to include the full internal.h just for this */
-#define STREQ(a,b) (strcmp((a),(b)) == 0)
+#define STREQ(a,b) (strcmp(a,b) == 0)
/* Make the comparisons below fail if your parted headers
are so old that they lack the definition. */
--
1.6.6
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Look in /usr/libexec for the qemu-kvm binary.
On RHEL-5 the qemu-kvm binary is located in /usr/libexec.
To reduce confusion for people trying to run upstream libvirt
on RHEL-5 machines, make the qemu driver look in /usr/libexec
for the qemu-kvm binary.
To make this work, I modified virFindFileInPath to handle an
absolute path correctly. I also ran into an issue where
NULL was sometimes being passed for the file parameter
to virFindFileInPath; it didn't crash prior to this patch
since it was building paths like /usr/bin/(null). This
is non-standard behavior, though, so I added a NULL
check at the beginning.
Signed-off-by: Chris Lalancette clala...@redhat.com
---
src/qemu/qemu_conf.c |3 ++-
src/util/util.c | 13 +
2 files changed, 15 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index f4a6c08..d5e38c2 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -804,7 +804,8 @@ qemudCapsInitGuest(virCapsPtr caps,
if (STREQ(info-arch, hostmachine) ||
(STREQ(hostmachine, x86_64) STREQ(info-arch, i686))) {
if (access(/dev/kvm, F_OK) == 0) {
-const char *const kvmbins[] = { qemu-kvm, /* Fedora */
+const char *const kvmbins[] = { /usr/libexec/qemu-kvm, /* RHEL */
+qemu-kvm, /* Fedora */
kvm }; /* Upstream .spec */
for (i = 0; i ARRAY_CARDINALITY(kvmbins); ++i) {
diff --git a/src/util/util.c b/src/util/util.c
index 0ce5026..394ef35 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -1114,6 +1114,19 @@ char *virFindFileInPath(const char *file)
char *pathseg;
char fullpath[PATH_MAX];
+if (file == NULL)
+return NULL;
+
+/* if we are passed an absolute path (starting with /), return a
+ * copy of that path
+ */
+if (file[0] == '/') {
+if (virFileExists(file))
+return strdup(file);
+else
+return NULL;
+}
+
/* copy PATH env so we can tweak it */
if (virStrcpyStatic(pathenv, getenv(PATH)) == NULL)
return NULL;
--
1.6.6
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Fix PCI host reattach on domain detach.
Similar to the race fixed by
be34c3c7efbb1ea8999530f98b99c5dde3793f84, make sure
to wait around for KVM to release the resources from
a hot-detached PCI device before attempting to
rebind that device to the host driver.
Signed-off-by: Chris Lalancette clala...@redhat.com
---
src/qemu/qemu_driver.c | 39 +++
1 files changed, 23 insertions(+), 16 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index bbdbe33..5bf6743 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2240,6 +2240,26 @@ cleanup:
}
static void
+qemudReattachManagedDevice(pciDevice *dev)
+{
+int retries = 100;
+
+if (pciDeviceGetManaged(dev)) {
+while (pciWaitForDeviceCleanup(dev, kvm_assigned_device)
+retries) {
+usleep(100*1000);
+retries--;
+}
+if (pciReAttachDevice(NULL, dev) 0) {
+virErrorPtr err = virGetLastError();
+VIR_ERROR(_(Failed to re-attach PCI device: %s),
+ err ? err-message : );
+virResetError(err);
+}
+}
+}
+
+static void
qemuDomainReAttachHostDevices(virConnectPtr conn,
struct qemud_driver *driver,
virDomainDefPtr def)
@@ -2279,20 +2299,7 @@ qemuDomainReAttachHostDevices(virConnectPtr conn,
for (i = 0; i pciDeviceListCount(pcidevs); i++) {
pciDevice *dev = pciDeviceListGet(pcidevs, i);
-int retries = 100;
-if (pciDeviceGetManaged(dev)) {
-while (pciWaitForDeviceCleanup(dev, kvm_assigned_device)
-retries) {
-usleep(100*1000);
-retries--;
-}
-if (pciReAttachDevice(NULL, dev) 0) {
-virErrorPtr err = virGetLastError();
-VIR_ERROR(_(Failed to re-attach PCI device: %s),
- err ? err-message : );
-virResetError(err);
-}
-}
+qemudReattachManagedDevice(dev);
}
pciDeviceListFree(conn, pcidevs);
@@ -6128,11 +6135,11 @@ static int qemudDomainDetachHostPciDevice(virConnectPtr
conn,
if (!pci)
ret = -1;
else {
+pciDeviceSetManaged(pci, detach-managed);
pciDeviceListDel(conn, driver-activePciHostdevs, pci);
if (pciResetDevice(conn, pci, driver-activePciHostdevs) 0)
ret = -1;
-if (detach-managed pciReAttachDevice(conn, pci) 0)
-ret = -1;
+qemudReattachManagedDevice(pci);
pciFreeDevice(conn, pci);
}
--
1.6.6
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Allow a per-PCI passthrough device permissive attribute
Currently there is a global tag to let the administrator
turn off system-wide ACS checking when doing PCI device
passthrough. However, this is too coarse-grained of an
attribute, since it doesn't allow setups where certain
guests are trusted while other ones are untrusted. Allow
more complicated setups by making the device checking
a per-device setting.
The more detailed explanation of why this is necessary
delves deep into PCIe internals. Ideally we'd like
to be able to probe devices and figure out whether it
is safe to assign them. In practice, this isn't possible
because PCIe allows devices to have hidden bridges
that software can't discover. If you were to have
two devices assigned to two different domains behind
one of these hidden bridges, they could do P2P traffic
and bypass all of the VT-d/IOMMU checks.
The next thing we could try to do is to have a whitelist
of devices that we know to be safe. For instance, instead
of a hidden bridge, PCI devices can multiplex functions
instead, which causes all traffic to head to an upstream
bridge before P2P can take place. Additionally, some
hidden PCI bridges may have ACS on-board. In both of
these cases it's safe to passthrough the device(s), since
they can't P2P without the IOMMU getting involved.
However, even if we did have a whitelist, I think we still
need a permissive attribute. For one thing, the whitelist
will always be out of date with respect to new hardware,
so we'd need to allow administrators to temporarily
override the whitelist restriction until a new version of
the whitelist came out. Also, we want to support the case
where the administrator knows it is safe to assign possibly
unsafe devices to a domain he trusts.
Signed-off-by: Chris Lalancette clala...@redhat.com
---
docs/schemas/domain.rng |6 ++
src/conf/domain_conf.c | 14 +++---
src/conf/domain_conf.h |1 +
src/libvirt_private.syms |2 ++
src/qemu/qemu_driver.c |1 +
src/util/pci.c | 13 -
src/util/pci.h |3 +++
7 files changed, 36 insertions(+), 4 deletions(-)
diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng
index 827ff6f..9f90f4d 100644
--- a/docs/schemas/domain.rng
+++ b/docs/schemas/domain.rng
@@ -1175,6 +1175,12 @@
valueno/value
/choice
/attribute
+attribute name=permissive
+ choice
+valueyes/value
+valueno/value
+ /choice
+/attribute
/optional
group
element name=source
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e548d1d..899967d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2929,7 +2929,7 @@ virDomainHostdevDefParseXML(virConnectPtr conn,
xmlNodePtr cur;
virDomainHostdevDefPtr def;
-char *mode, *type = NULL, *managed = NULL;
+char *mode, *type = NULL, *managed = NULL, *permissive = NULL;
if (VIR_ALLOC(def) 0) {
virReportOOMError(conn);
@@ -2968,6 +2968,13 @@ virDomainHostdevDefParseXML(virConnectPtr conn,
VIR_FREE(managed);
}
+permissive = virXMLPropString(node, permissive);
+if (permissive != NULL) {
+if (STREQ(permissive, yes))
+def-permissive = 1;
+VIR_FREE(permissive);
+}
+
cur = node-children;
while (cur != NULL) {
if (cur-type == XML_ELEMENT_NODE) {
@@ -5243,8 +5250,9 @@ virDomainHostdevDefFormat(virConnectPtr conn,
return -1;
}
-virBufferVSprintf(buf, hostdev mode='%s' type='%s' managed='%s'\n,
- mode, type, def-managed ? yes : no);
+virBufferVSprintf(buf, hostdev mode='%s' type='%s' managed='%s'
permissive='%s'\n,
+ mode, type, def-managed ? yes : no,
+ def-permissive ? yes : no);
virBufferAddLit(buf, source\n);
if (def-source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) {
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 7be090d..57ab4b4 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -475,6 +475,7 @@ typedef virDomainHostdevDef *virDomainHostdevDefPtr;
struct _virDomainHostdevDef {
int mode; /* enum virDomainHostdevMode */
unsigned int managed : 1;
+unsigned int permissive : 1;
union {
struct {
int type; /* enum virDomainHostdevBusType */
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index d56fb7d..49e222e 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -441,6 +441,8 @@ pciWaitForDeviceCleanup;
pciResetDevice;
pciDeviceSetManaged;
pciDeviceGetManaged;
+pciDeviceSetPermissive;
+pciDeviceGetPermissive;
pciDeviceListNew;
pciDeviceListFree;
pciDeviceListAdd;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5bf6743..9848f1c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2137,6 +2137,7 @@
Re: [libvirt] [PATCH 1/1] Cleanups for udev init code
On 01/26/2010 05:39 PM, Matthias Bolte wrote:
Hi Matthias,
Thanks for your catch of the priv == NULL case in the shutdown code.
Priv does NOT leak, however, as it is freed in one place in the shutdown
code in all cases.
Updated patch to follow.
Dave
2010/1/26 David Allandal...@redhat.com:
---
src/node_device/node_device_udev.c | 34 +-
1 files changed, 13 insertions(+), 21 deletions(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index dcd889d..6895ac5 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1368,8 +1368,9 @@ static int udevDeviceMonitorShutdown(void)
priv = driverState-privateData;
-if (priv-watch != -1)
+if (priv-watch != -1) {
virEventRemoveHandle(priv-watch);
+}
udev_monitor = DRV_STATE_UDEV_MONITOR(driverState);
Due to your changes to udevDeviceMonitorStartup it is possible that
udevDeviceMonitorShutdown is called with driverState != NULL and
driverState-privateData == NULL. In this case 'if (priv-watch !=
-1)' will segfault and 'udev_monitor =
DRV_STATE_UDEV_MONITOR(driverState);' will segfault also. Changing it
like this will fix the problem:
if (priv != NULL) {
if (priv-watch != -1)
virEventRemoveHandle(priv-watch);
udev_monitor = DRV_STATE_UDEV_MONITOR(driverState);
}
@@ -1387,6 +1388,7 @@ static int udevDeviceMonitorShutdown(void)
virMutexDestroy(driverState-lock);
VIR_FREE(driverState);
VIR_FREE(priv);
+
} else {
ret = -1;
}
@@ -1547,28 +1549,22 @@ static int udevDeviceMonitorStartup(int privileged
ATTRIBUTE_UNUSED)
{
udevPrivate *priv = NULL;
struct udev *udev = NULL;
-int ret = 0;
+int ret = -1;
-if (VIR_ALLOC(priv) 0) {
+if (VIR_ALLOC(driverState) 0) {
virReportOOMError(NULL);
-ret = -1;
goto out;
}
-priv-watch = -1;
-
-if (VIR_ALLOC(driverState) 0) {
+if (VIR_ALLOC(priv) 0) {
virReportOOMError(NULL);
-VIR_FREE(priv);
-ret = -1;
goto out;
}
+priv-watch = -1;
+
if (virMutexInit(driverState-lock) 0) {
VIR_ERROR0(Failed to initialize mutex for driverState);
-VIR_FREE(priv);
-VIR_FREE(driverState);
-ret = -1;
priv leaks here.
goto out;
}
@@ -1585,10 +1581,8 @@ static int udevDeviceMonitorStartup(int privileged
ATTRIBUTE_UNUSED)
priv-udev_monitor = udev_monitor_new_from_netlink(udev, udev);
if (priv-udev_monitor == NULL) {
-VIR_FREE(priv);
priv leaks here.
Moving the driverState-privateData = priv; line directly after the
priv-watch = -1; will fix this leaks.
nodeDeviceUnlock(driverState);
VIR_ERROR0(udev_monitor_new_from_netlink returned NULL);
-ret = -1;
goto out;
}
@@ -1608,27 +1602,25 @@ static int udevDeviceMonitorStartup(int privileged
ATTRIBUTE_UNUSED)
priv-watch = virEventAddHandle(udev_monitor_get_fd(priv-udev_monitor),
VIR_EVENT_HANDLE_READABLE,
udevEventHandleCallback, NULL, NULL);
+
+nodeDeviceUnlock(driverState);
+
if (priv-watch == -1) {
-nodeDeviceUnlock(driverState);
-ret = -1;
goto out;
}
-nodeDeviceUnlock(driverState);
-
/* Create a fictional 'computer' device to root the device tree. */
if (udevSetupSystemDev() != 0) {
-ret = -1;
goto out;
}
/* Populate with known devices */
-
if (udevEnumerateDevices(udev) != 0) {
-ret = -1;
goto out;
}
+ret = 0;
+
out:
if (ret == -1) {
udevDeviceMonitorShutdown();
--
1.6.5.5
Matthias
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/1] Clean up udev init
The following patch contains cleanup for the udev init error handling and Matthias' fix for the case in which priv is NULL when shutdown is called. Dave -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/1] Cleanups for udev init code
This patch contains a fix for a segfault when priv is NULL pointed out
by Matthias Bolte.
---
src/node_device/node_device_udev.c | 42 +++
1 files changed, 18 insertions(+), 24 deletions(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index dcd889d..c76c568 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1363,15 +1363,18 @@ static int udevDeviceMonitorShutdown(void)
struct udev_monitor *udev_monitor = NULL;
struct udev *udev = NULL;
-if (driverState) {
+if (driverState != NULL) {
nodeDeviceLock(driverState);
priv = driverState-privateData;
-if (priv-watch != -1)
-virEventRemoveHandle(priv-watch);
+if (priv != NULL) {
+if (priv-watch != -1) {
+virEventRemoveHandle(priv-watch);
+}
-udev_monitor = DRV_STATE_UDEV_MONITOR(driverState);
+udev_monitor = DRV_STATE_UDEV_MONITOR(driverState);
+}
if (udev_monitor != NULL) {
udev = udev_monitor_get_udev(udev_monitor);
@@ -1387,6 +1390,7 @@ static int udevDeviceMonitorShutdown(void)
virMutexDestroy(driverState-lock);
VIR_FREE(driverState);
VIR_FREE(priv);
+
} else {
ret = -1;
}
@@ -1547,28 +1551,22 @@ static int udevDeviceMonitorStartup(int privileged
ATTRIBUTE_UNUSED)
{
udevPrivate *priv = NULL;
struct udev *udev = NULL;
-int ret = 0;
+int ret = -1;
-if (VIR_ALLOC(priv) 0) {
+if (VIR_ALLOC(driverState) 0) {
virReportOOMError(NULL);
-ret = -1;
goto out;
}
-priv-watch = -1;
-
-if (VIR_ALLOC(driverState) 0) {
+if (VIR_ALLOC(priv) 0) {
virReportOOMError(NULL);
-VIR_FREE(priv);
-ret = -1;
goto out;
}
+priv-watch = -1;
+
if (virMutexInit(driverState-lock) 0) {
VIR_ERROR0(Failed to initialize mutex for driverState);
-VIR_FREE(priv);
-VIR_FREE(driverState);
-ret = -1;
goto out;
}
@@ -1585,10 +1583,8 @@ static int udevDeviceMonitorStartup(int privileged
ATTRIBUTE_UNUSED)
priv-udev_monitor = udev_monitor_new_from_netlink(udev, udev);
if (priv-udev_monitor == NULL) {
-VIR_FREE(priv);
nodeDeviceUnlock(driverState);
VIR_ERROR0(udev_monitor_new_from_netlink returned NULL);
-ret = -1;
goto out;
}
@@ -1608,27 +1604,25 @@ static int udevDeviceMonitorStartup(int privileged
ATTRIBUTE_UNUSED)
priv-watch = virEventAddHandle(udev_monitor_get_fd(priv-udev_monitor),
VIR_EVENT_HANDLE_READABLE,
udevEventHandleCallback, NULL, NULL);
+
+nodeDeviceUnlock(driverState);
+
if (priv-watch == -1) {
-nodeDeviceUnlock(driverState);
-ret = -1;
goto out;
}
-nodeDeviceUnlock(driverState);
-
/* Create a fictional 'computer' device to root the device tree. */
if (udevSetupSystemDev() != 0) {
-ret = -1;
goto out;
}
/* Populate with known devices */
-
if (udevEnumerateDevices(udev) != 0) {
-ret = -1;
goto out;
}
+ret = 0;
+
out:
if (ret == -1) {
udevDeviceMonitorShutdown();
--
1.6.5.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] support XEN_SYSCTL_INTERFACE_VERSION
Daniel P. Berrange wrote:
On Tue, Jan 26, 2010 at 11:34:28PM -0700, Jim Fehlig wrote:
xen-unstable c/s 20762 bumped XEN_SYSCTL_INTERFACE_VERSION to 7. I
don't see how the interface change affects libvirt, other than failing
xenHypervisorInit() since version 7 is not tried.
The attached patch accommodates the upcoming Xen 4.0 release by checking
for XEN_SYSCTL_INTERFACE_VERSION 7. If found, it sets
XEN_DOMCTL_INTERFACE_VERSION to 6, which is also new to Xen 4.0.
Regards,
Jim
Index: libvirt-0.7.4/src/xen/xen_hypervisor.c
===
--- libvirt-0.7.4.orig/src/xen/xen_hypervisor.c
+++ libvirt-0.7.4/src/xen/xen_hypervisor.c
@@ -2100,12 +2100,14 @@ xenHypervisorInit(void)
DEBUG0(Using hypervisor call v2, sys ver6 dom ver5\n);
goto done;
}
-/* Xen 4.0 */
+}
+
+/* Xen 4.0 */
+sys_interface_version = 7; /* XEN_SYSCTL_INTERFACE_VERSION */
+if (virXen_getdomaininfo(fd, 0, info) == 1) {
dom_interface_version = 6; /* XEN_DOMCTL_INTERFACE_VERSION */
-if (virXen_getvcpusinfo(fd, 0, 0, ipt, NULL, 0) == 0){
-DEBUG0(Using hypervisor call v2, sys ver6 dom ver6\n);
-goto done;
-}
+DEBUG0(Using hypervisor call v2, sys ver7 dom ver6\n);
+goto done;
}
hypervisor_version = 1;
ACK
Thanks. I've pushed this fix.
Jim
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Allow a per-PCI passthrough device permissive attribute
On Wed, Jan 27, 2010 at 09:23:52AM -0500, Chris Lalancette wrote: Currently there is a global tag to let the administrator turn off system-wide ACS checking when doing PCI device passthrough. However, this is too coarse-grained of an attribute, since it doesn't allow setups where certain guests are trusted while other ones are untrusted. Allow more complicated setups by making the device checking a per-device setting. The more detailed explanation of why this is necessary delves deep into PCIe internals. Ideally we'd like to be able to probe devices and figure out whether it is safe to assign them. In practice, this isn't possible because PCIe allows devices to have hidden bridges that software can't discover. If you were to have two devices assigned to two different domains behind one of these hidden bridges, they could do P2P traffic and bypass all of the VT-d/IOMMU checks. The next thing we could try to do is to have a whitelist of devices that we know to be safe. For instance, instead of a hidden bridge, PCI devices can multiplex functions instead, which causes all traffic to head to an upstream bridge before P2P can take place. Additionally, some hidden PCI bridges may have ACS on-board. In both of these cases it's safe to passthrough the device(s), since they can't P2P without the IOMMU getting involved. However, even if we did have a whitelist, I think we still need a permissive attribute. For one thing, the whitelist will always be out of date with respect to new hardware, so we'd need to allow administrators to temporarily override the whitelist restriction until a new version of the whitelist came out. Also, we want to support the case where the administrator knows it is safe to assign possibly unsafe devices to a domain he trusts. A domain is only trusted until its guest OS gets exploited at which point this proposed change may let it escape into the host. If you don't have any IOMMU on your host, you can't use PCI device assignment with KVM at all, because it would not be safe in the event of guest exploit / mis- behavior. The same is true of device assignment in this non-ACS + hidden bridge case. Thus I don't see why we should introduce a special permissive flag solely for the non-ACS edge case, while at the smae time not allowing the same permissiveness for the far more common non-IOMMU case. NB, I'm not suggesting we allow skipping of the checks for the non-IOMMU case either. Keeping a whitelist of devices up2date wrt new hardware launches is no more troublesome than the existing problem of updating the PCI-IDs databse or actually providing updated kernel releases with new drivers. If we use the permissive attribute, then every admin with a device that is known to be safe has the pain of setting the permissive attribute, every time, on every machine with this hardware. If we have a whitelist, then 99% of the time everything will just work because it will already be known to the whitelist. If the whitelist were an external datafile the admin could even extend it in the rare occasion when a new device were not known. This is a choice of make everyone solve over over again themselves, or solve it once for everybody. I don't really like the idea of a whitelist, but I like it more than just pushing the problem onto admins via per guest flags. For that matter I don't like the host level flag we have either and would rather we removed it. If only there's a 3rd way that were neither flags or whitelists ... Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/7] Split out QEMU code for building PCI/USB hostdev arg values
To allow for better code reuse from hotplug methods, the code for
generating PCI/USB hostdev arg values is split out into separate
methods
* qemu/qemu_conf.h, qemu/qemu_conf.c: Introduce new APis for
qemuBuildPCIHostdevPCIDevStr, qemuBuildUSBHostdevUsbDevStr
and qemuBuildUSBHostdevDevStr
---
src/qemu/qemu_conf.c | 105 +
src/qemu/qemu_conf.h | 10 +
2 files changed, 81 insertions(+), 34 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index e11ec35..85320c1 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -2508,6 +2508,65 @@ error:
}
+char *
+qemuBuildPCIHostdevPCIDevStr(virDomainHostdevDefPtr dev)
+{
+char *ret = NULL;
+
+if (virAsprintf(ret, host=%.2x:%.2x.%.1x,
+dev-source.subsys.u.pci.bus,
+dev-source.subsys.u.pci.slot,
+dev-source.subsys.u.pci.function) 0)
+virReportOOMError(NULL);
+
+return ret;
+}
+
+
+char *
+qemuBuildUSBHostdevDevStr(virDomainHostdevDefPtr dev)
+{
+char *ret = NULL;
+
+if (!dev-source.subsys.u.usb.bus
+!dev-source.subsys.u.usb.device) {
+qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR, %s,
+ _(USB host device is missing bus/device
information));
+return NULL;
+}
+
+if (virAsprintf(ret, usb-host,hostbus=%.3d,hostaddr=%.3d,id=%s,
+dev-source.subsys.u.usb.bus,
+dev-source.subsys.u.usb.device,
+dev-info.alias) 0)
+virReportOOMError(NULL);
+
+return ret;
+}
+
+
+char *
+qemuBuildUSBHostdevUsbDevStr(virDomainHostdevDefPtr dev)
+{
+char *ret = NULL;
+
+if (!dev-source.subsys.u.usb.bus
+!dev-source.subsys.u.usb.device) {
+qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR, %s,
+ _(USB host device is missing bus/device
information));
+return NULL;
+}
+
+if (virAsprintf(ret, host:%.3d.%.3d,
+dev-source.subsys.u.usb.bus,
+dev-source.subsys.u.usb.device) 0)
+virReportOOMError(NULL);
+
+return ret;
+}
+
+
+
/* This function outputs a -chardev command line option which describes only
the
* host side of the character device */
char *
@@ -3747,10 +3806,8 @@ int qemudBuildCommandLine(virConnectPtr conn,
/* Add host passthrough hardware */
for (i = 0 ; i def-nhostdevs ; i++) {
-int ret;
-char* usbdev;
-char* pcidev;
virDomainHostdevDefPtr hostdev = def-hostdevs[i];
+char *devstr;
/* USB */
if (hostdev-mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS
@@ -3758,33 +3815,15 @@ int qemudBuildCommandLine(virConnectPtr conn,
if (qemuCmdFlags QEMUD_CMD_FLAG_DEVICE) {
ADD_ARG_LIT(-device);
-if (hostdev-source.subsys.u.usb.vendor) {
-ret = virAsprintf(usbdev,
usb-host,vendor=%.4x,product=%.4x,id=%s,
- hostdev-source.subsys.u.usb.vendor,
- hostdev-source.subsys.u.usb.product,
- hostdev-info.alias);
-} else {
-ret = virAsprintf(usbdev,
usb-host,hostbus=%.3d,hostaddr=%.3d,id=%s,
- hostdev-source.subsys.u.usb.bus,
- hostdev-source.subsys.u.usb.device,
- hostdev-info.alias);
-}
+if (!(devstr = qemuBuildUSBHostdevDevStr(hostdev)))
+goto error;
+ADD_ARG(devstr);
} else {
ADD_ARG_LIT(-usbdevice);
-if (hostdev-source.subsys.u.usb.vendor) {
-ret = virAsprintf(usbdev, host:%.4x:%.4x,
- hostdev-source.subsys.u.usb.vendor,
- hostdev-source.subsys.u.usb.product);
-} else {
-ret = virAsprintf(usbdev, host:%.3d.%.3d,
- hostdev-source.subsys.u.usb.bus,
- hostdev-source.subsys.u.usb.device);
-}
+if (!(devstr = qemuBuildUSBHostdevUsbDevStr(hostdev)))
+goto error;
+ADD_ARG(devstr);
}
-if (ret 0)
-goto error;
-
-ADD_ARG(usbdev);
}
/* PCI */
@@ -3792,21 +3831,19 @@ int qemudBuildCommandLine(virConnectPtr conn,
hostdev-source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
{
if (qemuCmdFlags QEMUD_CMD_FLAG_DEVICE) {
ADD_ARG_LIT(-device);
-if (!(pcidev = qemuBuildPCIHostdevDevStr(hostdev)))
+if (!(devstr = qemuBuildPCIHostdevDevStr(hostdev)))
[libvirt] [PATCH 0/7] Update hotplug code to use device_add
I previously changed the command line syntax to use -device for starting guests. The key reason for this was to allow unique names and PCI addresses to be assigned to all devices. I forgot todo the same for hotplug :-( This series is not yet complete, but it goes some of the way towards fixing the hotplug code to use 'device_add' for creating devices. The main remaining problem is that I'm nt assigning aliases to devices, to they all get the name of '(null)'. Obviously I'll fix that before submitting again... Daniel -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 5/7] Introduce generic virDomainDeviceInfo iterator function
The virDomainDeviceInfoIterate() function will provide a
convenient way to iterate over all devices in a domain.
* src/conf/domain_conf.c, src/conf/domain_conf.h,
src/libvirt_private.syms: Add virDomainDeviceInfoIterate()
function.
---
src/conf/domain_conf.c | 67 +++---
src/conf/domain_conf.h |8 +
src/libvirt_private.syms |1 +
3 files changed, 54 insertions(+), 22 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e548d1d..691fc84 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -824,59 +824,82 @@ void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info)
}
-static void virDomainDeviceInfoClearField(virDomainDeviceInfoPtr info, int
alias, int pciaddr)
+static int virDomainDeviceInfoClearAlias(virDomainDefPtr def ATTRIBUTE_UNUSED,
+ virDomainDeviceInfoPtr info,
+ void *opaque ATTRIBUTE_UNUSED)
{
-if (alias)
-VIR_FREE(info-alias);
-if (pciaddr
-info-type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
+VIR_FREE(info-alias);
+return 0;
+}
+
+static int virDomainDeviceInfoClearPCIAddress(virDomainDefPtr def
ATTRIBUTE_UNUSED,
+ virDomainDeviceInfoPtr info,
+ void *opaque ATTRIBUTE_UNUSED)
+{
+if (info-type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
memset(info-addr, 0, sizeof(info-addr));
info-type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
}
+return 0;
}
-
-static void virDomainDefClearDeviceInfo(virDomainDefPtr def, int alias, int
pciaddr)
+int virDomainDeviceInfoIterate(virDomainDefPtr def,
+ virDomainDeviceInfoCallback cb,
+ void *opaque)
{
int i;
for (i = 0; i def-ndisks ; i++)
-virDomainDeviceInfoClearField(def-disks[i]-info, alias, pciaddr);
+if (cb(def, def-disks[i]-info, opaque) 0)
+return -1;
for (i = 0; i def-nnets ; i++)
-virDomainDeviceInfoClearField(def-nets[i]-info, alias, pciaddr);
+if (cb(def, def-nets[i]-info, opaque) 0)
+return -1;
for (i = 0; i def-nsounds ; i++)
-virDomainDeviceInfoClearField(def-sounds[i]-info, alias, pciaddr);
+if (cb(def, def-sounds[i]-info, opaque) 0)
+return -1;
for (i = 0; i def-nhostdevs ; i++)
-virDomainDeviceInfoClearField(def-hostdevs[i]-info, alias, pciaddr);
+if (cb(def, def-hostdevs[i]-info, opaque) 0)
+return -1;
for (i = 0; i def-nvideos ; i++)
-virDomainDeviceInfoClearField(def-videos[i]-info, alias, pciaddr);
+if (cb(def, def-videos[i]-info, opaque) 0)
+return -1;
for (i = 0; i def-ncontrollers ; i++)
-virDomainDeviceInfoClearField(def-controllers[i]-info, alias,
pciaddr);
+if (cb(def, def-controllers[i]-info, opaque) 0)
+return -1;
for (i = 0; i def-nserials ; i++)
-virDomainDeviceInfoClearField(def-serials[i]-info, alias, pciaddr);
+if (cb(def, def-serials[i]-info, opaque) 0)
+return -1;
for (i = 0; i def-nparallels ; i++)
-virDomainDeviceInfoClearField(def-parallels[i]-info, alias,
pciaddr);
+if (cb(def, def-parallels[i]-info, opaque) 0)
+return -1;
for (i = 0; i def-nchannels ; i++)
-virDomainDeviceInfoClearField(def-channels[i]-info, alias, pciaddr);
+if (cb(def, def-channels[i]-info, opaque) 0)
+return -1;
for (i = 0; i def-ninputs ; i++)
-virDomainDeviceInfoClearField(def-inputs[i]-info, alias, pciaddr);
+if (cb(def, def-inputs[i]-info, opaque) 0)
+return -1;
for (i = 0; i def-nfss ; i++)
-virDomainDeviceInfoClearField(def-fss[i]-info, alias, pciaddr);
+if (cb(def, def-fss[i]-info, opaque) 0)
+return -1;
if (def-watchdog)
-virDomainDeviceInfoClearField(def-watchdog-info, alias, pciaddr);
+if (cb(def, def-watchdog-info, opaque) 0)
+return -1;
if (def-console)
-virDomainDeviceInfoClearField(def-console-info, alias, pciaddr);
+if (cb(def, def-console-info, opaque) 0)
+return -1;
+return 0;
}
void virDomainDefClearPCIAddresses(virDomainDefPtr def)
{
-virDomainDefClearDeviceInfo(def, 0, 1);
+virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearPCIAddress, NULL);
}
void virDomainDefClearDeviceAliases(virDomainDefPtr def)
{
-virDomainDefClearDeviceInfo(def, 1, 0);
+virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearAlias, NULL);
}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 7be090d..be0dc92 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -742,6 +742,14 @@ void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info);
[libvirt] [PATCH 3/7] Introduce internal QEMU monitor APIs for drive + device hotadd
The way QEMU is started has been changed to use '-device' and
the new style '-drive' syntax. This needs to be mirrored in
the hotplug code, requiring addition of two new APIs.
* src/qemu/qemu_monitor.h, src/qemu/qemu_monitor.c: Define APIs
qemuMonitorAddDevice() and qemuMonitorAddDrive()
* src/qemu/qemu_monitor_json.c, src/qemu/qemu_monitor_json.h,
src/qemu/qemu_monitor_text.c, src/qemu/qemu_monitor_text.h:
Implement the new monitor APIs
---
src/qemu/qemu_monitor.c | 27 +++
src/qemu/qemu_monitor.h |6 +++
src/qemu/qemu_monitor_json.c | 49 ++
src/qemu/qemu_monitor_json.h |6 +++
src/qemu/qemu_monitor_text.c | 77 ++
src/qemu/qemu_monitor_text.h |6 +++
6 files changed, 171 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 817ccd7..9e09876 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1304,3 +1304,30 @@ int qemuMonitorGetAllPCIAddresses(qemuMonitorPtr mon,
ret = qemuMonitorTextGetAllPCIAddresses(mon, addrs);
return ret;
}
+
+
+int qemuMonitorAddDevice(qemuMonitorPtr mon,
+ const char *devicestr)
+{
+DEBUG(mon=%p, fd=%d device=%s, mon, mon-fd, devicestr);
+int ret;
+
+if (mon-json)
+ret = qemuMonitorJSONAddDevice(mon, devicestr);
+else
+ret = qemuMonitorTextAddDevice(mon, devicestr);
+return ret;
+}
+
+int qemuMonitorAddDrive(qemuMonitorPtr mon,
+const char *drivestr)
+{
+DEBUG(mon=%p, fd=%d drive=%s, mon, mon-fd, drivestr);
+int ret;
+
+if (mon-json)
+ret = qemuMonitorJSONAddDrive(mon, drivestr);
+else
+ret = qemuMonitorTextAddDrive(mon, drivestr);
+return ret;
+}
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 8a405ce..a330eff 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -285,4 +285,10 @@ struct _qemuMonitorPCIAddress {
int qemuMonitorGetAllPCIAddresses(qemuMonitorPtr mon,
qemuMonitorPCIAddress **addrs);
+int qemuMonitorAddDevice(qemuMonitorPtr mon,
+ const char *devicestr);
+
+int qemuMonitorAddDrive(qemuMonitorPtr mon,
+const char *drivestr);
+
#endif /* QEMU_MONITOR_H */
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 8e88c7e..a556088 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -1785,3 +1785,52 @@ int qemuMonitorJSONGetAllPCIAddresses(qemuMonitorPtr mon
ATTRIBUTE_UNUSED,
_(query-pci not suppported in JSON mode));
return -1;
}
+
+
+int qemuMonitorJSONAddDevice(qemuMonitorPtr mon,
+ const char *devicestr)
+{
+int ret;
+virJSONValuePtr cmd;
+virJSONValuePtr reply = NULL;
+
+cmd = qemuMonitorJSONMakeCommand(device_add,
+ s:config, devicestr,
+ NULL);
+if (!cmd)
+return -1;
+
+ret = qemuMonitorJSONCommand(mon, cmd, reply);
+
+if (ret == 0)
+ret = qemuMonitorJSONCheckError(cmd, reply);
+
+virJSONValueFree(cmd);
+virJSONValueFree(reply);
+return ret;
+}
+
+
+int qemuMonitorJSONAddDrive(qemuMonitorPtr mon,
+const char *drivestr)
+{
+int ret;
+virJSONValuePtr cmd;
+virJSONValuePtr reply = NULL;
+
+cmd = qemuMonitorJSONMakeCommand(drive_add,
+ s:pci_addr, dummy,
+ s:opts, drivestr,
+ NULL);
+if (!cmd)
+return -1;
+
+ret = qemuMonitorJSONCommand(mon, cmd, reply);
+
+if (ret == 0)
+ret = qemuMonitorJSONCheckError(cmd, reply);
+
+virJSONValueFree(cmd);
+virJSONValueFree(reply);
+return ret;
+}
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index 858aac0..ac6458c 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -156,4 +156,10 @@ int qemuMonitorJSONAttachDrive(qemuMonitorPtr mon,
int qemuMonitorJSONGetAllPCIAddresses(qemuMonitorPtr mon,
qemuMonitorPCIAddress **addrs);
+int qemuMonitorJSONAddDevice(qemuMonitorPtr mon,
+ const char *devicestr);
+
+int qemuMonitorJSONAddDrive(qemuMonitorPtr mon,
+const char *drivestr);
+
#endif /* QEMU_MONITOR_JSON_H */
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index 380bcdc..b2a0c53 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -2029,3 +2029,80 @@ error:
#undef SKIP_SPACE
#undef CHECK_END
#undef SKIP_TO
+
+
+int qemuMonitorTextAddDevice(qemuMonitorPtr mon,
+ const char *devicestr)
+{
+char *cmd = NULL;
+char *reply = NULL;
+char *safedev;
+
[libvirt] [PATCH 1/7] Standard syntax for building QEMU command line arguments
All the helper functions for building command line arguments
now return a 'char *', instead of acepting a 'char **' or
virBufferPtr argument
* qemu/qemu_conf.c: Standardize syntax for building args
* qemu/qemu_conf.h: Export all functions for building args
* qemu/qemu_driver.c: Update for changed syntax for building
NIC/hostnet args
---
src/qemu/qemu_conf.c | 341 +++-
src/qemu/qemu_conf.h | 56 ++--
src/qemu/qemu_driver.c |6 +-
3 files changed, 211 insertions(+), 192 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index f4a6c08..e11ec35 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -2056,10 +2056,10 @@ error:
return NULL;
}
-static int
+
+char *
qemuBuildDriveDevStr(virConnectPtr conn,
- virDomainDiskDefPtr disk,
- char **str)
+ virDomainDiskDefPtr disk)
{
virBuffer opt = VIR_BUFFER_INITIALIZER;
const char *bus = virDomainDiskQEMUBusTypeToString(disk-bus);
@@ -2100,17 +2100,20 @@ qemuBuildDriveDevStr(virConnectPtr conn,
virBufferVSprintf(opt, ,drive=drive-%s, disk-info.alias);
virBufferVSprintf(opt, ,id=%s, disk-info.alias);
-*str = virBufferContentAndReset(opt);
-return 0;
+if (virBufferError(opt)) {
+virReportOOMError(NULL);
+goto error;
+}
+
+return virBufferContentAndReset(opt);
error:
virBufferFreeAndReset(opt);
-*str = NULL;
-return -1;
+return NULL;
}
-static char *
+char *
qemuBuildControllerDevStr(virDomainControllerDefPtr def)
{
virBuffer buf = VIR_BUFFER_INITIALIZER;
@@ -2133,8 +2136,10 @@ qemuBuildControllerDevStr(virDomainControllerDefPtr def)
if (qemuBuildDeviceAddressStr(buf, def-info) 0)
goto error;
-if (virBufferError(buf))
+if (virBufferError(buf)) {
+virReportOOMError(NULL);
goto error;
+}
return virBufferContentAndReset(buf);
@@ -2144,14 +2149,14 @@ error:
}
-int
+char *
qemuBuildNicStr(virConnectPtr conn,
virDomainNetDefPtr net,
const char *prefix,
-int vlan,
-char **str)
+int vlan)
{
-if (virAsprintf(str,
+char *str;
+if (virAsprintf(str,
%smacaddr=%02x:%02x:%02x:%02x:%02x:%02x,vlan=%d%s%s%s%s,
prefix ? prefix : ,
net-mac[0], net-mac[1],
@@ -2163,13 +2168,14 @@ qemuBuildNicStr(virConnectPtr conn,
(net-info.alias ? ,name= : ),
(net-info.alias ? net-info.alias : )) 0) {
virReportOOMError(conn);
-return -1;
+return NULL;
}
-return 0;
+return str;
}
-static char *
+
+char *
qemuBuildNicDevStr(virDomainNetDefPtr net)
{
virBuffer buf = VIR_BUFFER_INITIALIZER;
@@ -2204,23 +2210,25 @@ error:
return NULL;
}
-int
+
+char *
qemuBuildHostNetStr(virConnectPtr conn,
virDomainNetDefPtr net,
char type_sep,
int vlan,
-const char *tapfd,
-char **str)
+const char *tapfd)
{
+char *str = NULL;
+
switch (net-type) {
case VIR_DOMAIN_NET_TYPE_NETWORK:
case VIR_DOMAIN_NET_TYPE_BRIDGE:
-if (virAsprintf(str, tap%cfd=%s,vlan=%d%s%s,
+if (virAsprintf(str, tap%cfd=%s,vlan=%d%s%s,
type_sep, tapfd, vlan,
(net-hostnet_name ? ,name= : ),
(net-hostnet_name ? net-hostnet_name : )) 0) {
virReportOOMError(conn);
-return -1;
+return NULL;
}
break;
@@ -2247,10 +2255,10 @@ qemuBuildHostNetStr(virConnectPtr conn,
if (virBufferError(buf)) {
virBufferFreeAndReset(buf);
virReportOOMError(conn);
-return -1;
+return NULL;
}
-*str = virBufferContentAndReset(buf);
+str = virBufferContentAndReset(buf);
}
break;
@@ -2272,7 +2280,7 @@ qemuBuildHostNetStr(virConnectPtr conn,
break;
}
-if (virAsprintf(str, socket%c%s=%s:%d,vlan=%d%s%s,
+if (virAsprintf(str, socket%c%s=%s:%d,vlan=%d%s%s,
type_sep, mode,
net-data.socket.address,
net-data.socket.port,
@@ -2280,40 +2288,41 @@ qemuBuildHostNetStr(virConnectPtr conn,
(net-hostnet_name ? ,name= : ),
(net-hostnet_name ? net-hostnet_name : )) 0)
{
virReportOOMError(conn);
-return -1;
+return NULL;
}
}
break;
case VIR_DOMAIN_NET_TYPE_USER:
default:
-if (virAsprintf(str, user%cvlan=%d%s%s,
+if
[libvirt] [PATCH 6/7] Rewrite way QEMU PCI addresses are allocated
The current QEMU code allocates PCI addresses incrementally starting
at 4. This is not satisfactory because the user may have given some
addresses in their XML config, which need to be skipped over when
allocating addresses to remaining devices.
It is thus neccessary to maintain a list of already allocated PCI
addresses and then only allocate ones that remain unused. This is
also required for domain device hotplug to work properly later.
* src/qemu/qemu_conf.c, src/qemu/qemu_conf.h: Add APIs for creating
list of existing PCI addresses, and allocating new addresses.
Refactor address assignment to use this code
* src/qemu/qemu_driver.c: Pull PCI address assignment up into the
qemuStartVMDaemon() method, as a prelude to moving it into the
'define' method. Update list of allocated addresses when connecting
to a running VM at daemon startup.
---
src/qemu/qemu_conf.c | 228 +---
src/qemu/qemu_conf.h | 12 +++
src/qemu/qemu_driver.c | 23 +
3 files changed, 233 insertions(+), 30 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 85320c1..7de28be 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1587,28 +1587,174 @@ qemuAssignDeviceAliases(virDomainDefPtr def)
}
-static void
-qemuAssignDevicePCISlot(virDomainDeviceInfoPtr info,
-int slot)
+#define QEMU_PCI_ADDRESS_LAST_SLOT 31
+struct _qemuDomainPCIAddressSet {
+virHashTablePtr used;
+int nextslot;
+/* XXX add domain, bus later when QEMU allows 1 */
+};
+
+
+static char *qemuPCIAddressAsString(virDomainDeviceInfoPtr dev)
{
-info-type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
-info-addr.pci.domain = 0;
-info-addr.pci.bus = 0;
-info-addr.pci.slot = slot;
-info-addr.pci.function = 0;
+char *addr;
+
+if (virAsprintf(addr, %d:%d:%d,
+dev-addr.pci.domain,
+dev-addr.pci.bus,
+dev-addr.pci.slot) 0) {
+virReportOOMError(NULL);
+return NULL;
+}
+return addr;
}
-static void
-qemuAssignDevicePCISlots(virDomainDefPtr def)
+
+static int qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
+ virDomainDeviceInfoPtr dev,
+ void *opaque)
+{
+qemuDomainPCIAddressSetPtr addrs = opaque;
+
+if (dev-type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
+char *addr = qemuPCIAddressAsString(dev);
+
+if (virHashAddEntry(addrs-used, addr, addr) 0) {
+VIR_FREE(addr);
+return -1;
+}
+}
+
+return 0;
+}
+
+
+qemuDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def)
+{
+qemuDomainPCIAddressSetPtr addrs;
+
+if (VIR_ALLOC(addrs) 0)
+goto no_memory;
+
+if (!(addrs-used = virHashCreate(10)))
+goto no_memory;
+
+if (virDomainDeviceInfoIterate(def, qemuCollectPCIAddress, addrs) 0)
+goto error;
+
+return addrs;
+
+no_memory:
+virReportOOMError(NULL);
+error:
+qemuDomainPCIAddressSetFree(addrs);
+return NULL;
+}
+
+int qemuDomainPCIAddressReserve(qemuDomainPCIAddressSetPtr addrs,
+int slot)
+{
+virDomainDeviceInfo dev;
+char *addr;
+
+dev.addr.pci.domain = 0;
+dev.addr.pci.bus = 0;
+dev.addr.pci.slot = slot;
+
+addr = qemuPCIAddressAsString(dev);
+if (!addr)
+return -1;
+
+if (virHashLookup(addrs-used, addr)) {
+qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+ _(unable to reserve PCI address %s), addr);
+VIR_FREE(addr);
+return -1;
+}
+
+if (virHashAddEntry(addrs-used, addr, addr)) {
+VIR_FREE(addr);
+return -1;
+}
+
+return 0;
+}
+
+
+static void qemuDomainPCIAddressSetFreeEntry(void *payload, const char *name
ATTRIBUTE_UNUSED)
+{
+VIR_FREE(payload);
+}
+
+
+void qemuDomainPCIAddressSetFree(qemuDomainPCIAddressSetPtr addrs)
+{
+if (!addrs)
+return;
+
+virHashFree(addrs-used, qemuDomainPCIAddressSetFreeEntry);
+}
+
+
+int qemuDomainPCIAddressSetNextAddr(qemuDomainPCIAddressSetPtr addrs,
+virDomainDeviceInfoPtr dev)
{
int i;
-/*
- * slot = 0 - Host bridge
- * slot = 1 - PIIX3 (ISA bridge, IDE controller, something else unknown,
USB controller)
- * slot = 2 - VGA
- * slot = 3 - VirtIO Balloon
- */
-int nextslot = 4;
+
+for (i = addrs-nextslot ; i = QEMU_PCI_ADDRESS_LAST_SLOT ; i++) {
+virDomainDeviceInfo maybe;
+char *addr;
+
+memset(maybe, 0, sizeof(maybe));
+maybe.addr.pci.domain = 0;
+maybe.addr.pci.bus = 0;
+maybe.addr.pci.slot = i;
+
+addr = qemuPCIAddressAsString(maybe);
+
+if (virHashLookup(addrs-used, addr)) {
+VIR_FREE(addr);
+continue;
+}
+
+if (virHashAddEntry(addrs-used,
[libvirt] [PATCH 4/7] Make hotplug use new device_add where possible
Since QEMU startup uses the new -device argument, the hotplug
code needs todo the same.
* src/qemu/qemu_driver.c: Use new device_add monitor APIs
whereever possible
---
src/qemu/qemu_driver.c | 222
1 files changed, 169 insertions(+), 53 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index bf21923..4608a69 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5185,12 +5185,14 @@ error:
static int qemudDomainAttachPciDiskDevice(virConnectPtr conn,
struct qemud_driver *driver,
virDomainObjPtr vm,
- virDomainDiskDefPtr disk)
+ virDomainDiskDefPtr disk,
+ int qemuCmdFlags)
{
int i, ret;
const char* type = virDomainDiskBusTypeToString(disk-bus);
qemuDomainObjPrivatePtr priv = vm-privateData;
-virDomainDevicePCIAddress guestAddr;
+char *devstr = NULL;
+char *drivestr = NULL;
for (i = 0 ; i vm-def-ndisks ; i++) {
if (STREQ(vm-def-disks[i]-dst, disk-dst)) {
@@ -5205,28 +5207,52 @@ static int qemudDomainAttachPciDiskDevice(virConnectPtr
conn,
driver-securityDriver-domainSetSecurityImageLabel(conn, vm, disk)
0)
return -1;
+if (qemuCmdFlags QEMUD_CMD_FLAG_DEVICE) {
+if (!(drivestr = qemuBuildDriveStr(disk, 0, qemuCmdFlags)))
+goto error;
+
+if (!(devstr = qemuBuildDriveDevStr(NULL, disk)))
+goto error;
+}
+
if (VIR_REALLOC_N(vm-def-disks, vm-def-ndisks+1) 0) {
virReportOOMError(conn);
goto error;
}
qemuDomainObjEnterMonitorWithDriver(driver, vm);
-ret = qemuMonitorAddPCIDisk(priv-mon,
-disk-src,
-type,
-guestAddr);
+if (qemuCmdFlags QEMUD_CMD_FLAG_DEVICE) {
+ret = qemuMonitorAddDrive(priv-mon, drivestr);
+if (ret == 0)
+qemuMonitorAddDevice(priv-mon, devstr);
+/* XXX remove the drive upon fail */
+} else {
+virDomainDevicePCIAddress guestAddr;
+ret = qemuMonitorAddPCIDisk(priv-mon,
+disk-src,
+type,
+guestAddr);
+if (ret == 0) {
+disk-info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
+memcpy(disk-info.addr.pci, guestAddr, sizeof(guestAddr));
+}
+}
qemuDomainObjExitMonitorWithDriver(driver, vm);
if (ret 0)
goto error;
-disk-info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
-memcpy(disk-info.addr.pci, guestAddr, sizeof(guestAddr));
virDomainDiskInsertPreAlloced(vm-def, disk);
+VIR_FREE(devstr);
+VIR_FREE(drivestr);
+
return 0;
error:
+VIR_FREE(devstr);
+VIR_FREE(drivestr);
+
if (driver-securityDriver
driver-securityDriver-domainRestoreSecurityImageLabel
driver-securityDriver-domainRestoreSecurityImageLabel(conn, vm,
disk) 0)
@@ -5239,10 +5265,13 @@ error:
static int qemudDomainAttachPciControllerDevice(virConnectPtr conn,
struct qemud_driver *driver,
virDomainObjPtr vm,
-virDomainControllerDefPtr
controller)
+virDomainControllerDefPtr
controller,
+int qemuCmdFlags)
{
-int i, ret;
+int i;
+int ret = -1;
const char* type = virDomainControllerTypeToString(controller-type);
+char *devstr = NULL;
qemuDomainObjPrivatePtr priv = vm-privateData;
for (i = 0 ; i vm-def-ncontrollers ; i++) {
@@ -5255,15 +5284,24 @@ static int
qemudDomainAttachPciControllerDevice(virConnectPtr conn,
}
}
+if (!(devstr = qemuBuildControllerDevStr(controller))) {
+virReportOOMError(NULL);
+goto cleanup;
+}
+
if (VIR_REALLOC_N(vm-def-controllers, vm-def-ncontrollers+1) 0) {
-virReportOOMError(conn);
-return -1;
+virReportOOMError(NULL);
+goto cleanup;
}
qemuDomainObjEnterMonitorWithDriver(driver, vm);
-ret = qemuMonitorAttachPCIDiskController(priv-mon,
- type,
- controller-info.addr.pci);
+if (qemuCmdFlags QEMUD_CMD_FLAG_DEVICE) {
+ret = qemuMonitorAddDevice(priv-mon, devstr);
+} else {
+ret = qemuMonitorAttachPCIDiskController(priv-mon,
+ type,
+ controller-info.addr.pci);
+}
qemuDomainObjExitMonitorWithDriver(driver,
[libvirt] [PATCH 7/7] Assign PCI addresses before hotplugging devices
PCI disk, disk controllers, net devices and host devices need to
have PCI addresses assigned before they are hot-plugged
* src/qemu/qemu_conf.c: Add APIs for ensuring a device has an
address and releasing unused addresses
* src/qemu/qemu_driver.c: Ensure all devices have addresses
when hotplugging.
---
src/qemu/qemu_conf.c | 60 ++-
src/qemu/qemu_conf.h | 11 +++-
src/qemu/qemu_driver.c | 43 --
3 files changed, 97 insertions(+), 17 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 7de28be..c93e473 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1651,17 +1651,12 @@ error:
return NULL;
}
-int qemuDomainPCIAddressReserve(qemuDomainPCIAddressSetPtr addrs,
-int slot)
+int qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr addrs,
+virDomainDeviceInfoPtr dev)
{
-virDomainDeviceInfo dev;
char *addr;
-dev.addr.pci.domain = 0;
-dev.addr.pci.bus = 0;
-dev.addr.pci.slot = slot;
-
-addr = qemuPCIAddressAsString(dev);
+addr = qemuPCIAddressAsString(dev);
if (!addr)
return -1;
@@ -1680,6 +1675,29 @@ int
qemuDomainPCIAddressReserve(qemuDomainPCIAddressSetPtr addrs,
return 0;
}
+int qemuDomainPCIAddressReserveSlot(qemuDomainPCIAddressSetPtr addrs,
+int slot)
+{
+virDomainDeviceInfo dev;
+
+dev.addr.pci.domain = 0;
+dev.addr.pci.bus = 0;
+dev.addr.pci.slot = slot;
+
+return qemuDomainPCIAddressReserveAddr(addrs, dev);
+}
+
+
+int qemuDomainPCIAddressEnsureAddr(qemuDomainPCIAddressSetPtr addrs,
+virDomainDeviceInfoPtr dev)
+{
+int ret = 0;
+if (dev-type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)
+ret = qemuDomainPCIAddressReserveAddr(addrs, dev);
+else
+ret = qemuDomainPCIAddressSetNextAddr(addrs, dev);
+return ret;
+}
static void qemuDomainPCIAddressSetFreeEntry(void *payload, const char *name
ATTRIBUTE_UNUSED)
{
@@ -1687,6 +1705,24 @@ static void qemuDomainPCIAddressSetFreeEntry(void
*payload, const char *name ATT
}
+int qemuDomainPCIAddressReleaseAddr(qemuDomainPCIAddressSetPtr addrs,
+virDomainDeviceInfoPtr dev)
+{
+char *addr;
+int ret;
+
+addr = qemuPCIAddressAsString(dev);
+if (!addr)
+return -1;
+
+ret = virHashRemoveEntry(addrs-used, addr,
qemuDomainPCIAddressSetFreeEntry);
+
+VIR_FREE(addr);
+
+return ret;
+}
+
+
void qemuDomainPCIAddressSetFree(qemuDomainPCIAddressSetPtr addrs)
{
if (!addrs)
@@ -1744,16 +1780,16 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,
qemuDomainPCIAddressSetPtr addrs)
int i;
/* Host bridge */
-if (qemuDomainPCIAddressReserve(addrs, 0) 0)
+if (qemuDomainPCIAddressReserveSlot(addrs, 0) 0)
goto error;
/* PIIX3 (ISA bridge, IDE controller, something else unknown, USB
controller) */
-if (qemuDomainPCIAddressReserve(addrs, 1) 0)
+if (qemuDomainPCIAddressReserveSlot(addrs, 1) 0)
goto error;
/* VGA */
-if (qemuDomainPCIAddressReserve(addrs, 2) 0)
+if (qemuDomainPCIAddressReserveSlot(addrs, 2) 0)
goto error;
/* VirtIO Balloon */
-if (qemuDomainPCIAddressReserve(addrs, 3) 0)
+if (qemuDomainPCIAddressReserveSlot(addrs, 3) 0)
goto error;
for (i = 0; i def-ndisks ; i++) {
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index bcddf3c..b94153f 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -275,10 +275,17 @@ virDomainDefPtr qemuParseCommandLineString(virConnectPtr
conn,
const char *args);
qemuDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def);
-int qemuDomainPCIAddressReserve(qemuDomainPCIAddressSetPtr addrs,
-int slot);
+int qemuDomainPCIAddressReserveSlot(qemuDomainPCIAddressSetPtr addrs,
+int slot);
+int qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr addrs,
+virDomainDeviceInfoPtr dev);
int qemuDomainPCIAddressSetNextAddr(qemuDomainPCIAddressSetPtr addrs,
virDomainDeviceInfoPtr dev);
+int qemuDomainPCIAddressEnsureAddr(qemuDomainPCIAddressSetPtr addrs,
+ virDomainDeviceInfoPtr dev);
+int qemuDomainPCIAddressReleaseAddr(qemuDomainPCIAddressSetPtr addrs,
+virDomainDeviceInfoPtr dev);
+
void qemuDomainPCIAddressSetFree(qemuDomainPCIAddressSetPtr addrs);
int qemuAssignDevicePCISlots(virDomainDefPtr def, qemuDomainPCIAddressSetPtr
addrs);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 465beaf..8debe20 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@
Re: [libvirt] [PATCH 1/1] Cleanups for udev init code
2010/1/27 David Allan dal...@redhat.com:
This patch contains a fix for a segfault when priv is NULL pointed out
by Matthias Bolte.
---
src/node_device/node_device_udev.c | 42 +++
1 files changed, 18 insertions(+), 24 deletions(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index dcd889d..c76c568 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1363,15 +1363,18 @@ static int udevDeviceMonitorShutdown(void)
struct udev_monitor *udev_monitor = NULL;
struct udev *udev = NULL;
- if (driverState) {
+ if (driverState != NULL) {
nodeDeviceLock(driverState);
priv = driverState-privateData;
- if (priv-watch != -1)
- virEventRemoveHandle(priv-watch);
+ if (priv != NULL) {
+ if (priv-watch != -1) {
+ virEventRemoveHandle(priv-watch);
+ }
- udev_monitor = DRV_STATE_UDEV_MONITOR(driverState);
+ udev_monitor = DRV_STATE_UDEV_MONITOR(driverState);
+ }
if (udev_monitor != NULL) {
udev = udev_monitor_get_udev(udev_monitor);
@@ -1387,6 +1390,7 @@ static int udevDeviceMonitorShutdown(void)
virMutexDestroy(driverState-lock);
VIR_FREE(driverState);
VIR_FREE(priv);
+
} else {
ret = -1;
}
@@ -1547,28 +1551,22 @@ static int udevDeviceMonitorStartup(int privileged
ATTRIBUTE_UNUSED)
{
udevPrivate *priv = NULL;
struct udev *udev = NULL;
- int ret = 0;
+ int ret = -1;
- if (VIR_ALLOC(priv) 0) {
+ if (VIR_ALLOC(driverState) 0) {
virReportOOMError(NULL);
- ret = -1;
goto out;
}
- priv-watch = -1;
-
- if (VIR_ALLOC(driverState) 0) {
+ if (VIR_ALLOC(priv) 0) {
virReportOOMError(NULL);
- VIR_FREE(priv);
- ret = -1;
goto out;
}
+ priv-watch = -1;
+
if (virMutexInit(driverState-lock) 0) {
VIR_ERROR0(Failed to initialize mutex for driverState);
- VIR_FREE(priv);
- VIR_FREE(driverState);
- ret = -1;
priv does leak here.
priv is a local variable and it is allocated at this point, but not
assigned to driverState-privateData yet. So goto out will leak it,
because driverState-privateData is still NULL when calling
udevDeviceMonitorShutdown to cleanup.
goto out;
}
@@ -1585,10 +1583,8 @@ static int udevDeviceMonitorStartup(int privileged
ATTRIBUTE_UNUSED)
priv-udev_monitor = udev_monitor_new_from_netlink(udev, udev);
if (priv-udev_monitor == NULL) {
- VIR_FREE(priv);
nodeDeviceUnlock(driverState);
VIR_ERROR0(udev_monitor_new_from_netlink returned NULL);
- ret = -1;
priv does leak here too.
As I said before, moving the driverState-privateData = priv
assignment directly after the allocation and initialization or priv
will fix the leak, because then the call to udevDeviceMonitorShutdown
can free it as the local priv is assigned to driverState-privateData.
goto out;
}
@@ -1608,27 +1604,25 @@ static int udevDeviceMonitorStartup(int privileged
ATTRIBUTE_UNUSED)
priv-watch = virEventAddHandle(udev_monitor_get_fd(priv-udev_monitor),
VIR_EVENT_HANDLE_READABLE,
udevEventHandleCallback, NULL, NULL);
+
+ nodeDeviceUnlock(driverState);
+
if (priv-watch == -1) {
- nodeDeviceUnlock(driverState);
- ret = -1;
goto out;
}
- nodeDeviceUnlock(driverState);
-
/* Create a fictional 'computer' device to root the device tree. */
if (udevSetupSystemDev() != 0) {
- ret = -1;
goto out;
}
/* Populate with known devices */
-
if (udevEnumerateDevices(udev) != 0) {
- ret = -1;
goto out;
}
+ ret = 0;
+
out:
if (ret == -1) {
udevDeviceMonitorShutdown();
--
1.6.5.5
Matthias
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Release 0.4.1 of libvirt-java
Alright.. let me give it a try. I was not seeing the NPE in the tests I ran.
-- bk
On 01/25/2010 01:15 PM, frederic.dangt...@orange-ftgroup.com wrote:
On Jan 18, 2010, at 3:44 PM, Bryan Kearney wrote:
I have just released 0.4.1 of libvirt java. There are 2 main items in
this release:
- Better null checking in for Scheduled Parameters which should fix
the issues reported on the list.
- Error Callbacks to provide better handling of errors encountered by
libvirt (virConnSetErrorFunc and virSetErrorFunc).
You can access the latest version via the following means:
Source Code: http://www.libvirt.org/git/?p=libvirt-java.git;a=summary
Bundled Source (tarball and SRPM): http://libvirt.org/sources/java/
Maven: http://libvirt.org/maven2/
RPMS are making their way through F-11 and F12 build systems
Thank you!
-- bk
--
libvir-list mailing list
libvir-list@redhat.com mailto:libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Thanks for the update, Bryan, but the problem still persists. In my
setup (Xen hosts): Domain.getSchedulerParameters() still raises a NPE.
In libvirt.h, _virSchedParameter is defined as follows using a union for
the value:
struct _virSchedParameter {
char field[VIR_DOMAIN_SCHED_FIELD_LENGTH]; /* parameter name */
int type; /* parameter type */
union {
int i; /* data for integer case */
unsigned int ui; /* data for unsigned integer case */
long long int l; /* data for long long integer case */
unsigned long long int ul; /* data for unsigned long long integer case */
double d; /* data for double case */
char b; /* data for char case */
} value; /* parameter value */
};
I believe the Java mapping of virSchedParameterValue should be a Union
instead of a Structure:
public class virSchedParameterValue extends Union {...}
Regards,
Frederic
--
Orange Labs
38-40 rue du General Leclerc
92794 Issy Moulineaux Cedex 9
FRANCE
*
This message and any attachments (the message) are confidential and intended
solely for the addressees.
Any unauthorised use or dissemination is prohibited.
Messages are susceptible to alteration.
France Telecom Group shall not be liable for the message if altered, changed or
falsified.
If you are not the intended addressee of this message, please cancel it
immediately and inform the sender.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/1] Cleanups for udev init code
On 01/27/2010 02:39 PM, Matthias Bolte wrote:
2010/1/27 David Allandal...@redhat.com:
This patch contains a fix for a segfault when priv is NULL pointed out
by Matthias Bolte.
---
src/node_device/node_device_udev.c | 42 +++
1 files changed, 18 insertions(+), 24 deletions(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index dcd889d..c76c568 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1363,15 +1363,18 @@ static int udevDeviceMonitorShutdown(void)
struct udev_monitor *udev_monitor = NULL;
struct udev *udev = NULL;
-if (driverState) {
+if (driverState != NULL) {
nodeDeviceLock(driverState);
priv = driverState-privateData;
-if (priv-watch != -1)
-virEventRemoveHandle(priv-watch);
+if (priv != NULL) {
+if (priv-watch != -1) {
+virEventRemoveHandle(priv-watch);
+}
-udev_monitor = DRV_STATE_UDEV_MONITOR(driverState);
+udev_monitor = DRV_STATE_UDEV_MONITOR(driverState);
+}
if (udev_monitor != NULL) {
udev = udev_monitor_get_udev(udev_monitor);
@@ -1387,6 +1390,7 @@ static int udevDeviceMonitorShutdown(void)
virMutexDestroy(driverState-lock);
VIR_FREE(driverState);
VIR_FREE(priv);
+
} else {
ret = -1;
}
@@ -1547,28 +1551,22 @@ static int udevDeviceMonitorStartup(int privileged
ATTRIBUTE_UNUSED)
{
udevPrivate *priv = NULL;
struct udev *udev = NULL;
-int ret = 0;
+int ret = -1;
-if (VIR_ALLOC(priv) 0) {
+if (VIR_ALLOC(driverState) 0) {
virReportOOMError(NULL);
-ret = -1;
goto out;
}
-priv-watch = -1;
-
-if (VIR_ALLOC(driverState) 0) {
+if (VIR_ALLOC(priv) 0) {
virReportOOMError(NULL);
-VIR_FREE(priv);
-ret = -1;
goto out;
}
+priv-watch = -1;
+
if (virMutexInit(driverState-lock) 0) {
VIR_ERROR0(Failed to initialize mutex for driverState);
-VIR_FREE(priv);
-VIR_FREE(driverState);
-ret = -1;
priv does leak here.
priv is a local variable and it is allocated at this point, but not
assigned to driverState-privateData yet. So goto out will leak it,
because driverState-privateData is still NULL when calling
udevDeviceMonitorShutdown to cleanup.
Indeed, you are correct, and I am clearly rushing through this.
goto out;
}
@@ -1585,10 +1583,8 @@ static int udevDeviceMonitorStartup(int privileged
ATTRIBUTE_UNUSED)
priv-udev_monitor = udev_monitor_new_from_netlink(udev, udev);
if (priv-udev_monitor == NULL) {
-VIR_FREE(priv);
nodeDeviceUnlock(driverState);
VIR_ERROR0(udev_monitor_new_from_netlink returned NULL);
-ret = -1;
priv does leak here too.
As I said before, moving the driverState-privateData = priv
assignment directly after the allocation and initialization or priv
will fix the leak, because then the call to udevDeviceMonitorShutdown
can free it as the local priv is assigned to driverState-privateData.
goto out;
}
@@ -1608,27 +1604,25 @@ static int udevDeviceMonitorStartup(int privileged
ATTRIBUTE_UNUSED)
priv-watch = virEventAddHandle(udev_monitor_get_fd(priv-udev_monitor),
VIR_EVENT_HANDLE_READABLE,
udevEventHandleCallback, NULL, NULL);
+
+nodeDeviceUnlock(driverState);
+
if (priv-watch == -1) {
-nodeDeviceUnlock(driverState);
-ret = -1;
goto out;
}
-nodeDeviceUnlock(driverState);
-
/* Create a fictional 'computer' device to root the device tree. */
if (udevSetupSystemDev() != 0) {
-ret = -1;
goto out;
}
/* Populate with known devices */
-
if (udevEnumerateDevices(udev) != 0) {
-ret = -1;
goto out;
}
+ret = 0;
+
out:
if (ret == -1) {
udevDeviceMonitorShutdown();
--
1.6.5.5
Matthias
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
