Re: [libvirt] [PATCH 1/1] Fix the crash when seclable is freed
On 2013年04月02日 14:47, Osier Yang wrote:
On 02/04/13 13:58, Li Zhang wrote:
From: Li Zhang
When seclabel's type is VIR_DOMAIN_SECLABEL_NONE,
virSecurityLabelDefPtr's members are not allocated.
So it will cause crash when calling VIR_FREE.
This problem is found when running autotest on PPC.
Failed to remove cgroup for virt-tests-vm1
*** glibc detected *** /usr/sbin/libvirtd: free(): invalid pointer:
0x3fff9c187510 ***
=== Backtrace: =
/lib64/libc.so.6(+0xb89c4)[0x3fffa9bc89c4]
/lib64/libvirt.so.0(virFree-0x3e2320)[0x3fffaa82e9c0]
/lib64/libvirt.so.0(virSecurityLabelDefFree-0x378984)[0x3fffaa89d69c]
/lib64/libvirt.so.0(virDomainDefFree-0x367c98)[0x3fffaa8ae968]
/usr/lib64/libvirt/connection-driver/libvirt_driver_qemu.so(qemuProcessStop-0xc85f8)[0x3fffa2899d58]
/usr/lib64/libvirt/connection-driver/libvirt_driver_qemu.so(+0xc3668)[0x3fffa28e3668]
/lib64/libvirt.so.0(virDomainDestroy-0x309bd0)[0x3fffaa90f6f0]
/usr/sbin/libvirtd[0x10035230]
/lib64/libvirt.so.0(virNetServerProgramDispatch-0x289b50)[0x3fffaa995930]
/lib64/libvirt.so.0(+0x20db18)[0x3fffaa98db18]
/lib64/libvirt.so.0(+0xfbd24)[0x3fffaa87bd24]
/lib64/libvirt.so.0(+0xfaec8)[0x3fffaa87aec8]
/lib64/libpthread.so.0(+0xc604)[0x3fffa9d7c604]
/lib64/libc.so.6(clone-0xb8fe4)[0x3fffa9c3f094]
Signed-off-by: Li Zhang
---
src/conf/domain_conf.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f3fca7f..2856660 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1006,6 +1006,8 @@ virSecurityLabelDefFree(virSecurityLabelDefPtr
def)
{
if (!def)
return;
+ if (def->type == VIR_DOMAIN_SECLABEL_NONE)
+ return;
VIR_FREE(def->model);
model is always parsed. So it will be leaked if someone specifies
"model" even the type is "none".
Okay, it seems that is not always non-NULL although it is always parsed.
My XML file is as:
It should be better to add if clause before VIR_FREE.
VIR_FREE(def->label);
VIR_FREE(def->imagelabel);
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/1] Fix the crash when seclable is freed
On 2013年04月02日 15:03, Li Zhang wrote:
On 2013年04月02日 14:47, Osier Yang wrote:
On 02/04/13 13:58, Li Zhang wrote:
From: Li Zhang
When seclabel's type is VIR_DOMAIN_SECLABEL_NONE,
virSecurityLabelDefPtr's members are not allocated.
So it will cause crash when calling VIR_FREE.
This problem is found when running autotest on PPC.
Failed to remove cgroup for virt-tests-vm1
*** glibc detected *** /usr/sbin/libvirtd: free(): invalid pointer:
0x3fff9c187510 ***
=== Backtrace: =
/lib64/libc.so.6(+0xb89c4)[0x3fffa9bc89c4]
/lib64/libvirt.so.0(virFree-0x3e2320)[0x3fffaa82e9c0]
/lib64/libvirt.so.0(virSecurityLabelDefFree-0x378984)[0x3fffaa89d69c]
/lib64/libvirt.so.0(virDomainDefFree-0x367c98)[0x3fffaa8ae968]
/usr/lib64/libvirt/connection-driver/libvirt_driver_qemu.so(qemuProcessStop-0xc85f8)[0x3fffa2899d58]
/usr/lib64/libvirt/connection-driver/libvirt_driver_qemu.so(+0xc3668)[0x3fffa28e3668]
/lib64/libvirt.so.0(virDomainDestroy-0x309bd0)[0x3fffaa90f6f0]
/usr/sbin/libvirtd[0x10035230]
/lib64/libvirt.so.0(virNetServerProgramDispatch-0x289b50)[0x3fffaa995930]
/lib64/libvirt.so.0(+0x20db18)[0x3fffaa98db18]
/lib64/libvirt.so.0(+0xfbd24)[0x3fffaa87bd24]
/lib64/libvirt.so.0(+0xfaec8)[0x3fffaa87aec8]
/lib64/libpthread.so.0(+0xc604)[0x3fffa9d7c604]
/lib64/libc.so.6(clone-0xb8fe4)[0x3fffa9c3f094]
Signed-off-by: Li Zhang
---
src/conf/domain_conf.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f3fca7f..2856660 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1006,6 +1006,8 @@ virSecurityLabelDefFree(virSecurityLabelDefPtr
def)
{
if (!def)
return;
+ if (def->type == VIR_DOMAIN_SECLABEL_NONE)
+ return;
VIR_FREE(def->model);
model is always parsed. So it will be leaked if someone specifies
"model" even the type is "none".
Okay, it seems that is not always non-NULL although it is always parsed.
My XML file is as:
It should be better to add if clause before VIR_FREE.
Sorry, it seems that this is not right solution to resolve the problem.
free(ptr): ptr can be NULL.
This root cause is because of invalid pointer which is not NULL.
I need to find out the invalid pointer.
VIR_FREE(def->label);
VIR_FREE(def->imagelabel);
--
Li Zhang
IBM China Linux Technology Centre
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/1] Fix the crash when seclable is freed
On 02/04/13 16:04, Li Zhang wrote:
On 2013年04月02日 15:03, Li Zhang wrote:
On 2013年04月02日 14:47, Osier Yang wrote:
On 02/04/13 13:58, Li Zhang wrote:
From: Li Zhang
When seclabel's type is VIR_DOMAIN_SECLABEL_NONE,
virSecurityLabelDefPtr's members are not allocated.
So it will cause crash when calling VIR_FREE.
This problem is found when running autotest on PPC.
Failed to remove cgroup for virt-tests-vm1
*** glibc detected *** /usr/sbin/libvirtd: free(): invalid pointer:
0x3fff9c187510 ***
=== Backtrace: =
/lib64/libc.so.6(+0xb89c4)[0x3fffa9bc89c4]
/lib64/libvirt.so.0(virFree-0x3e2320)[0x3fffaa82e9c0]
/lib64/libvirt.so.0(virSecurityLabelDefFree-0x378984)[0x3fffaa89d69c]
/lib64/libvirt.so.0(virDomainDefFree-0x367c98)[0x3fffaa8ae968]
/usr/lib64/libvirt/connection-driver/libvirt_driver_qemu.so(qemuProcessStop-0xc85f8)[0x3fffa2899d58]
/usr/lib64/libvirt/connection-driver/libvirt_driver_qemu.so(+0xc3668)[0x3fffa28e3668]
/lib64/libvirt.so.0(virDomainDestroy-0x309bd0)[0x3fffaa90f6f0]
/usr/sbin/libvirtd[0x10035230]
/lib64/libvirt.so.0(virNetServerProgramDispatch-0x289b50)[0x3fffaa995930]
/lib64/libvirt.so.0(+0x20db18)[0x3fffaa98db18]
/lib64/libvirt.so.0(+0xfbd24)[0x3fffaa87bd24]
/lib64/libvirt.so.0(+0xfaec8)[0x3fffaa87aec8]
/lib64/libpthread.so.0(+0xc604)[0x3fffa9d7c604]
/lib64/libc.so.6(clone-0xb8fe4)[0x3fffa9c3f094]
Signed-off-by: Li Zhang
---
src/conf/domain_conf.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f3fca7f..2856660 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1006,6 +1006,8 @@
virSecurityLabelDefFree(virSecurityLabelDefPtr def)
{
if (!def)
return;
+ if (def->type == VIR_DOMAIN_SECLABEL_NONE)
+ return;
VIR_FREE(def->model);
model is always parsed. So it will be leaked if someone specifies
"model" even the type is "none".
Okay, it seems that is not always non-NULL although it is always parsed.
My XML file is as:
It should be better to add if clause before VIR_FREE.
Sorry, it seems that this is not right solution to resolve the problem.
free(ptr): ptr can be NULL.
This root cause is because of invalid pointer which is not NULL.
I need to find out the invalid pointer.
The solution is not to parse "model" when the seclabel type is "none".
I might not be 100% correct. But I think the "model" is just useless
for "none". Then your patch is right.
VIR_FREE(def->label);
VIR_FREE(def->imagelabel);
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/1] Fix the crash when seclable is freed
On 2013年04月02日 16:45, Osier Yang wrote:
On 02/04/13 16:04, Li Zhang wrote:
On 2013年04月02日 15:03, Li Zhang wrote:
On 2013年04月02日 14:47, Osier Yang wrote:
On 02/04/13 13:58, Li Zhang wrote:
From: Li Zhang
When seclabel's type is VIR_DOMAIN_SECLABEL_NONE,
virSecurityLabelDefPtr's members are not allocated.
So it will cause crash when calling VIR_FREE.
This problem is found when running autotest on PPC.
Failed to remove cgroup for virt-tests-vm1
*** glibc detected *** /usr/sbin/libvirtd: free(): invalid
pointer: 0x3fff9c187510 ***
=== Backtrace: =
/lib64/libc.so.6(+0xb89c4)[0x3fffa9bc89c4]
/lib64/libvirt.so.0(virFree-0x3e2320)[0x3fffaa82e9c0]
/lib64/libvirt.so.0(virSecurityLabelDefFree-0x378984)[0x3fffaa89d69c]
/lib64/libvirt.so.0(virDomainDefFree-0x367c98)[0x3fffaa8ae968]
/usr/lib64/libvirt/connection-driver/libvirt_driver_qemu.so(qemuProcessStop-0xc85f8)[0x3fffa2899d58]
/usr/lib64/libvirt/connection-driver/libvirt_driver_qemu.so(+0xc3668)[0x3fffa28e3668]
/lib64/libvirt.so.0(virDomainDestroy-0x309bd0)[0x3fffaa90f6f0]
/usr/sbin/libvirtd[0x10035230]
/lib64/libvirt.so.0(virNetServerProgramDispatch-0x289b50)[0x3fffaa995930]
/lib64/libvirt.so.0(+0x20db18)[0x3fffaa98db18]
/lib64/libvirt.so.0(+0xfbd24)[0x3fffaa87bd24]
/lib64/libvirt.so.0(+0xfaec8)[0x3fffaa87aec8]
/lib64/libpthread.so.0(+0xc604)[0x3fffa9d7c604]
/lib64/libc.so.6(clone-0xb8fe4)[0x3fffa9c3f094]
Signed-off-by: Li Zhang
---
src/conf/domain_conf.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f3fca7f..2856660 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1006,6 +1006,8 @@
virSecurityLabelDefFree(virSecurityLabelDefPtr def)
{
if (!def)
return;
+ if (def->type == VIR_DOMAIN_SECLABEL_NONE)
+ return;
VIR_FREE(def->model);
model is always parsed. So it will be leaked if someone specifies
"model" even the type is "none".
Okay, it seems that is not always non-NULL although it is always
parsed.
My XML file is as:
It should be better to add if clause before VIR_FREE.
Sorry, it seems that this is not right solution to resolve the problem.
free(ptr): ptr can be NULL.
This root cause is because of invalid pointer which is not NULL.
I need to find out the invalid pointer.
The solution is not to parse "model" when the seclabel type is "none".
I might not be 100% correct. But I think the "model" is just useless
for "none". Then your patch is right.
Originally, I was thinking about when model is not specified, then model
is NULL.
And free(NULL) is not right. I look into the lib, free(NULL) can work.
I think the invalid pointer which is not NULL should be one wild pointer
here.
I need to verify it on my machine with autotest.
VIR_FREE(def->label);
VIR_FREE(def->imagelabel);
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Libvirt-announce] Release of libvirt-1.0.4
On Mon, Apr 1, 2013 at 7:58 PM, Justin Clift wrote: > Ouch. It looks like something is causing the LXC building code to > be run, even on OSX where there's no chance it would work. :( > > For good measure, I just tried it with the 1.0.4 tarball and had > the same problem (when enabling qemu after modifying configure). > > Someone (not me) would have to investigate why the LXC stuff is > trying to compile. It's probably just something in the configure.ac > or maybe some ifdef statements needed in the right places in the > code. (no idea) > > Don't suppose that's your kind of thing? :) > Ah, this nothing to do with enabling qemu, but everything with Apple's rpcgen which seems to be broken. This has been reported earlier as https://www.redhat.com/archives/libvirt-users/2013-February/msg00033.html Kind regards, Ruben -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [libvirt-designer 2/3] Accept 'iso' format in gvir_designer_domain_add_disk_full()
We need this to be able to mark the device we are creating as a
CDROM.
---
examples/virtxml.c | 2 +-
libvirt-designer/libvirt-designer-domain.c | 8
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/examples/virtxml.c b/examples/virtxml.c
index d4a5fe2..9a36142 100644
--- a/examples/virtxml.c
+++ b/examples/virtxml.c
@@ -712,7 +712,7 @@ Domain with Fedora 17 from locally stored ISO and one NIC
with mac
To add multiple devices just use appropriate argument multiple times:
- # virtxml -d /tmp/Fedora-17-x86_64-Live-KDE.iso,raw \
+ # virtxml -d /tmp/Fedora-17-x86_64-Live-KDE.iso,iso \
-d /var/lib/libvirt/images/f17.img,qcow2 \
-i default,mac=00:11:22:33:44:55,link=down \
-i blue_network \
diff --git a/libvirt-designer/libvirt-designer-domain.c
b/libvirt-designer/libvirt-designer-domain.c
index 0d47d3c..5da8dd3 100644
--- a/libvirt-designer/libvirt-designer-domain.c
+++ b/libvirt-designer/libvirt-designer-domain.c
@@ -894,6 +894,14 @@ gvir_designer_domain_add_disk_full(GVirDesignerDomain
*design,
gvir_config_domain_disk_set_type(disk, type);
gvir_config_domain_disk_set_source(disk, path);
gvir_config_domain_disk_set_driver_name(disk, driver_name);
+if (g_strcmp0(format, "iso") == 0) {
+/* FIXME: Should probably reorder the disk devices so that floppies
+ * go first, then disks, then CDROMs
+ */
+gvir_config_domain_disk_set_guest_device_type(disk,
+
GVIR_CONFIG_DOMAIN_DISK_GUEST_DEVICE_CDROM);
+format = "raw";
+}
if (format)
gvir_config_domain_disk_set_driver_type(disk, format);
if (g_str_equal(bus_str, "ide")) {
--
1.8.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [libvirt-designer 0/3] A few more cleanups
Hey, here are a few small patches for libvirt-designer to fix a few leaks/polish minor things. Christophe -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [libvirt-designer 3/3] virtxml: Fix 2 memory leaks
---
examples/virtxml.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/examples/virtxml.c b/examples/virtxml.c
index 9a36142..1e9f78a 100644
--- a/examples/virtxml.c
+++ b/examples/virtxml.c
@@ -178,6 +178,7 @@ add_disk(gpointer data,
gpointer user_data)
{
GVirDesignerDomain *domain = (GVirDesignerDomain *) user_data;
+GVirConfigDomainDisk *disk;
char *path = (char *) data;
char *format = NULL;
struct stat buf;
@@ -196,10 +197,12 @@ add_disk(gpointer data,
if (!stat(path, &buf) &&
!S_ISREG(buf.st_mode)) {
-gvir_designer_domain_add_disk_device(domain, path, &error);
+disk = gvir_designer_domain_add_disk_device(domain, path, &error);
} else {
-gvir_designer_domain_add_disk_file(domain, path, format, &error);
+disk = gvir_designer_domain_add_disk_file(domain, path, format,
&error);
}
+if (disk)
+g_object_unref(G_OBJECT(disk));
if (error) {
print_error("%s", error->message);
@@ -493,7 +496,7 @@ main(int argc, char *argv[])
static char *connect_uri = NULL;
static char *resources_str = NULL;
GVirDesignerDomainResources resources;
-GOptionContext *context;
+GOptionContext *context = NULL;
static GOptionEntry entries[] =
{
@@ -603,6 +606,8 @@ main(int argc, char *argv[])
ret = EXIT_SUCCESS;
cleanup:
+if (context)
+g_option_context_free(context);
if (os)
g_object_unref(G_OBJECT(os));
if (platform)
--
1.8.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [libvirt-designer 1/3] Fix some wrong gtk-doc transfer annotations
--- libvirt-designer/libvirt-designer-domain.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/libvirt-designer/libvirt-designer-domain.c b/libvirt-designer/libvirt-designer-domain.c index 4805102..0d47d3c 100644 --- a/libvirt-designer/libvirt-designer-domain.c +++ b/libvirt-designer/libvirt-designer-domain.c @@ -946,7 +946,7 @@ error: * * Add a new disk to the domain. * - * Returns: (transfer none): the pointer to new disk. + * Returns: (transfer full): the pointer to new disk. * If something fails NULL is returned and @error is set. */ GVirConfigDomainDisk *gvir_designer_domain_add_disk_file(GVirDesignerDomain *design, @@ -976,7 +976,7 @@ GVirConfigDomainDisk *gvir_designer_domain_add_disk_file(GVirDesignerDomain *des * * Add given device as a new disk to the domain designer instance. * - * Returns: (transfer none): the pointer to the new disk. + * Returns: (transfer full): the pointer to the new disk. * If something fails NULL is returned and @error is set. */ GVirConfigDomainDisk *gvir_designer_domain_add_disk_device(GVirDesignerDomain *design, @@ -1059,7 +1059,7 @@ cleanup: * Add new network interface card into @design. The interface is * of 'network' type with @network used as the source network. * - * Returns: (transfer none): the pointer to the new interface. + * Returns: (transfer full): the pointer to the new interface. */ GVirConfigDomainInterface * gvir_designer_domain_add_interface_network(GVirDesignerDomain *design, -- 1.8.1.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 1/2] Optimize machine option to set more options with it
On Fri, Mar 29, 2013 at 01:22:46PM +0800, Li Zhang wrote: > From: Li Zhang > > Currently, -machine option is used only when dump-guest-core is set. > > To use options defined in machine option for newer version of QEMU, > it needs to use -machine xxx, and to be compatible with older version > -M, this patch addes QEMU_CAPS_MACHINE_OPT capability for newer > version which supports -machine option. > > Signed-off-by: Li Zhang > --- > v3 -> v2: >* Set QEMU_CAPS_MACHINE_OPT with help string >* Set QEMU_CAPS_DUMP_GUEST_CORE with QMP > > v2 -> v1: >* Split the patch to 2 parts suggested by Daniel P.Berrange >* Rename QEMU_CAPS_MACH_OPT to QEMU_CAPS_MACHINE_OPT >* Remove version 1.1 assertion for QEMU_CAPS_MACHINE_OPT > > src/qemu/qemu_capabilities.c | 11 +++ > src/qemu/qemu_capabilities.h |1 + > src/qemu/qemu_command.c | 32 ++-- > tests/qemuhelptest.c |4 > tests/qemuxml2argvtest.c |6 +++--- > 5 files changed, 37 insertions(+), 17 deletions(-) ACK Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Support multiple queue virtio-scsi
- Messaggio originale - > Da: "Eric Blake" > A: "Osier Yang" > Cc: libvir-list@redhat.com, pbonz...@redhat.com > Inviato: Lunedì, 1 aprile 2013 18:43:11 > Oggetto: Re: [libvirt] [PATCH] qemu: Support multiple queue virtio-scsi > > On 04/01/2013 10:31 AM, Osier Yang wrote: > > No, multiple queue is not enabled by default. For "what happens if it > > doesn't match the number of vcpus", honestly, I'm not sure about it, > > my understanding is it doesn't have to match. But matching will have > > the best performance, and with this thought I think an attribute allows > > the user to specify the number makes sense instead of a boolean > > attribute. @paolo, can you confirm or deny this? > > Then maybe all we need is just a recommendation that matching the number > of vcpus tends to give best performance, but not stating that it is > mandatory to match. I think in the current patches anything that doesn't exactly matches the number of VCPUs will fallback to single-queue. But it doesn't have to be the case, so a numeric attribute is preferrable. Paolo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 2/2] Add USB option capability
On Fri, Mar 29, 2013 at 01:22:47PM +0800, Li Zhang wrote: > From: Li Zhang > > To avoid the collision for creating USB controllers in machine->init() > and -device xx command line, it needs to set usb=off to avoid one USB > controller created in machine->init(). So that libvirt can use -device > or -usb to create USB controller sucessfully. > So QEMU_CAPS_MACHINE_USB_OPT capability is added, and it is for QEMU > v1.3.0 onwards which supports USB option. > > Signed-off-by: Li Zhang I'm not seeing why this is needed - we pass -nodefconfig and -nodefaults which ought to disable any default built-in USB controller already surely ? Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 06/10] qemu: Build qemu command line for scsi-generic
On 04/01/2013 08:00 PM, Han Cheng wrote:
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index eac72c2..e1af64f 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -557,7 +557,7 @@ qemuAssignDeviceDiskAliasCustom(virDomainDefPtr def,
if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) {
if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) {
controllerModel =
-virDomainInfoFindControllerModel(def,&disk->info,
+virDomainDeviceFindControllerModel(def,&disk->info,
VIR_DOMAIN_CONTROLLER_TYPE_SCSI);
if ((qemuSetScsiControllerModel(def, qemuCaps,&controllerModel))<
0)
Oh, God. I changed the reference place here. It must be a rebase mistake.
@@ -3179,7 +3188,7 @@ qemuBuildDriveDevStr(virDomainDefPtr def,
}
controllerModel =
-virDomainInfoFindControllerModel(def,&disk->info,
+virDomainDeviceFindControllerModel(def,&disk->info,
VIR_DOMAIN_CONTROLLER_TYPE_SCSI);
if ((qemuSetScsiControllerModel(def, qemuCaps,&controllerModel))< 0)
goto error;
And here.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] smartcard: spell ccid-card-emulated qemu property correctly
On Mon, Apr 01, 2013 at 04:57:00PM -0600, Eric Blake wrote:
> Reported by Anthony Messina in
> https://bugzilla.redhat.com/show_bug.cgi?id=904692
> Present since introduction of smartcard support in commit f5fd9baa
>
> * src/qemu/qemu_command.c (qemuBuildCommandLine): Match qemu spelling.
> ---
>
> No one has spotted my bug, latent since Feb 2011, until now. I
> guess smartcard use is not very common...
>
> src/qemu/qemu_command.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index a0c278f..59a6061 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -6843,7 +6843,7 @@ qemuBuildCommandLine(virConnectPtr conn,
> } else {
> database = VIR_DOMAIN_SMARTCARD_DEFAULT_DATABASE;
> }
> -virBufferAsprintf(&opt, ",database=%s", database);
> +virBufferAsprintf(&opt, ",db=%s", database);
> break;
>
> case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
ACK,
Is a test case worthwhile ? It wouldn't have spotted this if the test
case had had the same typo though
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-designer 3/3] virtxml: Fix 2 memory leaks
On Tue, Apr 02, 2013 at 11:45:11AM +0200, Christophe Fergeau wrote: > --- > examples/virtxml.c | 11 --- > 1 file changed, 8 insertions(+), 3 deletions(-) ACK Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-designer 1/3] Fix some wrong gtk-doc transfer annotations
On Tue, Apr 02, 2013 at 11:45:09AM +0200, Christophe Fergeau wrote: > --- > libvirt-designer/libvirt-designer-domain.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) ACK Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-designer 2/3] Accept 'iso' format in gvir_designer_domain_add_disk_full()
On Tue, Apr 02, 2013 at 11:45:10AM +0200, Christophe Fergeau wrote:
> We need this to be able to mark the device we are creating as a
> CDROM.
> ---
> examples/virtxml.c | 2 +-
> libvirt-designer/libvirt-designer-domain.c | 8
> 2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/examples/virtxml.c b/examples/virtxml.c
> index d4a5fe2..9a36142 100644
> --- a/examples/virtxml.c
> +++ b/examples/virtxml.c
> @@ -712,7 +712,7 @@ Domain with Fedora 17 from locally stored ISO and one NIC
> with mac
>
> To add multiple devices just use appropriate argument multiple times:
>
> - # virtxml -d /tmp/Fedora-17-x86_64-Live-KDE.iso,raw \
> + # virtxml -d /tmp/Fedora-17-x86_64-Live-KDE.iso,iso \
> -d /var/lib/libvirt/images/f17.img,qcow2 \
> -i default,mac=00:11:22:33:44:55,link=down \
> -i blue_network \
> diff --git a/libvirt-designer/libvirt-designer-domain.c
> b/libvirt-designer/libvirt-designer-domain.c
> index 0d47d3c..5da8dd3 100644
> --- a/libvirt-designer/libvirt-designer-domain.c
> +++ b/libvirt-designer/libvirt-designer-domain.c
> @@ -894,6 +894,14 @@ gvir_designer_domain_add_disk_full(GVirDesignerDomain
> *design,
> gvir_config_domain_disk_set_type(disk, type);
> gvir_config_domain_disk_set_source(disk, path);
> gvir_config_domain_disk_set_driver_name(disk, driver_name);
> +if (g_strcmp0(format, "iso") == 0) {
> +/* FIXME: Should probably reorder the disk devices so that floppies
> + * go first, then disks, then CDROMs
> + */
> +gvir_config_domain_disk_set_guest_device_type(disk,
> +
> GVIR_CONFIG_DOMAIN_DISK_GUEST_DEVICE_CDROM);
> +format = "raw";
> +}
I don't think it is a good idea to overload 'format' for this purpose.
It is perfectly acceptable to back a CDROM device by a qcow2 files.
I think we should just have a gvir_designer_domain_add_cdrom() method
or some other indicator
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 04/11] Helper functions for host TPM support
On 04/01/2013 04:07 PM, Corey Bryant wrote:
On 03/21/2013 11:42 AM, Stefan Berger wrote:
Signed-off-by: Stefan Berger
+
+char *
+virTPMFindCancelPath(void)
+{
+unsigned int idx;
+int len;
+DIR *pnp_dir;
+char path[100], *p;
Is there any reason not to use PATH_MAX instead of 100 here?
You get a compiler error then due to the function using too much space
on the stack.
Stefan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 04/11] Helper functions for host TPM support
On Tue, Apr 02, 2013 at 06:23:55AM -0400, Stefan Berger wrote:
> On 04/01/2013 04:07 PM, Corey Bryant wrote:
> >
> >
> >On 03/21/2013 11:42 AM, Stefan Berger wrote:
> >>Signed-off-by: Stefan Berger
> >>
> >>+
> >>+char *
> >>+virTPMFindCancelPath(void)
> >>+{
> >>+unsigned int idx;
> >>+int len;
> >>+DIR *pnp_dir;
> >>+char path[100], *p;
> >
> >Is there any reason not to use PATH_MAX instead of 100 here?
>
> You get a compiler error then due to the function using too much
> space on the stack.
The answer to that is *NOT* to use a shorter 100 byte buffer,
but to use virAsprintf() instead of snprintf(). Never use
fixed length buffers for filepaths.
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] qemu: Error out if the bitmap for pinning is all clear
On 04/02/2013 01:42 AM, Osier Yang wrote: > For both "live" and "config" changes of vcpupin and emulatorpin, an > all clear bitmap doesn't make sense, and it can just cause corruptions. > E.g (similar for emulatorpin). > > % virsh vcpupin hame 0 8,^8 --config > > % virsh vcpupin hame > VCPU: CPU Affinity > -- >0: >1: 0-63 >2: 0-63 >3: 0-63 > > % virsh dumpxml hame |grep cpuset > > > % virsh start ham > error: Failed to start domain hame > error: An error occurred, but the cause is unknown > --- > src/qemu/qemu_driver.c | 12 > 1 file changed, 12 insertions(+) > ACK - nit - provide the "new" error (also your virsh start line is missing the 'e' on ham... John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] virsh: Prohibit all clear cpumap
On 04/02/2013 01:42 AM, Osier Yang wrote:
> This prohibits all clear cpumap eariler in virsh, for both vcpupin
> and emulatorpin.
> ---
> tools/virsh-domain.c | 35 ++-
> 1 file changed, 30 insertions(+), 5 deletions(-)
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 965f92c..8d3d63f 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -5552,6 +5552,20 @@ cleanup:
> }
>
> static bool
> +vshIsCpumapAllClear(const unsigned char *cpumap,
> +int maxcpu)
> +{
> +int i;
> +
> +for (i = 0; i < maxcpu; i++) {
> +if (VIR_CPU_USED(cpumap, i))
> +return false;
> +}
> +
> +return true;
> +}
> +
> +static bool
> cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
> {
> virDomainInfo info;
> @@ -5620,7 +5634,7 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
>
> cpumaplen = VIR_CPU_MAPLEN(maxcpu);
>
> -/* Query mode: show CPU affinity information then exit.*/
> +/* Query mode: show CPU affinity information then exit. */
> if (query) {
> /* When query mode and neither "live", "config" nor "current"
> * is specified, set VIR_DOMAIN_AFFECT_CURRENT as flags */
> @@ -5649,10 +5663,15 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
> goto cleanup;
> }
>
> -/* Pin mode: pinning specified vcpu to specified physical cpus*/
> +/* Pin mode: pinning specified vcpu to specified physical cpus. */
> if (!(cpumap = vshParseCPUList(ctl, cpulist, maxcpu, cpumaplen)))
> goto cleanup;
>
> +if (vshIsCpumapAllClear(cpumap, maxcpu)) {
> +vshError(ctl, "%s", _("No CPU specified"));
> +goto cleanup;
> +}
> +
> if (flags == -1) {
> if (virDomainPinVcpu(dom, vcpu, cpumap, cpumaplen) != 0)
> goto cleanup;
> @@ -5755,10 +5774,11 @@ cmdEmulatorPin(vshControl *ctl, const vshCmd *cmd)
>
> cpumaplen = VIR_CPU_MAPLEN(maxcpu);
>
> -/* Query mode: show CPU affinity information then exit.*/
> +/* Query mode: show CPU affinity information then exit. */
> if (query) {
> /* When query mode and neither "live", "config" nor "current"
> - * is specified, set VIR_DOMAIN_AFFECT_CURRENT as flags */
> + * is specified, set VIR_DOMAIN_AFFECT_CURRENT as flags.
> + */
> if (flags == -1)
> flags = VIR_DOMAIN_AFFECT_CURRENT;
>
> @@ -5775,10 +5795,15 @@ cmdEmulatorPin(vshControl *ctl, const vshCmd *cmd)
> goto cleanup;
> }
>
> -/* Pin mode: pinning emulator threads to specified physical cpus*/
> +/* Pin mode: pinning emulator threads to specified physical cpus. */
> if (!(cpumap = vshParseCPUList(ctl, cpulist, maxcpu, cpumaplen)))
> goto cleanup;
>
> +if (vshIsCpumapAllClear(cpumap, maxcpu)) {
> +vshError(ctl, "%s", _("No CPU is specified"));
> +goto cleanup;
> +}
> +
> if (flags == -1)
> flags = VIR_DOMAIN_AFFECT_LIVE;
>
>
Unlike the virReportError() which provides the function trace - how
would one know which of the two functions had the error here? OK other
than looking at the code and noting one error has an "is" verb and the
other doesn't...
One message could indicate "No domain vCPU affinity" while the other
could indicate "No domain emulator affinity"...
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] util: Add a helper to check if all bits of a bitmap are clear
On 04/02/2013 01:42 AM, Osier Yang wrote:
> ---
> src/libvirt_private.syms | 1 +
> src/util/virbitmap.c | 30 ++
> src/util/virbitmap.h | 3 +++
> 3 files changed, 34 insertions(+)
>
Since there already is a "virBitmapIsAllSet()" - why isn't it used? I
see callers which do "if (virBitmapIsAllSet(...)" and "if
(!virBitmapIsAllSet(...)".
If you're going to have a AllClear(), then why not change those !
callers to use AllClear()...
I only wonder about the last comparison - it's the "-1" logic that
throws me off especially since the IsAllSet() code is doing a comparison.
It also stands to reason that tests/virbitmaptest.c could add new tests
to ensure you did get the logic right.
John
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 96eea0a..35ac957 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1048,6 +1048,7 @@ virBitmapEqual;
> virBitmapFormat;
> virBitmapFree;
> virBitmapGetBit;
> +virBitmapIsAllClear;
> virBitmapIsAllSet;
> virBitmapNew;
> virBitmapNewCopy;
> diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c
> index 21509ac..99a8572 100644
> --- a/src/util/virbitmap.c
> +++ b/src/util/virbitmap.c
> @@ -591,6 +591,36 @@ bool virBitmapIsAllSet(virBitmapPtr bitmap)
> }
>
> /**
> + * virBitmapIsAllClear:
> + * @bitmap: the bitmap to check
> + *
> + * check if all bits in @bitmap are clear
> + */
> +bool virBitmapIsAllClear(virBitmapPtr bitmap)
> +{
> +int i;
> +int unusedBits;
> +size_t sz;
> +
> +unusedBits = bitmap->map_len * VIR_BITMAP_BITS_PER_UNIT -
> bitmap->max_bit;
> +
> +sz = bitmap->map_len;
> +if (unusedBits > 0)
> +sz--;
> +
> +for (i = 0; i < sz; i++)
> +if (bitmap->map[i] != 0)
> +return false;
> +
> +if (unusedBits > 0) {
> +if ((bitmap->map[sz] & ((1UL << (VIR_BITMAP_BITS_PER_UNIT -
> unusedBits)) - 1)))
> +return false;
> +}
> +
> +return true;
> +}
> +
> +/**
> * virBitmapNextSetBit:
> * @bitmap: the bitmap
> * @pos: the position after which to search for a set bit
> diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h
> index 044c7a6..b682523 100644
> --- a/src/util/virbitmap.h
> +++ b/src/util/virbitmap.h
> @@ -100,6 +100,9 @@ void virBitmapClearAll(virBitmapPtr bitmap)
> bool virBitmapIsAllSet(virBitmapPtr bitmap)
> ATTRIBUTE_NONNULL(1);
>
> +bool virBitmapIsAllClear(virBitmapPtr bitmap)
> +ATTRIBUTE_NONNULL(1);
> +
> ssize_t virBitmapNextSetBit(virBitmapPtr bitmap, ssize_t pos)
> ATTRIBUTE_NONNULL(1);
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] util: Add a helper to check if all bits of a bitmap are clear
On Tue, Apr 02, 2013 at 06:52:22AM -0400, John Ferlan wrote: > On 04/02/2013 01:42 AM, Osier Yang wrote: > > --- > > src/libvirt_private.syms | 1 + > > src/util/virbitmap.c | 30 ++ > > src/util/virbitmap.h | 3 +++ > > 3 files changed, 34 insertions(+) > > > > Since there already is a "virBitmapIsAllSet()" - why isn't it used? I > see callers which do "if (virBitmapIsAllSet(...)" and "if > (!virBitmapIsAllSet(...)". > > If you're going to have a AllClear(), then why not change those ! > callers to use AllClear()... !virBitmapIsAllSet() is not the same as virBitmapIsAllClear(). !virBitmapIsAllSet() allows for [0 -> (n-1)] bits to be set, whereas virBitmapIsAllClear() only allows for 0 bits to be set. Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 07/11] Add SELinux and DAC labeling support for TPM passthrough
On 04/01/2013 05:06 PM, Corey Bryant wrote:
On 03/21/2013 11:42 AM, Stefan Berger wrote:
Signed-off-by: Stefan Berger
---
src/security/security_dac.c | 53 ++
src/security/security_selinux.c | 96
2 files changed, 149 insertions(+)
Index: libvirt/src/security/security_selinux.c
===
--- libvirt.orig/src/security/security_selinux.c
+++ libvirt/src/security/security_selinux.c
@@ -45,6 +45,7 @@
#include "virrandom.h"
#include "virutil.h"
#include "virconf.h"
+#include "virtpm.h"
#define VIR_FROM_THIS VIR_FROM_SECURITY
@@ -76,6 +77,12 @@ struct _virSecuritySELinuxCallbackData {
#define SECURITY_SELINUX_VOID_DOI "0"
#define SECURITY_SELINUX_NAME "selinux"
+static int
+virSecuritySELinuxRestoreSecurityTPMFileLabelInt(virSecurityManagerPtr
mgr,
+ virDomainDefPtr def,
+ virDomainTPMDefPtr tpm);
+
+
/*
* Returns 0 on success, 1 if already reserved, or -1 on fatal error
*/
@@ -1062,6 +1069,84 @@ err:
return rc;
}
+
+static int
+virSecuritySELinuxSetSecurityTPMFileLabel(virSecurityManagerPtr mgr,
+ virDomainDefPtr def,
+ virDomainTPMDefPtr tpm)
+{
+int rc;
+virSecurityLabelDefPtr seclabel;
+char *cancel_path;
+
+seclabel = virDomainDefGetSecurityLabelDef(def,
SECURITY_SELINUX_NAME);
+if (seclabel == NULL)
+return -1;
+
+switch (tpm->type) {
+case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
+rc = virSecuritySELinuxSetFilecon(
+ tpm->data.passthrough.source.data.file.path,
+ seclabel->imagelabel);
+if (rc < 0)
+return -1;
+
+if ((cancel_path = virTPMFindCancelPath()) != NULL) {
+rc = virSecuritySELinuxSetFilecon(cancel_path,
+ seclabel->imagelabel);
+VIR_FREE(cancel_path);
+if (rc < 0) {
+ virSecuritySELinuxRestoreSecurityTPMFileLabelInt(mgr, def,
+ tpm);
+return -1;
+}
+} else {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Cannot determine TPM command cancel
path"));
+return -1;
This makes me wonder if cancel-path should be specifiable at the
libvirt level rather than just using the default sysfs entry. If I've
read the code correctly I don't think it can currently be specified.
However QEMU is capable of taking a cancel-path string in case it is
different from the default sysfs path.
I am not sure whether to allow users to specify the path and
misconfigure it and to have QEMU write a letter into the wrong file. I
wonder whether we could have libvirt determine the path and display it
in the XML as read-only, though.
Stefan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/1] Fix the crash when seclable is freed
On 02.04.2013 07:58, Li Zhang wrote:
> From: Li Zhang
>
> When seclabel's type is VIR_DOMAIN_SECLABEL_NONE,
> virSecurityLabelDefPtr's members are not allocated.
> So it will cause crash when calling VIR_FREE.
>
> This problem is found when running autotest on PPC.
>
> Failed to remove cgroup for virt-tests-vm1
> *** glibc detected *** /usr/sbin/libvirtd: free(): invalid pointer:
> 0x3fff9c187510 ***
> === Backtrace: =
> /lib64/libc.so.6(+0xb89c4)[0x3fffa9bc89c4]
> /lib64/libvirt.so.0(virFree-0x3e2320)[0x3fffaa82e9c0]
> /lib64/libvirt.so.0(virSecurityLabelDefFree-0x378984)[0x3fffaa89d69c]
> /lib64/libvirt.so.0(virDomainDefFree-0x367c98)[0x3fffaa8ae968]
>
> /usr/lib64/libvirt/connection-driver/libvirt_driver_qemu.so(qemuProcessStop-0xc85f8)[0x3fffa2899d58]
>
> /usr/lib64/libvirt/connection-driver/libvirt_driver_qemu.so(+0xc3668)[0x3fffa28e3668]
> /lib64/libvirt.so.0(virDomainDestroy-0x309bd0)[0x3fffaa90f6f0]
> /usr/sbin/libvirtd[0x10035230]
> /lib64/libvirt.so.0(virNetServerProgramDispatch-0x289b50)[0x3fffaa995930]
> /lib64/libvirt.so.0(+0x20db18)[0x3fffaa98db18]
> /lib64/libvirt.so.0(+0xfbd24)[0x3fffaa87bd24]
> /lib64/libvirt.so.0(+0xfaec8)[0x3fffaa87aec8]
> /lib64/libpthread.so.0(+0xc604)[0x3fffa9d7c604]
> /lib64/libc.so.6(clone-0xb8fe4)[0x3fffa9c3f094]
>
> Signed-off-by: Li Zhang
> ---
> src/conf/domain_conf.c |2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index f3fca7f..2856660 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1006,6 +1006,8 @@ virSecurityLabelDefFree(virSecurityLabelDefPtr def)
> {
> if (!def)
> return;
> +if (def->type == VIR_DOMAIN_SECLABEL_NONE)
> +return;
> VIR_FREE(def->model);
> VIR_FREE(def->label);
> VIR_FREE(def->imagelabel);
>
NACK
As you already found out, we are freeing invalid pointers. We need to
find out root cause. I wonder where those pointers come from, as
VIR_ALLOC(), which is used to alloc a virSecurityLabelDefPtr, fill
allocated memory with zeros, so calling VIR_FREE() even for struct
members is just fine. Are you able to reproduce this crash? What are the
steps?
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5] qemu: Allow migration over IPv6
On Fri, Mar 22, 2013 at 08:06:00PM +0100, Ján Tomko wrote:
> Allow migration over IPv6 by listening on [::] instead of 0.0.0.0
> when QEMU supports it (QEMU_CAPS_IPV6_MIGRATION) and there is
> at least one v6 address configured on the system.
>
> Use virURIParse in qemuMigrationPrepareDirect to allow parsing
> IPv6 addresses, which would cause an 'incorrect :port' error
> message before.
>
> Move setting of migrateFrom from qemuMigrationPrepare{Direct,Tunnel}
> after domain XML parsing, since we need the QEMU binary path from it
> to get its capabilities.
>
> Bug: https://bugzilla.redhat.com/show_bug.cgi?id=846013
> ---
>
> diff to v4:
> Always listen on IPv6 if it's available.
> Don't add a migration flag.
>
> v4:
> https://www.redhat.com/archives/libvir-list/2013-March/msg01213.html
>
> discussion:
> https://www.redhat.com/archives/libvir-list/2013-March/msg00515.html
>
> src/qemu/qemu_capabilities.c | 6 +++
> src/qemu/qemu_capabilities.h | 1 +
> src/qemu/qemu_migration.c| 104
> +--
> tests/qemuhelptest.c | 9 ++--
> 4 files changed, 93 insertions(+), 27 deletions(-)
ACK
> @@ -2084,6 +2088,45 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
> }
> }
>
> +if (tunnel) {
> +/* QEMU will be started with -incoming stdio
> + * (which qemu_command might convert to exec:cat or fd:n)
> + */
> +if (!(migrateFrom = strdup("stdio"))) {
> +virReportOOMError();
> +goto cleanup;
> +}
> +} else {
> +virQEMUCapsPtr qemuCaps = NULL;
> +struct addrinfo *info = NULL;
> +struct addrinfo hints = { .ai_flags = AI_ADDRCONFIG,
> + .ai_socktype = SOCK_STREAM };
> +
> +if (!(qemuCaps = virQEMUCapsCacheLookupCopy(driver->qemuCapsCache,
> +def->emulator)))
> +goto cleanup;
> +
> +/* Listen on :: instead of 0.0.0.0 if QEMU understands it
> + * and there is at least one IPv6 address configured
> + */
> +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_IPV6_MIGRATION) &&
> +getaddrinfo("::", NULL, &hints, &info) == 0) {
> +freeaddrinfo(info);
> +listenAddr = "[::]";
> +} else {
> +listenAddr = "0.0.0.0";
> +}
> +virObjectUnref(qemuCaps);
> +
> +/* QEMU will be started with -incoming [::]:port
> + * or -incoming 0.0.0.0:port
> + */
> +if (virAsprintf(&migrateFrom, "tcp:%s:%d", listenAddr, port) < 0) {
> +virReportOOMError();
> +goto cleanup;
> +}
> +}
It is a little gross that we're doing this command line
construction code in here and not in qemu_command.c
qemuBuildCommandLine() method, but we can live with that
for now
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] virsh: Prohibit all clear cpumap
On 04/01/2013 11:42 PM, Osier Yang wrote: > This prohibits all clear cpumap eariler in virsh, for both vcpupin > and emulatorpin. Is that really necessary? Sometimes, letting virsh allow obviously wrong constructs, in order to prove that libvirt itself will flag things as sensible errors, makes for nicer testing. Virsh should only filter out obviously wrong things if it can give a nicer error message than what the underlying API would produce. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3] test: Return Libvirt logo as domain screenshot
This is just a bare Easter Egg. Whenever a user runs virDomainScreenshot
over a domain in test driver, he'll get the Libvirt PNG logo in return.
---
docs/Makefile.am | 1 +
libvirt.spec.in| 1 +
src/test/test_driver.c | 24
3 files changed, 26 insertions(+)
diff --git a/docs/Makefile.am b/docs/Makefile.am
index 7583772..b5d7575 100644
--- a/docs/Makefile.am
+++ b/docs/Makefile.am
@@ -287,6 +287,7 @@ install-data-local:
for file in $(devhelphtml) $(devhelppng) $(devhelpcss); do \
$(INSTALL) -m 0644 $(srcdir)/$${file} $(DESTDIR)$(DEVHELP_DIR) ; \
done
+ $(INSTALL_DATA) $(srcdir)/../docs/libvirtLogo.png
$(DESTDIR)$(pkgdatadir)
uninstall-local:
for h in $(apihtml); do rm $(DESTDIR)$(HTML_DIR)/$$h; done
diff --git a/libvirt.spec.in b/libvirt.spec.in
index 2da0558..34b3f9c 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -1941,6 +1941,7 @@ fi
%{_datadir}/libvirt/schemas/storagevol.rng
%{_datadir}/libvirt/cpu_map.xml
+%{_datadir}/libvirt/libvirtLogo.png
%if %{with_systemd}
%{_unitdir}/libvirt-guests.service
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index c5fffb9..4dbd775 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -39,11 +39,13 @@
#include "virutil.h"
#include "viruuid.h"
#include "capabilities.h"
+#include "configmake.h"
#include "viralloc.h"
#include "network_conf.h"
#include "interface_conf.h"
#include "domain_conf.h"
#include "domain_event.h"
+#include "fdstream.h"
#include "storage_conf.h"
#include "node_device_conf.h"
#include "virxml.h"
@@ -5773,6 +5775,27 @@ cleanup:
return ret;
}
+static char *
+testDomainScreenshot(virDomainPtr dom ATTRIBUTE_UNUSED,
+ virStreamPtr st,
+ unsigned int screen ATTRIBUTE_UNUSED,
+ unsigned int flags)
+{
+char *ret = NULL;
+
+virCheckFlags(0, NULL);
+
+if (!(ret = strdup("image/png"))) {
+virReportOOMError();
+return NULL;
+}
+
+if (virFDStreamOpenFile(st, PKGDATADIR "/libvirtLogo.png", 0, 0, O_RDONLY
< 0))
+VIR_FREE(ret);
+
+return ret;
+}
+
static virDriver testDriver = {
.no = VIR_DRV_TEST,
@@ -5843,6 +5866,7 @@ static virDriver testDriver = {
.domainEventDeregisterAny = testDomainEventDeregisterAny, /* 0.8.0 */
.isAlive = testIsAlive, /* 0.9.8 */
.nodeGetCPUMap = testNodeGetCPUMap, /* 1.0.0 */
+.domainScreenshot = testDomainScreenshot, /* 1.0.5 */
};
static virNetworkDriver testNetworkDriver = {
--
1.8.1.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] test: Return Libvirt logo as domain screenshot
On Tue, Apr 02, 2013 at 01:52:39PM +0200, Michal Privoznik wrote:
> This is just a bare Easter Egg. Whenever a user runs virDomainScreenshot
> over a domain in test driver, he'll get the Libvirt PNG logo in return.
> ---
> docs/Makefile.am | 1 +
> libvirt.spec.in| 1 +
> src/test/test_driver.c | 24
> 3 files changed, 26 insertions(+)
>
> diff --git a/docs/Makefile.am b/docs/Makefile.am
> index 7583772..b5d7575 100644
> --- a/docs/Makefile.am
> +++ b/docs/Makefile.am
> @@ -287,6 +287,7 @@ install-data-local:
> for file in $(devhelphtml) $(devhelppng) $(devhelpcss); do \
> $(INSTALL) -m 0644 $(srcdir)/$${file} $(DESTDIR)$(DEVHELP_DIR) ; \
> done
> + $(INSTALL_DATA) $(srcdir)/../docs/libvirtLogo.png
> $(DESTDIR)$(pkgdatadir)
Huh, with '$(srcdir)' you're in 'docs' already, so why are you doing
'../docs/' ?
Regards,
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] test: Return Libvirt logo as domain screenshot
On 04/02/2013 05:52 AM, Michal Privoznik wrote:
> This is just a bare Easter Egg. Whenever a user runs virDomainScreenshot
> over a domain in test driver, he'll get the Libvirt PNG logo in return.
> ---
> docs/Makefile.am | 1 +
> libvirt.spec.in| 1 +
> src/test/test_driver.c | 24
> 3 files changed, 26 insertions(+)
>
> +++ b/libvirt.spec.in
> @@ -1941,6 +1941,7 @@ fi
> %{_datadir}/libvirt/schemas/storagevol.rng
>
> %{_datadir}/libvirt/cpu_map.xml
> +%{_datadir}/libvirt/libvirtLogo.png
mingw-libvirt.spec.in needs the same change.
ACK if you fix that and Daniel's comment about a simpler Makefile.am.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] util: Add a helper to check if all bits of a bitmap are clear
On 02/04/13 18:52, John Ferlan wrote: On 04/02/2013 01:42 AM, Osier Yang wrote: --- src/libvirt_private.syms | 1 + src/util/virbitmap.c | 30 ++ src/util/virbitmap.h | 3 +++ 3 files changed, 34 insertions(+) Since there already is a "virBitmapIsAllSet()" - why isn't it used? I see callers which do "if (virBitmapIsAllSet(...)" and "if (!virBitmapIsAllSet(...)". I want to check if the bitmap is all zero. Obviously !virBitmapIsAllSet can't do it. If you're going to have a AllClear(), then why not change those ! callers to use AllClear()... I only wonder about the last comparison - it's the "-1" logic that throws me off especially since the IsAllSet() code is doing a comparison. It also stands to reason that tests/virbitmaptest.c could add new tests to ensure you did get the logic right. Agreed. I didn't notice this. Will add. Osier -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] util: Add a helper to check if all bits of a bitmap are clear
On 04/01/2013 11:42 PM, Osier Yang wrote:
> ---
> src/libvirt_private.syms | 1 +
> src/util/virbitmap.c | 30 ++
> src/util/virbitmap.h | 3 +++
> 3 files changed, 34 insertions(+)
>
> +bool virBitmapIsAllClear(virBitmapPtr bitmap)
> +{
> +int i;
> +int unusedBits;
> +size_t sz;
> +
> +unusedBits = bitmap->map_len * VIR_BITMAP_BITS_PER_UNIT -
> bitmap->max_bit;
> +
> +sz = bitmap->map_len;
> +if (unusedBits > 0)
> +sz--;
> +
> +for (i = 0; i < sz; i++)
> +if (bitmap->map[i] != 0)
> +return false;
> +
> +if (unusedBits > 0) {
> +if ((bitmap->map[sz] & ((1UL << (VIR_BITMAP_BITS_PER_UNIT -
> unusedBits)) - 1)))
> +return false;
You are being careful to avoid assuming any state about the bits in the
tail beyond the bitmap size. But I thought our code was already careful
to ensure that the tail bits are always 0. Therefore, you should be
able to just check that the entire bitmap->map is 0, without
special-casing the tail.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] virsh: Prohibit all clear cpumap
On 02/04/13 19:50, Eric Blake wrote: On 04/01/2013 11:42 PM, Osier Yang wrote: This prohibits all clear cpumap eariler in virsh, for both vcpupin and emulatorpin. Is that really necessary? Honestly, I'm not sure. :-) Sometimes, letting virsh allow obviously wrong constructs, in order to prove that libvirt itself will flag things as sensible errors, makes for nicer testing. Virsh should only filter out obviously wrong things if it can give a nicer error message than what the underlying API would produce. I tended to error out earlier, but I also agreed with you that sometimes the good way is to let it fall through to driver. So I'm fine to drop this patch. Osier -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] virsh: Prohibit all clear cpumap
On 02/04/13 18:53, John Ferlan wrote:
On 04/02/2013 01:42 AM, Osier Yang wrote:
This prohibits all clear cpumap eariler in virsh, for both vcpupin
and emulatorpin.
---
tools/virsh-domain.c | 35 ++-
1 file changed, 30 insertions(+), 5 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 965f92c..8d3d63f 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -5552,6 +5552,20 @@ cleanup:
}
static bool
+vshIsCpumapAllClear(const unsigned char *cpumap,
+int maxcpu)
+{
+int i;
+
+for (i = 0; i < maxcpu; i++) {
+if (VIR_CPU_USED(cpumap, i))
+return false;
+}
+
+return true;
+}
+
+static bool
cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
{
virDomainInfo info;
@@ -5620,7 +5634,7 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
cpumaplen = VIR_CPU_MAPLEN(maxcpu);
-/* Query mode: show CPU affinity information then exit.*/
+/* Query mode: show CPU affinity information then exit. */
if (query) {
/* When query mode and neither "live", "config" nor "current"
* is specified, set VIR_DOMAIN_AFFECT_CURRENT as flags */
@@ -5649,10 +5663,15 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
goto cleanup;
}
-/* Pin mode: pinning specified vcpu to specified physical cpus*/
+/* Pin mode: pinning specified vcpu to specified physical cpus. */
if (!(cpumap = vshParseCPUList(ctl, cpulist, maxcpu, cpumaplen)))
goto cleanup;
+if (vshIsCpumapAllClear(cpumap, maxcpu)) {
+vshError(ctl, "%s", _("No CPU specified"));
+goto cleanup;
+}
+
if (flags == -1) {
if (virDomainPinVcpu(dom, vcpu, cpumap, cpumaplen) != 0)
goto cleanup;
@@ -5755,10 +5774,11 @@ cmdEmulatorPin(vshControl *ctl, const vshCmd *cmd)
cpumaplen = VIR_CPU_MAPLEN(maxcpu);
-/* Query mode: show CPU affinity information then exit.*/
+/* Query mode: show CPU affinity information then exit. */
if (query) {
/* When query mode and neither "live", "config" nor "current"
- * is specified, set VIR_DOMAIN_AFFECT_CURRENT as flags */
+ * is specified, set VIR_DOMAIN_AFFECT_CURRENT as flags.
+ */
if (flags == -1)
flags = VIR_DOMAIN_AFFECT_CURRENT;
@@ -5775,10 +5795,15 @@ cmdEmulatorPin(vshControl *ctl, const vshCmd *cmd)
goto cleanup;
}
-/* Pin mode: pinning emulator threads to specified physical cpus*/
+/* Pin mode: pinning emulator threads to specified physical cpus. */
if (!(cpumap = vshParseCPUList(ctl, cpulist, maxcpu, cpumaplen)))
goto cleanup;
+if (vshIsCpumapAllClear(cpumap, maxcpu)) {
+vshError(ctl, "%s", _("No CPU is specified"));
+goto cleanup;
+}
+
if (flags == -1)
flags = VIR_DOMAIN_AFFECT_LIVE;
Unlike the virReportError() which provides the function trace - how
would one know which of the two functions had the error here? OK other
than looking at the code and noting one error has an "is" verb and the
other doesn't...
Something like "vcpupin: No CPU is specified" should be clear
enough to tell which function throws the error. But what I was
thinking is the prefix "vcpupin:" is redundant. As one must known
he is executing a "vcpupin" command. E.g.
% virsh vcpupin toy 0 0,^0
error: No CPU specified
One message could indicate "No domain vCPU affinity" while the other
could indicate "No domain emulator affinity"...
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5] qemu: Allow migration over IPv6
On 04/02/2013 01:29 PM, Daniel P. Berrange wrote: > On Fri, Mar 22, 2013 at 08:06:00PM +0100, Ján Tomko wrote: >> Allow migration over IPv6 by listening on [::] instead of 0.0.0.0 >> when QEMU supports it (QEMU_CAPS_IPV6_MIGRATION) and there is >> at least one v6 address configured on the system. >> ... >> >> src/qemu/qemu_capabilities.c | 6 +++ >> src/qemu/qemu_capabilities.h | 1 + >> src/qemu/qemu_migration.c| 104 >> +-- >> tests/qemuhelptest.c | 9 ++-- >> 4 files changed, 93 insertions(+), 27 deletions(-) > > ACK > Thanks, pushed now. Jan -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 1/2] Optimize machine option to set more options with it
On 04/02/2013 04:05 AM, Daniel P. Berrange wrote:
> On Fri, Mar 29, 2013 at 01:22:46PM +0800, Li Zhang wrote:
>> From: Li Zhang
>>
>> Currently, -machine option is used only when dump-guest-core is set.
>>
>> To use options defined in machine option for newer version of QEMU,
>> it needs to use -machine xxx, and to be compatible with older version
>> -M, this patch addes QEMU_CAPS_MACHINE_OPT capability for newer
s/addes/adds/
>> version which supports -machine option.
>>
>
> ACK
Pushed, after making some fixes (don't have two spaces in the error
message, smaller scope for the buffer, use faster buffer functions, and
report an error if dump_guest_core is present with the -M form where it
is not supported):
diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c
index 14be49f..a6d011e 100644
--- i/src/qemu/qemu_command.c
+++ w/src/qemu/qemu_command.c
@@ -5200,8 +5200,6 @@ qemuBuildMachineArgStr(virCommandPtr cmd,
const virDomainDefPtr def,
virQEMUCapsPtr qemuCaps)
{
-virBuffer buf = VIR_BUFFER_INITIALIZER;
-
/* This should *never* be NULL, since we always provide
* a machine in the capabilities data for QEMU. So this
* check is just here as a safety in case the unexpected
@@ -5214,16 +5212,23 @@ qemuBuildMachineArgStr(virCommandPtr cmd,
* '-M' to keep the most of the compatibility with older versions.
*/
virCommandAddArgList(cmd, "-M", def->os.machine, NULL);
+if (def->mem.dump_core) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("dump-guest-core is not available "
+ "with this QEMU binary"));
+return -1;
+}
} else {
+virBuffer buf = VIR_BUFFER_INITIALIZER;
virCommandAddArg(cmd, "-machine");
-virBufferAsprintf(&buf, "%s", def->os.machine);
+virBufferAdd(&buf, def->os.machine, -1);
if (def->mem.dump_core) {
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DUMP_GUEST_CORE)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- "%s", _("dump-guest-core is not available "
-" with this QEMU binary"));
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("dump-guest-core is not available "
+ "with this QEMU binary"));
return -1;
}
@@ -5231,7 +5236,7 @@ qemuBuildMachineArgStr(virCommandPtr cmd,
virDomainMemDumpTypeToString(def->mem.dump_core));
}
-virCommandAddArg(cmd, virBufferContentAndReset(&buf));
+virCommandAddArgBuffer(cmd, &buf);
}
return 0;
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] test: Return Libvirt logo as domain screenshot
On 02.04.2013 13:56, Daniel P. Berrange wrote:
> On Tue, Apr 02, 2013 at 01:52:39PM +0200, Michal Privoznik wrote:
>> This is just a bare Easter Egg. Whenever a user runs virDomainScreenshot
>> over a domain in test driver, he'll get the Libvirt PNG logo in return.
>> ---
>> docs/Makefile.am | 1 +
>> libvirt.spec.in| 1 +
>> src/test/test_driver.c | 24
>> 3 files changed, 26 insertions(+)
>>
>> diff --git a/docs/Makefile.am b/docs/Makefile.am
>> index 7583772..b5d7575 100644
>> --- a/docs/Makefile.am
>> +++ b/docs/Makefile.am
>> @@ -287,6 +287,7 @@ install-data-local:
>> for file in $(devhelphtml) $(devhelppng) $(devhelpcss); do \
>> $(INSTALL) -m 0644 $(srcdir)/$${file} $(DESTDIR)$(DEVHELP_DIR) ; \
>> done
>> +$(INSTALL_DATA) $(srcdir)/../docs/libvirtLogo.png
>> $(DESTDIR)$(pkgdatadir)
>
> Huh, with '$(srcdir)' you're in 'docs' already, so why are you doing
> '../docs/' ?
>
Dunno. My mind must had a blackout :) Fixed. and pushed among with mingw
spec file changes.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2] conf: Enforce ranges on cputune variables
On 04/01/13 19:11, Eric Blake wrote: On 03/31/2013 10:35 AM, Peter Krempa wrote: The limits are documented at http://libvirt.org/formatdomain.html#elementsCPUTuning . Enforce them when going through XML parsing in addition to being enforced by the API. --- Notes: Version 2: - split out from the conf callback series and applied separately to the parser code src/conf/domain_conf.c | 35 +++ 1 file changed, 35 insertions(+) ACK. Pushed. Thanks. Peter -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Libvirt-announce] Candidate release 2 of libvirt-1.0.4 available
Hi, On Mon, Apr 01, 2013 at 11:04:09AM +0800, Daniel Veillard wrote: > On Mon, Apr 01, 2013 at 03:30:47AM +0100, Justin Clift wrote: > > On 28/03/2013, at 3:08 AM, Daniel Veillard wrote: > > > I tagged it in git and pushed the tarball to the usual area: > > > > > > ftp://libvirt.org/libvirt/ > > > > > > the rpms are being pushed ATM. > > > Based on my own limited testing this still looks like good to go, > > > but more feedback (portability ?) would be nice. > > > If all goes well this will go out very early on Monday as scheduled, > > > > > > thanks ! > > > > > > Tried this on OSX 10.7.5, with Osier's build breaker patch added, and > > compilation succeeds. > > > > No vm's handy at the moment to try it out against though. :/ > > > > Hope that helps at least a bit. :) > > Yup, feel better seeing this just as I'm about to roll 1.0.4 :-) > >thanks ! I'm a bit late in the game this time but the build on the Debian buildds looks good: https://buildd.debian.org/status/package.php?p=libvirt&suite=experimental I still didn't get a chance to investigate the kFreeBSD failure but hopefully later this week. Cheers, -- Guido -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] smartcard: spell ccid-card-emulated qemu property correctly
On 04/02/2013 04:10 AM, Daniel P. Berrange wrote: > On Mon, Apr 01, 2013 at 04:57:00PM -0600, Eric Blake wrote: >> Reported by Anthony Messina in >> https://bugzilla.redhat.com/show_bug.cgi?id=904692 >> Present since introduction of smartcard support in commit f5fd9baa >> >> } >> -virBufferAsprintf(&opt, ",database=%s", database); >> +virBufferAsprintf(&opt, ",db=%s", database); >> break; >> >> case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH: > > ACK, > > Is a test case worthwhile ? It wouldn't have spotted this if the test > case had had the same typo though Indeed, I failed to run 'make check' before submitting this, and we did have a test with the same typo. I'm squashing this in, then pushing. diff --git i/tests/qemuxml2argvdata/qemuxml2argv-smartcard-host-certificates.args w/tests/qemuxml2argvdata/qemuxml2argv-smartcard-host-certificates.args index 8b4be3a..b7b14c7 100644 --- i/tests/qemuxml2argvdata/qemuxml2argv-smartcard-host-certificates.args +++ w/tests/qemuxml2argvdata/qemuxml2argv-smartcard-host-certificates.args @@ -4,5 +4,5 @@ socket,id=charmonitor,path=/tmp/test-monitor,server,nowait -mon \ chardev=charmonitor,id=monitor,mode=readline -no-acpi -boot c -device \ usb-ccid,id=ccid0 -usb -device \ ccid-card-emulated,backend=certificates,cert1=cert1,cert2=cert2,cert3=cert3\ -,database=/etc/pki/nssdb,id=smartcard0,bus=ccid0.0 -device \ +,db=/etc/pki/nssdb,id=smartcard0,bus=ccid0.0 -device \ virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2 2/3] Use virMacAddrFormat instead of manual mac address formatting
Format the address using the helper instead of having similar code in
multiple places.
This patch also fixes leak of the MAC address string in
ebtablesRemoveForwardAllowIn() and ebtablesAddForwardAllowIn() in
src/util/virebtables.c
---
src/conf/domain_conf.c | 20 ++--
src/qemu/qemu_command.c | 15 +++
src/uml/uml_conf.c | 7 +++
src/util/virebtables.c | 26 +++---
src/util/virnetdevmacvlan.c | 9 +++--
src/util/virnetdevtap.c | 9 -
src/xen/xend_internal.c | 9 +++--
src/xenxs/xen_sxpr.c| 8 +++-
src/xenxs/xen_xm.c | 8 +++-
9 files changed, 43 insertions(+), 68 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 371d80c..cc26f21 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -11521,14 +11521,15 @@ static bool
virDomainNetDefCheckABIStability(virDomainNetDefPtr src,
virDomainNetDefPtr dst)
{
+char srcmac[VIR_MAC_STRING_BUFLEN];
+char dstmac[VIR_MAC_STRING_BUFLEN];
+
if (virMacAddrCmp(&src->mac, &dst->mac) != 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("Target network card mac
%02x:%02x:%02x:%02x:%02x:%02x"
- " does not match source
%02x:%02x:%02x:%02x:%02x:%02x"),
- dst->mac.addr[0], dst->mac.addr[1], dst->mac.addr[2],
- dst->mac.addr[3], dst->mac.addr[4], dst->mac.addr[5],
- src->mac.addr[0], src->mac.addr[1], src->mac.addr[2],
- src->mac.addr[3], src->mac.addr[4], src->mac.addr[5]);
+ _("Target network card mac %s"
+ " does not match source %s"),
+ virMacAddrFormat(&dst->mac, dstmac),
+ virMacAddrFormat(&src->mac, srcmac));
return false;
}
@@ -13389,6 +13390,7 @@ virDomainNetDefFormat(virBufferPtr buf,
unsigned int flags)
{
const char *type = virDomainNetTypeToString(def->type);
+char macstr[VIR_MAC_STRING_BUFLEN];
if (!type) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -13404,10 +13406,8 @@ virDomainNetDefFormat(virBufferPtr buf,
virBufferAddLit(buf, ">\n");
virBufferAdjustIndent(buf, 6);
-virBufferAsprintf(buf,
- "\n",
- def->mac.addr[0], def->mac.addr[1], def->mac.addr[2],
- def->mac.addr[3], def->mac.addr[4], def->mac.addr[5]);
+virBufferAsprintf(buf, "\n",
+ virMacAddrFormat(&def->mac, macstr));
switch (def->type) {
case VIR_DOMAIN_NET_TYPE_NETWORK:
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c80218d..17b0a9f 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3638,12 +3638,12 @@ qemuBuildNicStr(virDomainNetDefPtr net,
int vlan)
{
char *str;
+char macaddr[VIR_MAC_STRING_BUFLEN];
+
if (virAsprintf(&str,
-"%smacaddr=%02x:%02x:%02x:%02x:%02x:%02x,vlan=%d%s%s%s%s",
+"%smacaddr=%s,vlan=%d%s%s%s%s",
prefix ? prefix : "",
-net->mac.addr[0], net->mac.addr[1],
-net->mac.addr[2], net->mac.addr[3],
-net->mac.addr[4], net->mac.addr[5],
+virMacAddrFormat(&net->mac, macaddr),
vlan,
(net->model ? ",model=" : ""),
(net->model ? net->model : ""),
@@ -3666,6 +3666,7 @@ qemuBuildNicDevStr(virDomainNetDefPtr net,
virBuffer buf = VIR_BUFFER_INITIALIZER;
const char *nic;
bool usingVirtio = false;
+char macaddr[VIR_MAC_STRING_BUFLEN];
if (!net->model) {
nic = "rtl8139";
@@ -3722,10 +3723,8 @@ qemuBuildNicDevStr(virDomainNetDefPtr net,
else
virBufferAsprintf(&buf, ",vlan=%d", vlan);
virBufferAsprintf(&buf, ",id=%s", net->info.alias);
-virBufferAsprintf(&buf, ",mac=%02x:%02x:%02x:%02x:%02x:%02x",
- net->mac.addr[0], net->mac.addr[1],
- net->mac.addr[2], net->mac.addr[3],
- net->mac.addr[4], net->mac.addr[5]);
+virBufferAsprintf(&buf, ",mac=%s",
+ virMacAddrFormat(&net->mac, macaddr));
if (qemuBuildDeviceAddressStr(&buf, &net->info, qemuCaps) < 0)
goto error;
if (qemuBuildRomStr(&buf, &net->info, qemuCaps) < 0)
diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c
index b3ac326..0fe59fb 100644
--- a/src/uml/uml_conf.c
+++ b/src/uml/uml_conf.c
@@ -1,7 +1,7 @@
/*
* uml_conf.c: UML driver configuration
*
- * Copyright (C) 2006-2012 Red Hat, Inc.
+ * Copyright (C) 2006-2013 Red Hat, Inc.
* Copyright (C) 2006 Daniel P. Berrange
*
* This library is free software; you can redistribute it and/or
@@ -164,6 +164,7 @@ umlBuildCommandLineNet(virConnectP
Re: [libvirt] [PATCHv2 2/3] Use virMacAddrFormat instead of manual mac address formatting
On 04/02/2013 03:37 PM, Peter Krempa wrote: > Format the address using the helper instead of having similar code in > multiple places. > > This patch also fixes leak of the MAC address string in > ebtablesRemoveForwardAllowIn() and ebtablesAddForwardAllowIn() in > src/util/virebtables.c > --- [...] > diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c > index 73ba06b..405ebf3 100644 > --- a/src/xenxs/xen_xm.c > +++ b/src/xenxs/xen_xm.c > @@ -1,7 +1,7 @@ > /* > * xen_xm.c: Xen XM parsing functions > * > - * Copyright (C) 2006-2007, 2009-2010, 2012 Red Hat, Inc. > + * Copyright (C) 2006-2007, 2009-2010, 2012, 2013 Red Hat, Inc. s/2012, 2013/2012-2013/ and this is my fault, sorry for that. ACK with this fixed. Martin -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] virsh-domain-monitor: Refactor cmdDomIfGetLink
On 03/26/13 15:43, Martin Kletzander wrote: On 03/26/2013 12:26 PM, Peter Krempa wrote: The domif-getlink command did not terminate successfully when the interface state was found. As the code used old and too complex approach to do the job, this patch refactors it and fixed the bug. s/fixed/fixes/ ACK, Thanks for the reviews. I've just pushed this series. Peter -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] manual: Add info about migrateuri in virsh manual
The virsh(1) man page wasn't saying anything about the 'migrateuri' parameter other than it can be usually omitted. A patched version of docs/migrate.html.in is taken in this patch to fix that up in the man page. --- docs/migration.html.in | 4 ++-- tools/virsh.pod| 28 +++- 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/docs/migration.html.in b/docs/migration.html.in index 2416275..aecef41 100644 --- a/docs/migration.html.in +++ b/docs/migration.html.in @@ -192,12 +192,12 @@ should specify the hypervisor specific URI, using an IP address associated with the network to be used. The firewall restricts what ports are available. When libvirt -generates a migration URI will pick a port number using hypervisor +generates a migration URI it will pick a port number using hypervisor specific rules. Some hypervisors only require a single port to be open in the firewalls, while others require a whole range of port numbers. In the latter case the management application may wish to choose a specific port number outside the default range in order -to comply with local firewall policies +to comply with local firewall policies. Configuration file handling diff --git a/tools/virsh.pod b/tools/virsh.pod index e7e82e3..11447fe 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1080,7 +1080,7 @@ such as GFS2 or GPFS. If you are sure the migration is safe or you just do not care, use I<--unsafe> to force the migration. The I is the connection URI of the destination host, and -I is the migration URI, which usually can be omitted. +I is the migration URI, which usually can be omitted (see below). I is used for renaming the domain to new name during migration, which also usually can be omitted. Likewise, I<--xml> B is usually omitted, but can be used to supply an alternative XML file for use on @@ -1108,6 +1108,32 @@ seen from the source machine. =back +When I is not specified, libvirt will automatically determine the +hypervisor specific URI, by looking up the target host's configured hostname. +There are a few scenarios where specifying I may help: + +=over 4 + +=item * The configured hostname is incorrect, or DNS is broken. If a host has a +hostname which will not resolve to match one of its public IP addresses, then +libvirt will generate an incorrect URI. In this case I should be +explicitly specified, using an IP address, or a correct hostname. + +=item * The host has multiple network interaces. If a host has multiple network +interfaces, it might be desirable for the migration data stream to be sent over +a specific interface for either security or performance reasons. In this case +I should be explicitly specified, using an IP address associated +with the network to be used. + +=item * The firewall restricts what ports are available. When libvirt generates +a migration URI, it will pick a port number using hypervisor specific rules. +Some hypervisors only require a single port to be open in the firewalls, while +others require a whole range of port numbers. In the latter case I +might be specified to choose a specific port number outside the default range in +order to comply with local firewall policies. + +=back + =item B I I Set maximum tolerable downtime for a domain which is being live-migrated to -- 1.8.1.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/5] Introduce VIR_STRDUP to replace strdup
--- WARNING: THIS PATCH IS NOT COMPLETE !!! For full patch see the cover letter. I've trimmed the patch and left only interesting parts. HACKING | 5 ++ cfg.mk| 8 ++ daemon/libvirtd-config.c | 24 ++--- daemon/libvirtd.c | 14 +-- daemon/remote.c | 72 +++ docs/hacking.html.in | 8 ++ src/conf/capabilities.c | 28 +++--- src/conf/cpu_conf.c | 12 +-- src/conf/domain_conf.c| 52 +-- src/conf/domain_event.c | 35 src/conf/node_device_conf.c | 11 +-- src/conf/nwfilter_conf.c | 4 +- src/conf/nwfilter_params.c| 8 +- src/conf/snapshot_conf.c | 6 +- src/conf/storage_conf.c | 6 +- src/conf/virchrdev.c | 4 +- src/cpu/cpu_generic.c | 10 +-- src/cpu/cpu_map.c | 7 +- src/cpu/cpu_powerpc.c | 16 ++-- src/cpu/cpu_x86.c | 6 +- src/datatypes.c | 27 +++--- src/esx/esx_device_monitor.c | 1 - src/esx/esx_device_monitor.h | 1 - src/esx/esx_driver.c | 15 ++-- src/esx/esx_driver.h | 1 - src/esx/esx_interface_driver.c| 20 ++--- src/esx/esx_interface_driver.h| 1 - src/esx/esx_network_driver.c | 24 ++--- src/esx/esx_network_driver.h | 1 - src/esx/esx_nwfilter_driver.c | 1 - src/esx/esx_nwfilter_driver.h | 1 - src/esx/esx_private.h | 1 - src/esx/esx_secret_driver.c | 1 - src/esx/esx_secret_driver.h | 1 - src/esx/esx_storage_backend_iscsi.c | 8 +- src/esx/esx_storage_backend_iscsi.h | 1 - src/esx/esx_storage_backend_vmfs.c| 5 +- src/esx/esx_storage_backend_vmfs.h| 1 - src/esx/esx_storage_driver.c | 1 - src/esx/esx_storage_driver.h | 1 - src/esx/esx_util.c| 13 ++- src/esx/esx_vi.c | 25 +++--- src/esx/esx_vi.h | 1 - src/esx/esx_vi_methods.c | 1 - src/esx/esx_vi_methods.h | 1 - src/esx/esx_vi_types.c| 9 +- src/esx/esx_vi_types.h| 1 - src/hyperv/hyperv_device_monitor.c| 3 - src/hyperv/hyperv_device_monitor.h| 1 - src/hyperv/hyperv_driver.c| 17 ++-- src/hyperv/hyperv_driver.h| 1 - src/hyperv/hyperv_interface_driver.c | 3 - src/hyperv/hyperv_interface_driver.h | 1 - src/hyperv/hyperv_network_driver.c| 3 - src/hyperv/hyperv_network_driver.h| 1 - src/hyperv/hyperv_nwfilter_driver.c | 3 - src/hyperv/hyperv_nwfilter_driver.h | 1 - src/hyperv/hyperv_private.h | 1 - src/hyperv/hyperv_secret_driver.c | 3 - src/hyperv/hyperv_secret_driver.h | 1 - src/hyperv/hyperv_storage_driver.c| 3 - src/hyperv/hyperv_storage_driver.h| 1 - src/hyperv/hyperv_util.c | 14 ++- src/hyperv/hyperv_util.h | 1 - src/hyperv/hyperv_wmi.c | 1 - src/hyperv/hyperv_wmi.h | 3 - src/hyperv/hyperv_wmi_classes.c | 1 - src/hyperv/hyperv_wmi_classes.h | 1 - src/hyperv/openwsman.h| 1 - src/interface/interface_backend_udev.c| 14 +-- src/libvirt.c | 6 +- src/libvirt_private.syms | 1 + src/libxl/libxl_conf.c| 36 src/libxl/libxl_driver.c | 10 +-- src/locking/lock_daemon.c | 12 +-- src/locking/lock_daemon_config.c | 8 +- src/locking/lock_daemon_dispatch.c| 3 +- src/locking/lock_driver_lockd.c | 22 ++--- src/locking/lock_driver_sanlock.c | 8 +- src/locking/lock_manager.c| 2 +- src/lxc/lxc_conf.c| 28 +++--- src/lxc/lxc_container.c | 14 +-- src/lxc/lxc_controller.c | 6 +- src/lxc/lxc_driver.c | 4 +- src/network/bridge_driver.c | 14 +-- src/node_device/node_device_driver.c | 20 ++--- src/node_device/node_device_hal.c | 6 +- src/node_device/node_device_udev.c| 48 +- src/nodeinfo.c| 2 +- src/nwfilter/nwfilter_dhcpsnoop.c | 8 +- src/nwfilter/nwfilter_driver.c| 4 +- src/nwfilter/nwfilter_ebiptables_driver.c | 2 +- src/nwfilter/nwfilter_g
[libvirt] [PATCH 3/5] virstring: Introduce virVasprintfNOOM and make virVasprintf report OOM
---
src/libvirt_private.syms | 1 +
src/qemu/qemu_domain.c | 4 +---
src/util/viraudit.c | 2 +-
src/util/vircommand.c| 4 ++--
src/util/virerror.c | 2 +-
src/util/virlog.c| 2 +-
src/util/virstring.c | 22 --
src/util/virstring.h | 3 +++
8 files changed, 30 insertions(+), 10 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 5060b87..8d1abe7 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1742,6 +1742,7 @@ virStrToLong_ul;
virStrToLong_ull;
virTrimSpaces;
virVasprintf;
+virVasprintfNOOM;
# util/virsysinfo.h
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ad8b811..78b4dd7 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1477,10 +1477,8 @@ int qemuDomainAppendLog(virQEMUDriverPtr driver,
(fd = qemuDomainCreateLog(driver, obj, true)) < 0)
goto cleanup;
-if (virVasprintf(&message, fmt, argptr) < 0) {
-virReportOOMError();
+if (virVasprintf(&message, fmt, argptr) < 0)
goto cleanup;
-}
if (safewrite(fd, message, strlen(message)) < 0) {
virReportSystemError(errno, _("Unable to write to domain logfile %s"),
obj->def->name);
diff --git a/src/util/viraudit.c b/src/util/viraudit.c
index 2588b21..7640357 100644
--- a/src/util/viraudit.c
+++ b/src/util/viraudit.c
@@ -96,7 +96,7 @@ void virAuditSend(const char *filename,
#endif
va_start(args, fmt);
-if (virVasprintf(&str, fmt, args) < 0) {
+if (virVasprintfNOOM(&str, fmt, args) < 0) {
VIR_WARN("Out of memory while formatting audit message");
str = NULL;
}
diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index 789d62d..c653f7b 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -1125,7 +1125,7 @@ virCommandAddEnvFormat(virCommandPtr cmd, const char
*format, ...)
return;
va_start(list, format);
-if (virVasprintf(&env, format, list) < 0) {
+if (virVasprintfNOOM(&env, format, list) < 0) {
cmd->has_error = ENOMEM;
va_end(list);
return;
@@ -1340,7 +1340,7 @@ virCommandAddArgFormat(virCommandPtr cmd, const char
*format, ...)
return;
va_start(list, format);
-if (virVasprintf(&arg, format, list) < 0) {
+if (virVasprintfNOOM(&arg, format, list) < 0) {
cmd->has_error = ENOMEM;
va_end(list);
return;
diff --git a/src/util/virerror.c b/src/util/virerror.c
index afb3c7a..d89f505 100644
--- a/src/util/virerror.c
+++ b/src/util/virerror.c
@@ -649,7 +649,7 @@ virRaiseErrorFull(const char *filename ATTRIBUTE_UNUSED,
} else {
va_list ap;
va_start(ap, fmt);
-ignore_value(virVasprintf(&str, fmt, ap));
+ignore_value(virVasprintfNOOM(&str, fmt, ap));
va_end(ap);
}
diff --git a/src/util/virlog.c b/src/util/virlog.c
index 7fb7896..ebe9762 100644
--- a/src/util/virlog.c
+++ b/src/util/virlog.c
@@ -830,7 +830,7 @@ virLogVMessage(virLogSource source,
/*
* serialize the error message, add level and timestamp
*/
-if (virVasprintf(&str, fmt, vargs) < 0) {
+if (virVasprintfNOOM(&str, fmt, vargs) < 0) {
goto cleanup;
}
diff --git a/src/util/virstring.c b/src/util/virstring.c
index 3847811..de7035f 100644
--- a/src/util/virstring.c
+++ b/src/util/virstring.c
@@ -316,7 +316,7 @@ virStrToDouble(char const *s,
* like glibc's vasprintf but makes sure *strp == NULL on failure
*/
int
-virVasprintf(char **strp, const char *fmt, va_list list)
+virVasprintfNOOM(char **strp, const char *fmt, va_list list)
{
int ret;
@@ -327,6 +327,23 @@ virVasprintf(char **strp, const char *fmt, va_list list)
}
/**
+ * virVasprintf
+ *
+ * like glibc's vasprintf but makes sure *strp == NULL on failure and reports
+ * OOM error.
+ */
+int
+virVasprintf(char **strp, const char *fmt, va_list list)
+{
+int ret;
+
+if ((ret = virVasprintfNOOM(strp, fmt, list)) == -1)
+virReportOOMError();
+
+return ret;
+}
+
+/**
* virAsprintf
*
* like glibc's_asprintf but makes sure *strp == NULL on failure
@@ -338,7 +355,8 @@ virAsprintf(char **strp, const char *fmt, ...)
int ret;
va_start(ap, fmt);
-ret = virVasprintf(strp, fmt, ap);
+/* XXX Don't report OOM error for now, unless all code is adapted. */
+ret = virVasprintfNOOM(strp, fmt, ap);
va_end(ap);
return ret;
}
diff --git a/src/util/virstring.h b/src/util/virstring.h
index 889951c..9c2518c 100644
--- a/src/util/virstring.h
+++ b/src/util/virstring.h
@@ -65,6 +65,9 @@ int virStrToDouble(char const *s,
char **end_ptr,
double *result);
+int virVasprintfNOOM(char **strp, const char *fmt, va_list list)
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_FMT_PRINTF(2, 0)
+ATTRIBUTE_RETURN_CHECK;
int virVasprintf(char **strp, const char *fmt, va_list list)
[libvirt] [PATCH 4/5] viralloc: Introduce OOM error reporting to VIR_ALLOC and friends
---
HACKING | 4 ++
docs/hacking.html.in| 4 ++
po/POTFILES.in | 1 +
python/libvirt-override.c | 102 ++--
src/esx/esx_vi.c| 5 +--
src/util/viralloc.c | 91 +--
src/util/viralloc.h | 52 +++---
src/util/virbuffer.c| 8 ++--
src/util/virerror.c | 4 +-
src/util/virthreadpthread.c | 2 +-
tests/commandhelper.c | 2 +-
tests/networkxml2conftest.c | 2 +-
tests/openvzutilstest.c | 2 +-
tests/test_conf.c | 2 +-
tests/xmconfigtest.c| 2 +-
15 files changed, 188 insertions(+), 95 deletions(-)
diff --git a/HACKING b/HACKING
index 6230ffd..c511cab 100644
--- a/HACKING
+++ b/HACKING
@@ -595,6 +595,10 @@ size:
+These memory allocation macros report OOM error automatically. In case, you
+want to suppress such behavior, use their NOOOM variant, e.g. VIR_ALLOCNOOM,
+VIR_REALLOC_NNOOM etc.
+
File handling
=
diff --git a/docs/hacking.html.in b/docs/hacking.html.in
index 0fbb500..531681b 100644
--- a/docs/hacking.html.in
+++ b/docs/hacking.html.in
@@ -712,6 +712,10 @@
+These memory allocation macros report OOM error automatically. In case,
+you want to suppress such behavior, use their NOOOM variant, e.g.
+VIR_ALLOCNOOM, VIR_REALLOC_NNOOM etc.
+
File handling
diff --git a/po/POTFILES.in b/po/POTFILES.in
index 91e5c02..78d6ffe 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -139,6 +139,7 @@ src/test/test_driver.c
src/uml/uml_conf.c
src/uml/uml_driver.c
src/util/iohelper.c
+src/util/viralloc.c
src/util/viraudit.c
src/util/virauth.c
src/util/virauthconfig.c
diff --git a/python/libvirt-override.c b/python/libvirt-override.c
index 5f9ae4b..08252ac 100644
--- a/python/libvirt-override.c
+++ b/python/libvirt-override.c
@@ -168,7 +168,7 @@ setPyVirTypedParameter(PyObject *info,
return NULL;
}
-if (VIR_ALLOC_N(ret, size) < 0) {
+if (VIR_ALLOC_NNOOM(ret, size) < 0) {
PyErr_NoMemory();
return NULL;
}
@@ -354,7 +354,7 @@ libvirt_virDomainBlockStatsFlags(PyObject *self
ATTRIBUTE_UNUSED,
if (!nparams)
return PyDict_New();
-if (VIR_ALLOC_N(params, nparams) < 0)
+if (VIR_ALLOC_NNOOM(params, nparams) < 0)
return PyErr_NoMemory();
LIBVIRT_BEGIN_ALLOW_THREADS;
@@ -420,7 +420,7 @@ libvirt_virDomainGetCPUStats(PyObject *self
ATTRIBUTE_UNUSED, PyObject *args)
sumparams = nparams * MIN(ncpus, 128);
-if (VIR_ALLOC_N(params, sumparams) < 0) {
+if (VIR_ALLOC_NNOOM(params, sumparams) < 0) {
error = PyErr_NoMemory();
goto error;
}
@@ -472,7 +472,7 @@ libvirt_virDomainGetCPUStats(PyObject *self
ATTRIBUTE_UNUSED, PyObject *args)
if (nparams) {
sumparams = nparams;
-if (VIR_ALLOC_N(params, nparams) < 0) {
+if (VIR_ALLOC_NNOOM(params, nparams) < 0) {
error = PyErr_NoMemory();
goto error;
}
@@ -652,7 +652,7 @@ libvirt_virDomainGetSchedulerParameters(PyObject *self
ATTRIBUTE_UNUSED,
if (!nparams)
return PyDict_New();
-if (VIR_ALLOC_N(params, nparams) < 0)
+if (VIR_ALLOC_NNOOM(params, nparams) < 0)
return PyErr_NoMemory();
LIBVIRT_BEGIN_ALLOW_THREADS;
@@ -700,7 +700,7 @@ libvirt_virDomainGetSchedulerParametersFlags(PyObject *self
ATTRIBUTE_UNUSED,
if (!nparams)
return PyDict_New();
-if (VIR_ALLOC_N(params, nparams) < 0)
+if (VIR_ALLOC_NNOOM(params, nparams) < 0)
return PyErr_NoMemory();
LIBVIRT_BEGIN_ALLOW_THREADS;
@@ -760,7 +760,7 @@ libvirt_virDomainSetSchedulerParameters(PyObject *self
ATTRIBUTE_UNUSED,
return NULL;
}
-if (VIR_ALLOC_N(params, nparams) < 0)
+if (VIR_ALLOC_NNOOM(params, nparams) < 0)
return PyErr_NoMemory();
LIBVIRT_BEGIN_ALLOW_THREADS;
@@ -836,7 +836,7 @@ libvirt_virDomainSetSchedulerParametersFlags(PyObject *self
ATTRIBUTE_UNUSED,
return NULL;
}
-if (VIR_ALLOC_N(params, nparams) < 0)
+if (VIR_ALLOC_NNOOM(params, nparams) < 0)
return PyErr_NoMemory();
LIBVIRT_BEGIN_ALLOW_THREADS;
@@ -910,7 +910,7 @@ libvirt_virDomainSetBlkioParameters(PyObject *self
ATTRIBUTE_UNUSED,
return NULL;
}
-if (VIR_ALLOC_N(params, nparams) < 0)
+if (VIR_ALLOC_NNOOM(params, nparams) < 0)
return PyErr_NoMemory();
LIBVIRT_BEGIN_ALLOW_THREADS;
@@ -970,7 +970,7 @@ libvirt_virDomainGetBlkioParameters(PyObject *self
ATTRIBUTE_UNUSED,
if (!nparams)
return PyDict_New();
-if (VIR_ALLOC_N(params, nparams) < 0)
+if (VIR_ALLOC_NNOOM(params, nparams) < 0)
return PyErr_NoMemory();
LIBVIRT_BEGIN_ALLOW_THREADS;
@@ -1030,7 +1030,7 @@ libvirt_virDomainSetMemoryParameters(PyObject *self
ATT
[libvirt] [PATCH 0/5] Drop virReportOOMReport from almost everywhere
You can find these patches applied on: git://gitorious.org/~zippy2/libvirt/michal-staging.git branch is called 'oom'. The patch 2/5 is totally mechaninc: s/strdup/VIR_STRDUP/ (not for all files, obviously). That's why I've trimmed it before sending. Michal Privoznik (5): virutil: Move string related functions to virstring.c Introduce VIR_STRDUP to replace strdup virstring: Introduce virVasprintfNOOM and make virVasprintf report OOM viralloc: Introduce OOM error reporting to VIR_ALLOC and friends Adapt to new OOM error reporting HACKING | 33 +- cfg.mk| 12 +- daemon/libvirtd-config.c | 61 ++-- daemon/libvirtd.c | 60 ++-- daemon/remote.c | 397 --- daemon/stream.c | 4 +- docs/hacking.html.in | 36 ++- po/POTFILES.in| 1 + python/libvirt-override.c | 108 --- src/conf/capabilities.c | 29 +- src/conf/cpu_conf.c | 55 ++-- src/conf/cpu_conf.h | 6 +- src/conf/device_conf.c| 11 +- src/conf/device_conf.h| 4 +- src/conf/domain_audit.c | 5 +- src/conf/domain_conf.c| 422 src/conf/domain_conf.h| 1 - src/conf/domain_event.c | 61 ++-- src/conf/domain_nwfilter.h| 2 + src/conf/interface_conf.c | 36 +-- src/conf/interface_conf.h | 2 +- src/conf/netdev_bandwidth_conf.c | 18 +- src/conf/netdev_vlan_conf.c | 4 +- src/conf/netdev_vport_profile_conf.c | 7 +- src/conf/network_conf.c | 92 ++ src/conf/node_device_conf.c | 38 +-- src/conf/node_device_conf.h | 4 +- src/conf/nwfilter_conf.c | 55 +--- src/conf/nwfilter_conf.h | 1 - src/conf/nwfilter_ipaddrmap.c | 8 +- src/conf/nwfilter_params.c| 58 ++-- src/conf/secret_conf.c| 5 +- src/conf/snapshot_conf.c | 63 ++-- src/conf/storage_conf.c | 54 +--- src/conf/storage_conf.h | 1 - src/conf/storage_encryption_conf.c| 13 +- src/conf/virchrdev.c | 23 +- src/cpu/cpu.c | 4 +- src/cpu/cpu_arm.c | 4 +- src/cpu/cpu_generic.c | 24 +- src/cpu/cpu_map.c | 7 +- src/cpu/cpu_map.h | 2 +- src/cpu/cpu_powerpc.c | 44 +-- src/cpu/cpu_s390.c| 4 +- src/cpu/cpu_x86.c | 73 ++--- src/datatypes.c | 37 +-- src/driver.c | 4 +- src/esx/esx_device_monitor.c | 2 - src/esx/esx_device_monitor.h | 1 - src/esx/esx_driver.c | 130 +++- src/esx/esx_driver.h | 1 - src/esx/esx_interface_driver.c| 25 +- src/esx/esx_interface_driver.h| 1 - src/esx/esx_network_driver.c | 57 ++-- src/esx/esx_network_driver.h | 1 - src/esx/esx_nwfilter_driver.c | 2 - src/esx/esx_nwfilter_driver.h | 1 - src/esx/esx_private.h | 1 - src/esx/esx_secret_driver.c | 2 - src/esx/esx_secret_driver.h | 1 - src/esx/esx_storage_backend_iscsi.c | 28 +- src/esx/esx_storage_backend_iscsi.h | 1 - src/esx/esx_storage_backend_vmfs.c| 113 ++- src/esx/esx_storage_backend_vmfs.h| 1 - src/esx/esx_storage_driver.c | 1 - src/esx/esx_storage_driver.h | 1 - src/esx/esx_util.c| 49 +-- src/esx/esx_vi.c | 126 +++- src/esx/esx_vi.h | 1 - src/esx/esx_vi_methods.c | 1 - src/esx/esx_vi_methods.h | 1 - src/esx/esx_vi_types.c| 33 +- src/esx/esx_vi_types.h| 1 - src/fdstream.c| 13 +- src/hyperv/hyperv_device_monitor.c| 4 - src/hyperv/hyperv_device_monitor.h| 1 - src/hyperv/hyperv_driver.c| 84 ++--- src/hyperv/hyperv_driver.h| 1 - src/hyperv/hyperv_interface_driver.c | 4 - src/hyperv/hyperv_interface_driver.h | 1 - src/hyperv/hyperv_network_driver.c| 4 - src/hyperv/hyperv_network_driver.h| 1 - src/hyperv/hyperv_nwfilter_driver.c | 4 - src/hyperv/hyperv_nwfilter_driver.h | 1 - src/hyperv/h
[libvirt] [PATCH 2/2] manual: Fix copy-paste errors
Descriptions for vol-download and vol-upload didn't make much of a sense. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=923613 --- tools/virsh.pod | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/virsh.pod b/tools/virsh.pod index 11447fe..768b334 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2541,7 +2541,7 @@ I is the name or key or path of the volume to delete. Upload the contents of I to a storage volume. I<--pool> I is the name or UUID of the storage pool the volume is in. -I is the name or key or path of the volume to wipe. +I is the name or key or path of the volume to write to. I<--offset> is the position in the storage volume at which to start writing the data. I<--length> is an upper bound of the amount of data to be uploaded. An error will occur if the I is greater than the specified length. @@ -2549,10 +2549,10 @@ An error will occur if the I is greater than the specified length. =item B [I<--pool> I] [I<--offset> I] [I<--length> I] I I -Download the contents of I from a storage volume. +Download the contents of a storage volume to I. I<--pool> I is the name or UUID of the storage pool the volume is in. -I is the name or key or path of the volume to wipe. +I is the name or key or path of the volume to download. I<--offset> is the position in the storage volume at which to start reading the data. I<--length> is an upper bound of the amount of data to be downloaded. -- 1.8.1.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] manual: Fix copy-paste errors
On 04/02/13 16:15, Martin Kletzander wrote: Descriptions for vol-download and vol-upload didn't make much of a sense. s/of a// Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=923613 --- tools/virsh.pod | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/virsh.pod b/tools/virsh.pod index 11447fe..768b334 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2541,7 +2541,7 @@ I is the name or key or path of the volume to delete. Upload the contents of I to a storage volume. I<--pool> I is the name or UUID of the storage pool the volume is in. -I is the name or key or path of the volume to wipe. +I is the name or key or path of the volume to write to. I'd write: I is the name or key or path of the volume where the file will be uploaded. I<--offset> is the position in the storage volume at which to start writing the data. I<--length> is an upper bound of the amount of data to be uploaded. An error will occur if the I is greater than the specified length. @@ -2549,10 +2549,10 @@ An error will occur if the I is greater than the specified length. =item B [I<--pool> I] [I<--offset> I] [I<--length> I] I I -Download the contents of I from a storage volume. +Download the contents of a storage volume to I. I<--pool> I is the name or UUID of the storage pool the volume is in. -I is the name or key or path of the volume to wipe. +I is the name or key or path of the volume to download. I<--offset> is the position in the storage volume at which to start reading the data. I<--length> is an upper bound of the amount of data to be downloaded. ACK with or without my suggestion used. Peter -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 1/4] virsh: Fix semantics of --config for "update-device" command
On 04/01/13 21:46, Laine Stump wrote: On 03/31/2013 05:22 PM, Peter Krempa wrote: The man page states that with --config the next boot is affected. This can be understood as if _only_ the next boot was affected. This isn't true if the machine is running. You should probably change this comment to make it clear that you're changing the behavior of the option, rather than the documentation of what it does. After our discussion last week, I do agree that, although this is a change in behavior of an already-released command, it's acceptable because 1) it was different behavior from all other commands using --config, and 2) it was documented as behaving as all other commands. This patch adds the full --live, --config, --current infrastructure and tweaks stuff to correctly support the obsolete --persistent flag. --- Notes: Version 2: - note in the docs that semantics of the flags were fixed ACK once you note in the commit log that the code was fixed, not the documentation. I mentioned this in the commit message, removed the stray newline in 3/4 and pushed this with the rest of the series. Thanks. Peter -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-glib 3/4] gconfig: Add GvirConfigStoragePoolTarget getters
On Tue, Mar 19, 2013 at 4:21 PM, Christophe Fergeau wrote: > --- ACK -- Regards, Zeeshan Ali (Khattak) FSF member#5124 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] manual: Add info about migrateuri in virsh manual
On 04/02/13 16:15, Martin Kletzander wrote: The virsh(1) man page wasn't saying anything about the 'migrateuri' parameter other than it can be usually omitted. A patched version of docs/migrate.html.in is taken in this patch to fix that up in the man page. --- docs/migration.html.in | 4 ++-- tools/virsh.pod| 28 +++- 2 files changed, 29 insertions(+), 3 deletions(-) ACK. Peter -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 07/11] Add SELinux and DAC labeling support for TPM passthrough
On 04/02/2013 07:15 AM, Stefan Berger wrote:
On 04/01/2013 05:06 PM, Corey Bryant wrote:
On 03/21/2013 11:42 AM, Stefan Berger wrote:
Signed-off-by: Stefan Berger
---
src/security/security_dac.c | 53 ++
src/security/security_selinux.c | 96
2 files changed, 149 insertions(+)
Index: libvirt/src/security/security_selinux.c
===
--- libvirt.orig/src/security/security_selinux.c
+++ libvirt/src/security/security_selinux.c
@@ -45,6 +45,7 @@
#include "virrandom.h"
#include "virutil.h"
#include "virconf.h"
+#include "virtpm.h"
#define VIR_FROM_THIS VIR_FROM_SECURITY
@@ -76,6 +77,12 @@ struct _virSecuritySELinuxCallbackData {
#define SECURITY_SELINUX_VOID_DOI "0"
#define SECURITY_SELINUX_NAME "selinux"
+static int
+virSecuritySELinuxRestoreSecurityTPMFileLabelInt(virSecurityManagerPtr
mgr,
+ virDomainDefPtr def,
+ virDomainTPMDefPtr tpm);
+
+
/*
* Returns 0 on success, 1 if already reserved, or -1 on fatal error
*/
@@ -1062,6 +1069,84 @@ err:
return rc;
}
+
+static int
+virSecuritySELinuxSetSecurityTPMFileLabel(virSecurityManagerPtr mgr,
+ virDomainDefPtr def,
+ virDomainTPMDefPtr tpm)
+{
+int rc;
+virSecurityLabelDefPtr seclabel;
+char *cancel_path;
+
+seclabel = virDomainDefGetSecurityLabelDef(def,
SECURITY_SELINUX_NAME);
+if (seclabel == NULL)
+return -1;
+
+switch (tpm->type) {
+case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
+rc = virSecuritySELinuxSetFilecon(
+ tpm->data.passthrough.source.data.file.path,
+ seclabel->imagelabel);
+if (rc < 0)
+return -1;
+
+if ((cancel_path = virTPMFindCancelPath()) != NULL) {
+rc = virSecuritySELinuxSetFilecon(cancel_path,
+ seclabel->imagelabel);
+VIR_FREE(cancel_path);
+if (rc < 0) {
+ virSecuritySELinuxRestoreSecurityTPMFileLabelInt(mgr, def,
+ tpm);
+return -1;
+}
+} else {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Cannot determine TPM command cancel
path"));
+return -1;
This makes me wonder if cancel-path should be specifiable at the
libvirt level rather than just using the default sysfs entry. If I've
read the code correctly I don't think it can currently be specified.
However QEMU is capable of taking a cancel-path string in case it is
different from the default sysfs path.
I am not sure whether to allow users to specify the path and
misconfigure it and to have QEMU write a letter into the wrong file. I
wonder whether we could have libvirt determine the path and display it
in the XML as read-only, though.
Stefan
After discussing with Stefan some more, I think just using the default
path is enough. I don't know why the sysfs path would not be the
default anyway. And as far as I know we've decided not to support fd
passing for vTPM, at least at this point, so that is not a concern.
--
Regards,
Corey Bryant
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] manual: Add info about migrateuri in virsh manual
On 04/02/2013 04:59 PM, Peter Krempa wrote: > On 04/02/13 16:15, Martin Kletzander wrote: >> The virsh(1) man page wasn't saying anything about the 'migrateuri' >> parameter other than it can be usually omitted. A patched version of >> docs/migrate.html.in is taken in this patch to fix that up in the man >> page. >> --- >> docs/migration.html.in | 4 ++-- >> tools/virsh.pod| 28 +++- >> 2 files changed, 29 insertions(+), 3 deletions(-) >> > > ACK. > > Peter Thanks, pushed both. Martin -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] v2:Support for adding a static route to a bridge
On 03/20/2013 11:17 AM, Laine Stump wrote: On 03/20/2013 10:57 AM, Gene Czarcinski wrote: On 03/16/2013 09:32 AM, Gene Czarcinski wrote: On 03/15/2013 03:48 PM, Gene Czarcinski wrote: On 03/15/2013 02:10 PM, Gene Czarcinski wrote: This patch adds support for adding a static route for a network. The "via" specifies the gateway's IP address. Both IPv4 and IPv6 static routes are supported although it is expected that this functionality will have more use with IPv6. ping Sorry, I've had a reply "almost" done for several days, but got pulled away to a fire drill. I'll finish it up and send it as soon as I can. ping Gene -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] virsh: Call virDomainFree in cmdDomFSTrim
The virsh domfstrim command was not freeing allocated domain, leaving leaked references behind. --- tools/virsh-domain.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 5ddcedc..5fbfeee 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -10058,6 +10058,8 @@ cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd) ret = true; cleanup: +if (dom) +virDomainFree(dom); return ret; } -- 1.8.1.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] Allow multiple parameters for schedinfo
ping, still applicable on master :)
On 03/21/2013 05:22 PM, Martin Kletzander wrote:
> virsh schedinfo was able to set only one parameter at a time (not
> counting the deprecated options), but it is useful to set more at
> once, so this patch adds the possibility to do stuff like this:
>
> virsh schedinfo cpu_shares=0 vcpu_period=0 vcpu_quota=0 \
> emulator_period=0 emulator_quota=0
>
> Invalid scheduler options are reported as well. These were previously
> reported only if the command hadn't updated any values (when
> cmdSchedInfoUpdate returned 0).
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=810078
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=919372
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=919375
>
> Signed-off-by: Martin Kletzander
> ---
> v2:
> - correctly report unsupported options
> - man page updated
>
> tests/virsh-schedinfo | 4 +-
> tools/virsh-domain.c | 119
> --
> tools/virsh.pod | 4 +-
> 3 files changed, 72 insertions(+), 55 deletions(-)
>
> diff --git a/tests/virsh-schedinfo b/tests/virsh-schedinfo
> index 4f462f8..37f7bd3 100755
> --- a/tests/virsh-schedinfo
> +++ b/tests/virsh-schedinfo
> @@ -1,7 +1,7 @@
> #!/bin/sh
> # Ensure that virsh schedinfo --set invalid=val fails
>
> -# Copyright (C) 2010-2011 Red Hat, Inc.
> +# Copyright (C) 2010-2011, 2013 Red Hat, Inc.
>
> # This program is free software: you can redistribute it and/or modify
> # it under the terms of the GNU General Public License as published by
> @@ -37,7 +37,7 @@ fi
> . "$srcdir/test-lib.sh"
>
> printf 'Scheduler : fair\n\n' > exp-out || framework_failure
> -printf 'error: invalid scheduler option: j=k\n' > exp-err ||
> framework_failure
> +printf 'error: invalid scheduler option: j\n' > exp-err || framework_failure
>
> fail=0
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 128e516..cc2eddc 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -3918,16 +3918,14 @@ static const vshCmdOptDef opts_schedinfo[] = {
> .flags = VSH_OFLAG_REQ,
> .help = N_("domain name, id or uuid")
> },
> -{.name = "set",
> - .type = VSH_OT_STRING,
> - .help = N_("parameter=value")
> -},
> {.name = "weight",
> .type = VSH_OT_INT,
> + .flags = VSH_OFLAG_REQ_OPT,
> .help = N_("weight for XEN_CREDIT")
> },
> {.name = "cap",
> .type = VSH_OT_INT,
> + .flags = VSH_OFLAG_REQ_OPT,
> .help = N_("cap for XEN_CREDIT")
> },
> {.name = "current",
> @@ -3942,72 +3940,100 @@ static const vshCmdOptDef opts_schedinfo[] = {
> .type = VSH_OT_BOOL,
> .help = N_("get/set value from running domain")
> },
> +{.name = "set",
> + .type = VSH_OT_ARGV,
> + .flags = VSH_OFLAG_NONE,
> + .help = N_("parameter=value")
> +},
> {.name = NULL}
> };
>
> static int
> +cmdSchedInfoUpdateOne(vshControl *ctl,
> + virTypedParameterPtr src_params, int nsrc_params,
> + virTypedParameterPtr *params,
> + int *nparams, int *maxparams,
> + const char *field, const char *value)
> +{
> +virTypedParameterPtr param;
> +int ret = -1;
> +int i;
> +
> +for (i = 0; i < nsrc_params; i++) {
> +param = &(src_params[i]);
> +
> +if (STRNEQ(field, param->field))
> +continue;
> +
> +if (virTypedParamsAddFromString(params, nparams, maxparams,
> +field, param->type,
> +value) < 0) {
> +vshSaveLibvirtError();
> +goto cleanup;
> +}
> +ret = 0;
> +break;
> +}
> +
> +if (ret < 0)
> +vshError(ctl, _("invalid scheduler option: %s"), field);
> +
> + cleanup:
> +return ret;
> +}
> +
> +static int
> cmdSchedInfoUpdate(vshControl *ctl, const vshCmd *cmd,
> virTypedParameterPtr src_params, int nsrc_params,
> virTypedParameterPtr *update_params)
> {
> -const char *set_arg;
> char *set_field = NULL;
> char *set_val = NULL;
> -virTypedParameterPtr param;
> +const char *val = NULL;
> +const vshCmdOpt *opt = NULL;
> virTypedParameterPtr params = NULL;
> int nparams = 0;
> int maxparams = 0;
> int ret = -1;
> int rv;
> -int val;
> -int i;
>
> -if (vshCommandOptString(cmd, "set", &set_arg) > 0) {
> -set_field = vshStrdup(ctl, set_arg);
> +while ((opt = vshCommandOptArgv(cmd, opt))) {
> +set_field = vshStrdup(ctl, opt->data);
> if (!(set_val = strchr(set_field, '='))) {
> -vshError(ctl, "%s", _("Invalid syntax for --set, expecting
> name=value"));
> +vshError(ctl, "%s", _("Invalid syntax for --set, "
> + "expecting name=value"));
> goto cleanup;
>
Re: [libvirt] [PATCH] virsh: Call virDomainFree in cmdDomFSTrim
On 04/02/2013 09:20 AM, Michal Privoznik wrote: > The virsh domfstrim command was not freeing allocated domain, > leaving leaked references behind. > --- > tools/virsh-domain.c | 2 ++ > 1 file changed, 2 insertions(+) ACK. > > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c > index 5ddcedc..5fbfeee 100644 > --- a/tools/virsh-domain.c > +++ b/tools/virsh-domain.c > @@ -10058,6 +10058,8 @@ cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd) > ret = true; > > cleanup: > +if (dom) > +virDomainFree(dom); > return ret; > } > > -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virsh: Call virDomainFree in cmdDomFSTrim
On 04/02/2013 05:20 PM, Michal Privoznik wrote:
> The virsh domfstrim command was not freeing allocated domain,
> leaving leaked references behind.
> ---
> tools/virsh-domain.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 5ddcedc..5fbfeee 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -10058,6 +10058,8 @@ cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd)
> ret = true;
>
> cleanup:
> +if (dom)
> +virDomainFree(dom);
> return ret;
> }
>
>
Alternatively, you could also get out of this with one line less:
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index d32218f..99f19a4 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -10057,7 +10057,7 @@ cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd)
unsigned int flags = 0;
if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
-goto cleanup;
+return false;
if (vshCommandOptULongLong(cmd, "minimum", &minimum) < 0) {
vshError(ctl, _("Unable to parse integer parameter minimum"));
@@ -10075,6 +10075,7 @@ cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd)
ret = true;
cleanup:
+virDomainFree(dom);
return ret;
}
--
However, your approach works as well. ACK.
Martin ;-)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virsh: Call virDomainFree in cmdDomFSTrim
On 04/02/2013 05:20 PM, Michal Privoznik wrote: > The virsh domfstrim command was not freeing allocated domain, > leaving leaked references behind. > --- > tools/virsh-domain.c | 2 ++ > 1 file changed, 2 insertions(+) > You could add the public bugzilla link to the commit message too: https://bugzilla.redhat.com/show_bug.cgi?id=928197 Jan -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-glib 4/4] gconfig: Add GvirConfigStoragePermission getters
On Tue, Mar 19, 2013 at 4:21 PM, Christophe Fergeau wrote: > --- ACK -- Regards, Zeeshan Ali (Khattak) FSF member#5124 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virsh: Call virDomainFree in cmdDomFSTrim
On 02.04.2013 17:32, Martin Kletzander wrote: > On 04/02/2013 05:20 PM, Michal Privoznik wrote: >> The virsh domfstrim command was not freeing allocated domain, >> leaving leaked references behind. >> --- >> tools/virsh-domain.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c >> index 5ddcedc..5fbfeee 100644 >> --- a/tools/virsh-domain.c >> +++ b/tools/virsh-domain.c >> @@ -10058,6 +10058,8 @@ cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd) >> ret = true; >> >> cleanup: >> +if (dom) >> +virDomainFree(dom); >> return ret; >> } >> >> > > Alternatively, you could also get out of this with one line less: > > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c > index d32218f..99f19a4 100644 > --- a/tools/virsh-domain.c > +++ b/tools/virsh-domain.c > @@ -10057,7 +10057,7 @@ cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd) > unsigned int flags = 0; > > if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) > -goto cleanup; > +return false; In fact, I prefer return ret here; as future changing of return value will require less lines changed and I am used to that. I went with my version. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-glib 1/4] gconfig: Add GVirConfigStoragePool getters
On Tue, Mar 19, 2013 at 4:21 PM, Christophe Fergeau wrote: > --- ACK -- Regards, Zeeshan Ali (Khattak) FSF member#5124 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virsh: Call virDomainFree in cmdDomFSTrim
On 04/02/2013 05:36 PM, Michal Privoznik wrote: > On 02.04.2013 17:32, Martin Kletzander wrote: >> On 04/02/2013 05:20 PM, Michal Privoznik wrote: >>> The virsh domfstrim command was not freeing allocated domain, >>> leaving leaked references behind. >>> --- >>> tools/virsh-domain.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c >>> index 5ddcedc..5fbfeee 100644 >>> --- a/tools/virsh-domain.c >>> +++ b/tools/virsh-domain.c >>> @@ -10058,6 +10058,8 @@ cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd) >>> ret = true; >>> >>> cleanup: >>> +if (dom) >>> +virDomainFree(dom); >>> return ret; >>> } >>> >>> >> >> Alternatively, you could also get out of this with one line less: >> >> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c >> index d32218f..99f19a4 100644 >> --- a/tools/virsh-domain.c >> +++ b/tools/virsh-domain.c >> @@ -10057,7 +10057,7 @@ cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd) >> unsigned int flags = 0; >> >> if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) >> -goto cleanup; >> +return false; > > In fact, I prefer return ret here; as future changing of return value > will require less lines changed and I am used to that. I went with my > version. > I'd personally prefer virDomainFree() to gracefully handle NULL as parameter, but life just ain't that simple ;-) [JK, there just wasn't much time to enjoy the April Fool's day.] Martin -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virsh: Call virDomainFree in cmdDomFSTrim
On 04/02/13 17:40, Martin Kletzander wrote: On 04/02/2013 05:36 PM, Michal Privoznik wrote: On 02.04.2013 17:32, Martin Kletzander wrote: On 04/02/2013 05:20 PM, Michal Privoznik wrote: The virsh domfstrim command was not freeing allocated domain, leaving leaked references behind. --- tools/virsh-domain.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 5ddcedc..5fbfeee 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -10058,6 +10058,8 @@ cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd) ret = true; cleanup: +if (dom) +virDomainFree(dom); return ret; } Alternatively, you could also get out of this with one line less: diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index d32218f..99f19a4 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -10057,7 +10057,7 @@ cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd) unsigned int flags = 0; if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) -goto cleanup; +return false; In fact, I prefer return ret here; as future changing of return value will require less lines changed and I am used to that. I went with my version. This actually doesn't conform with the rest of the virsh that either uses "goto cleanup" or "return false". I'd personally prefer virDomainFree() to gracefully handle NULL as parameter, but life just ain't that simple ;-) I tried that some time ago, wasn't accepted warmly :( [JK, there just wasn't much time to enjoy the April Fool's day.] The Czech republic didn't have the chance to but some of the libvirt folks had to work on monday unfortunately :) Martin Peter -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-glib] gconfig: Add calls to [gs]et_virt_type to domain tests
On Tue, Mar 19, 2013 at 4:20 PM, Christophe Fergeau wrote: > Setting GVirConfigDomain::virt_type is required for a working > domain configuration, using it in the test programs will be helpful > if people are using this as a base when starting to use libvirt-gconfig > --- ACK -- Regards, Zeeshan Ali (Khattak) FSF member#5124 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/5] virutil: Move string related functions to virstring.c
On 04/02/2013 08:22 AM, Michal Privoznik wrote: > --- > cfg.mk| 4 +- > daemon/libvirtd-config.c | 12 +- ... > src/util/virstatslinux.c | 1 - > src/util/virstoragefile.c | 9 +- Looks like most of the patch is fallout from new include file needed for using existing functions in their new location,... > src/util/virstring.c | 335 > src/util/virstring.h | 51 + > src/util/virsysinfo.c | 8 +- > src/util/virsysinfo.h | 2 +- > src/util/virtime.c| 1 - > src/util/virtypedparam.c | 3 +- > src/util/viruri.c | 4 +- > src/util/virusb.c | 7 +- > src/util/virutil.c| 350 > +- ...and that the split of virutil.c into virstring.h is the meat of the patch. > 270 files changed, 1505 insertions(+), 1455 deletions(-) Just from these numbers, I'm wondering if it would have been worth splitting this patch into at least two, so that the code motion portion of the patch is easier to find without having to hunt to the middle of the giant patch. More thoughts on splitting below. It is also possible to use 'git format-patch/send-email -O/path/to/file' where /path/to/file contains a list of shell globs that control the order in which the patch file is generated, so that the important changes (src/util/virstring.[ch], src/util/virutil.[ch]) come first, and the mechanical fallout last. > +++ b/daemon/libvirtd-config.c > @@ -23,15 +23,17 @@ > > #include > > +#include "configmake.h" > #include "libvirtd-config.h" > -#include "virconf.h" > +#include "remote/remote_driver.h" > +#include "remote/remote_protocol.h" > +#include "rpc/virnetserver.h" > #include "viralloc.h" > +#include "virconf.h" > #include "virerror.h" > #include "virlog.h" > -#include "rpc/virnetserver.h" > -#include "configmake.h" > -#include "remote/remote_protocol.h" > -#include "remote/remote_driver.h" > +#include "virstring.h" > +#include "virutil.h" You mixed sorting include names alongside adding the new include name, which makes it a bit harder to follow, and makes the diff longer to read. I would have split this into two to do include sorting first, code motion and addition of include virstring.h second. But making you redo things to split it now seems like a lot of work for little gain. Oh well. One thing is for certain - your commit message doesn't mention that you did more than just add a new include. It took me this far into the review to realize WHY your diff was so large (I was expecting lots of 1-line-additions for a new include, and was anticipating that things like a +12/-11 would be due to use of a renamed function, only to find out that you didn't rename any functions). Sorting includes is not necessarily bad, but I should have found about it from the commit message alone, not by diving into the patch itself. > +++ b/src/conf/domain_nwfilter.h > @@ -23,6 +23,8 @@ > #ifndef DOMAIN_NWFILTER_H > # define DOMAIN_NWFILTER_H > > +# include "domain_conf.h" > + > typedef int (*virDomainConfInstantiateNWFilter)(virConnectPtr conn, > const unsigned char *vmuuid, > virDomainNetDefPtr net); On the surface, this hunk feels completely unrelated. Obviously, it is fallout from some other file changing includes to be in alphabetical order, and it looks correct in isolation, but this is again evidence that you crammed a bit much into one patch. > +++ b/src/libvirt_private.syms > @@ -1722,9 +1722,25 @@ virStorageFileResize; > > > # util/virstring.h > +virArgvToString; > +virAsprintf; > +virSkipSpaces; > +virSkipSpacesAndBackslash; > +virSkipSpacesBackwards; > +virStrcpy; > virStringFreeList; > virStringJoin; > virStringSplit; > +virStrncpy; > +virStrToDouble; > +virStrToLong_i; > +virStrToLong_l; > +virStrToLong_ll; > +virStrToLong_ui; > +virStrToLong_ul; > +virStrToLong_ull; > +virTrimSpaces; > +virVasprintf; This list looks like a reasonable set of string-related functions. While we generally like to name functions with a prefix that matches the file it is found in, I agree that these names and this particular file are common enough to be an exception to that rule (that is, requiring someone to type something like virStrVasprintf would defeat the point of having a convenient asprintf wrapper). > +++ b/src/util/virstring.c > @@ -21,6 +21,10 @@ > > #include > > +#include > +#include > + > +#include "c-ctype.h" > #include "virstring.h" > #include "viralloc.h" > #include "virbuffer.h" > @@ -166,3 +170,334 @@ void virStringFreeList(char **strings) > } > VIR_FREE(strings); > } > + > +/* Like strtol, but produce an "int" result, and check more carefully
Re: [libvirt] [libvirt-glib] config: Fix 2 leaks in domain memory setters
On Thu, Mar 28, 2013 at 6:39 PM, Christophe Fergeau wrote: > --- ACK -- Regards, Zeeshan Ali (Khattak) FSF member#5124 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-glib 2/4] gconfig: Add GVirConfigStoragePoolSource getters
On Tue, Mar 19, 2013 at 4:21 PM, Christophe Fergeau wrote: > --- ACK -- Regards, Zeeshan Ali (Khattak) FSF member#5124 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-glib] build: Replace obsolete macro in configure.ac
On Fri, Mar 29, 2013 at 2:49 PM, Christophe Fergeau wrote: > On Fri, Mar 29, 2013 at 10:41:36AM +0100, Christophe Fergeau wrote: >> On Fri, Mar 29, 2013 at 09:34:51AM +0100, Christophe Fergeau wrote: >> > From: Stefano Facchini >> > diff --git a/configure.ac b/configure.ac >> > index 96dbf5a..2bf9c1b 100644 >> > --- a/configure.ac >> > +++ b/configure.ac >> > @@ -1,7 +1,7 @@ >> > AC_INIT(libvirt-glib, 0.1.7) >> > AC_CONFIG_SRCDIR(libvirt-glib/libvirt-glib-main.c) >> > AC_CONFIG_AUX_DIR([build-aux]) >> > -AM_CONFIG_HEADER(config.h) >> > +AC_CONFIG_HEADERS(config.h) >> > AC_CONFIG_MACRO_DIR([m4]) >> > dnl Make automake keep quiet about wildcards & other GNUmake-isms >> > AM_INIT_AUTOMAKE([-Wno-portability]) >> >> ACK from me, though I'll wait a bit before pushing in case there are some >> autoconf/automake subtleties I'm not aware of at play here :) > > Actually I'd tend to squash this in: > -AC_CONFIG_HEADERS(config.h) > +AC_CONFIG_HEADERS([config.h]) Sure. If you have tested this to work, ACK. -- Regards, Zeeshan Ali (Khattak) FSF member#5124 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Disable static libraries by default
From: "Daniel P. Berrange" Every source file is currently built twice by libtool, once for the shared library and once for the static library. Static libs are not commonly packaged by distros and slow down compilation time by as more than 50% compared to a shared-only build time. Time for 'make -j 4': shared only: 2 mins 9 secs shared + static: 3 mins 26 secs Time for non-parallel make shared only: 3 mins 32 secs shared + static: 5 mins 41 secs Those few people who really want them, can pass --enable-static to configure Disabling them by default requires use of LT_INIT, but for compat with RHEL5 we can't rely on that. So we conditionally use LT_INIT, but fallback to AM_PROG_LIBTOOL if not present. --- configure.ac | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 09e4ad9..0df9e5c 100644 --- a/configure.ac +++ b/configure.ac @@ -123,7 +123,11 @@ AC_TYPE_UID_T dnl Support building Win32 DLLs (must appear *before* AM_PROG_LIBTOOL) AC_LIBTOOL_WIN32_DLL -AM_PROG_LIBTOOL +m4_ifndef([LT_INIT], [ + AM_PROG_LIBTOOL +], [ + LT_INIT([shared disable-static]) +]) AM_PROG_CC_C_O AM_PROG_LD -- 1.8.1.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] sec_manager: Refuse to start domain with unsupported seclabel
https://bugzilla.redhat.com/show_bug.cgi?id=947387
If a user configures a domain to use a seclabel of a specific type,
but the appropriate driver is not accessible, we should refuse to
start the domain. For instance, if user requires selinux, but it is
either non present in the system, or is just disabled, we should not
start the domain. Moreover, since we are touching only those labels we
have a security driver for, the other labels may confuse libvirt when
reconnecting to a domain on libvirtd restart. In our selinux example,
when starting up a domain, missing security label is okay, as we
auto-generate one. But later, when libvirt is re-connecting to a live
qemu instance, we parse a state XML, where security label is required
and it is an error if missing:
error : virSecurityLabelDefParseXML:3228 : XML error: security label
is missing
This results in a qemu process left behind without any libvirt control.
---
src/security/security_manager.c | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index b671a91..757fe28 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -425,7 +425,7 @@ int virSecurityManagerGenLabel(virSecurityManagerPtr mgr,
virDomainDefPtr vm)
{
int ret = -1;
-size_t i;
+size_t i, j;
virSecurityManagerPtr* sec_managers = NULL;
virSecurityLabelDefPtr seclabel;
bool generated = false;
@@ -437,6 +437,19 @@ int virSecurityManagerGenLabel(virSecurityManagerPtr mgr,
return ret;
virObjectLock(mgr);
+for (i = 0; vm->nseclabels; i++) {
+for (j = 0; sec_managers[j]; j++)
+if (STREQ(vm->seclabels[i]->model, sec_managers[j]->drv->name))
+break;
+
+if (!sec_managers[j]) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Unable to find security driver for label %s"),
+ vm->seclabels[i]->model);
+goto cleanup;
+}
+}
+
for (i = 0; sec_managers[i]; i++) {
generated = false;
seclabel = virDomainDefGetSecurityLabelDef(vm,
sec_managers[i]->drv->name);
--
1.8.1.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv3 6/6] rpc: Fix connection close callback race condition and memory corruption/crash
On 03/31/2013 10:20 AM, Peter Krempa wrote:
> The last Viktor's effort to fix the race and memory corruption unfortunately
> wasn't complete in the case the close callback was not registered in an
> connection. At that time, the trail of event's that I'll describe later could
> still happend and corrupt the memory or cause a crash of the client (including
s/happend/happen/
> the daemon in case of a p2p migration).
>
> Consider the following prerequisities and trail of events:
> Let's have a remote connection to a hypervisor that doesn't have a close
> callback registered and the client is using the event loop. The crash happens
> in
> cooperation of 2 threads. Thread E is the event loop and thread W is the
> worker
> that does some stuff. R denotes the remote client.
>
> 1.) W - The client finishes everything and sheds the last reference on the
> client
> 2.) W - The virObject stuff invokes virConnectDispose that invokes
> doRemoteClose
> 3.) W - the remote close method invokes the REMOTE_PROC_CLOSE RPC method.
> 4.) W - The thread is preempted at this point.
> 5.) R - The remote side recieves the close and closes the socket.
s/recieves/receives/
> 6.) E - poll() wakes up due to the closed socket and invokes the close
> callback
> 7.) E - The event loop is preempted right before remoteClientCloseFunc is
> called
> 8.) W - The worker now finishes, and frees the conn object.
> 9.) E - The remoteClientCloseFunc accesses the now-freed conn object in the
> attempt to retrieve pointer for the real close callback.
> 10.) Kaboom, corrupted memory/segfault.
>
> This patch tries to fix this by introducing a new object that survives the
> freeing of the connection object. We can't increase the reference count on the
> connection object itself as the connection would never be closed as the
s/itself as/itself or/; s/closed as/closed, as/
> connection is closed only when the reference count reaches zero.
>
> The new object - virConnectCloseCallbackData - is a lockable object that keeps
> the pointers to the real user registered callback and ensures that the
> connection callback is either not called if the connection was already freed
> or
> that the connection isn't freed while this is being called.
> ---
> src/datatypes.c| 55
> src/datatypes.h| 22 ++
> src/libvirt.c | 29 ---
> src/remote/remote_driver.c | 57
> +++---
> 4 files changed, 112 insertions(+), 51 deletions(-)
So far, this is just a code review. I'm also planning on testing the
patch, and will report back with results later in the day.
> @@ -63,14 +65,19 @@ static void virStoragePoolDispose(void *obj);
> static int
> virDataTypesOnceInit(void)
> {
> -#define DECLARE_CLASS(basename) \
> -if (!(basename ## Class = virClassNew(virClassForObject(), \
> +#define DECLARE_CLASS_COMMON(basename, parent) \
> +if (!(basename ## Class = virClassNew(parent,\
>#basename, \
>sizeof(basename), \
>basename ## Dispose))) \
> return -1;
> +#define DECLARE_CLASS(basename) \
> +DECLARE_CLASS_COMMON(basename, virClassForObject())
> +#define DECLARE_CLASS_LOCKABLE(basename) \
> +DECLARE_CLASS_COMMON(basename, virClassForObjectLockable())
Wonder if these definitions are useful enough to rename to VIR_DECLARE_*
and move into virobject.h. But that would be a separate patch, to
adjust all clients that would benefit from it.
> +++ b/src/datatypes.h
> @@ -93,6 +93,22 @@ extern virClassPtr virStoragePoolClass;
> # define VIR_IS_DOMAIN_SNAPSHOT(obj) \
> (VIR_IS_SNAPSHOT(obj) && VIR_IS_DOMAIN((obj)->domain))
>
> +
> +typedef struct _virConnectCloseCallbackData virConnectCloseCallbackData;
> +typedef virConnectCloseCallbackData *virConnectCloseCallbackDataPtr;
> +
> +/**
> + * Internal structure holding data related to connection close callbacks.
> + */
> +struct _virConnectCloseCallbackData {
> +virObjectLockable parent;
> +
> +virConnectPtr conn;
> +virConnectCloseFunc callback;
> +void *opaque;
> +virFreeCallback freeCallback;
> +};
Would it make sense to move this struct definition to be local to
datatypes.c, and have all other uses of it treat it as opaque?...
> +++ b/src/libvirt.c
> @@ -20189,24 +20189,27 @@ int virConnectRegisterCloseCallback(virConnectPtr
> conn,
> virObjectRef(conn);
>
> virMutexLock(&conn->lock);
> +virObjectLock(conn->closeCallback);
>
> virCheckNonNullArgGoto(cb, error);
>
> -if (conn->closeCallback) {
> +if (conn->closeCallback->callback) {
...But then you would need to expose an accessor function instead of
dir
Re: [libvirt] [PATCH] sec_manager: Refuse to start domain with unsupported seclabel
On 04/02/2013 10:07 AM, Michal Privoznik wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=947387 > > If a user configures a domain to use a seclabel of a specific type, > but the appropriate driver is not accessible, we should refuse to > start the domain. For instance, if user requires selinux, but it is > either non present in the system, or is just disabled, we should not > start the domain. Moreover, since we are touching only those labels we > have a security driver for, the other labels may confuse libvirt when > reconnecting to a domain on libvirtd restart. In our selinux example, > when starting up a domain, missing security label is okay, as we > auto-generate one. But later, when libvirt is re-connecting to a live > qemu instance, we parse a state XML, where security label is required > and it is an error if missing: > > error : virSecurityLabelDefParseXML:3228 : XML error: security label > is missing > > This results in a qemu process left behind without any libvirt control. > --- > src/security/security_manager.c | 15 ++- > 1 file changed, 14 insertions(+), 1 deletion(-) ACK. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH libvirt-glib] Disable static libraries by default
On 04/02/2013 10:38 AM, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" > > Every source file is currently built twice by libtool, once for > the shared library and once for the static library. Static libs > are not commonly packaged by distros and slow down compilation > time by as more than 50% compared to a shared-only build time. s/by as more than/by as much as/ > > Time for non-parallel make > > shared only: 52 secs > shared + static: 1 min 26 secs > > Those few people who really want them, can pass --enable-static > to configure > > Disabling them by default requires use of LT_INIT. We don't need > to support older libtool, so drop use of AM_PROG_LIBTOOL entirely > --- > configure.ac | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) ACK. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Disable static libraries by default
On 04/02/2013 10:09 AM, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" > > Every source file is currently built twice by libtool, once for > the shared library and once for the static library. Static libs > are not commonly packaged by distros and slow down compilation > time by as more than 50% compared to a shared-only build time. s/by as more than/by as much as/ > > Time for 'make -j 4': > > shared only: 2 mins 9 secs > shared + static: 3 mins 26 secs > > Time for non-parallel make > > shared only: 3 mins 32 secs > shared + static: 5 mins 41 secs > > Those few people who really want them, can pass --enable-static > to configure Yep - we are just altering the defaults, not taking away the capability altogether. > > Disabling them by default requires use of LT_INIT, but for > compat with RHEL5 we can't rely on that. So we conditionally > use LT_INIT, but fallback to AM_PROG_LIBTOOL if not present. > --- > configure.ac | 6 +- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/configure.ac b/configure.ac > index 09e4ad9..0df9e5c 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -123,7 +123,11 @@ AC_TYPE_UID_T > dnl Support building Win32 DLLs (must appear *before* AM_PROG_LIBTOOL) > AC_LIBTOOL_WIN32_DLL > > -AM_PROG_LIBTOOL > +m4_ifndef([LT_INIT], [ > + AM_PROG_LIBTOOL > +], [ > + LT_INIT([shared disable-static]) > +]) ACK. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH libvirt-glib] Disable static libraries by default
From: "Daniel P. Berrange" Every source file is currently built twice by libtool, once for the shared library and once for the static library. Static libs are not commonly packaged by distros and slow down compilation time by as more than 50% compared to a shared-only build time. Time for non-parallel make shared only: 52 secs shared + static: 1 min 26 secs Those few people who really want them, can pass --enable-static to configure Disabling them by default requires use of LT_INIT. We don't need to support older libtool, so drop use of AM_PROG_LIBTOOL entirely --- configure.ac | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/configure.ac b/configure.ac index 96dbf5a..7b5a092 100644 --- a/configure.ac +++ b/configure.ac @@ -34,8 +34,7 @@ AC_SUBST([LIBVIRT_GLIB_VERSION_NUMBER]) AC_PROG_CC AM_PROG_CC_C_O -AC_LIBTOOL_WIN32_DLL -AC_PROG_LIBTOOL +LT_INIT([shared disable-static win32-dll]) dnl AC_CONFIG_LIBOBJ_DIR([src]) -- 1.8.1.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/5] Introduce VIR_STRDUP to replace strdup
On 04/02/2013 08:22 AM, Michal Privoznik wrote:
What about strndup? Then again, doing strndup in a second patch makes
sense.
> ---
>
> WARNING: THIS PATCH IS NOT COMPLETE !!!
>
> For full patch see the cover letter. I've trimmed the patch and left only
> interesting parts.
>
> HACKING | 5 ++
> cfg.mk| 8 ++
Good - a syntax check is necessary to enforce the new style :)
> src/xenxs/xen_xm.c| 38
> 183 files changed, 1133 insertions(+), 1128 deletions(-)
Big, but mostly mechanical fallout to comply to the syntax check.
>
> diff --git a/HACKING b/HACKING
> index c8833c0..6230ffd 100644
> --- a/HACKING
> +++ b/HACKING
> @@ -719,6 +719,11 @@ sizeof(dest) returns something meaningful). Note that
> this is a macro, so
> arguments could be evaluated more than once. This is equivalent to
> virStrncpy(dest, src, strlen(src), sizeof(dest)).
>
> + VIR_STRDUP(char *c)
> +
> +You should avoid using strdup directly as it does not report OOM error. Use
In HACKING, it might be worth spelling out "out-of-memory" in case the
reader is not yet familiar with the abbreviation.
> +VIR_STRDUP(c) macro instead (@c is type of char *).
> +
>
> Variable length string buffer
> =
> diff --git a/cfg.mk b/cfg.mk
> index 7a2c69f..8cc67a5 100644
> --- a/cfg.mk
> +++ b/cfg.mk
> @@ -383,6 +383,11 @@ sc_prohibit_asprintf:
> halt='use virAsprintf, not as'printf\
> $(_sc_search_regexp)
>
> +sc_prohibit_strdup:
> + @prohibit='\'
> \
> +halt='use VIR_STRUP, not strdup' \
s/STRUP/STRDUP/
When you add a later patch to cover strndup, this is easy enough to
generalize to cover both functions in one syntax checker.
If you use @prohibit='\ + $(_sc_search_regexp)
> +
> # Prefer virSetUIDGID.
> sc_prohibit_setuid:
> @prohibit='\ *\(' \
> @@ -814,6 +819,9 @@
> exclude_file_name_regexp--sc_prohibit_always_true_header_tests = \
> exclude_file_name_regexp--sc_prohibit_asprintf = \
>
> ^(bootstrap.conf$$|src/util/virstring\.c$$|examples/domain-events/events-c/event-test\.c$$)
>
> +exclude_file_name_regexp--sc_prohibit_strdup = \
> +
> ^(bootstrap.conf$$|cfg.mk$$|docs/|examples/|python/|src/storage/parthelper.c$$|src/util/virerror.c$$|src/util/virstring.c$$|tests/|tools/)
...you wouldn't have to list bootstrap.conf here. Also, instead of
exempting lots of entire files, it might be worth exempting just
specific uses of strdup, by using exclude='exempt from syntax-check'
(the way we do things for sc_prohibit_strtol).
> +++ b/daemon/libvirtd-config.c
> @@ -59,7 +59,7 @@ remoteConfigGetStringList(virConfPtr conf, const char *key,
> char ***list_arg,
> key);
> return -1;
> }
> -list[0] = strdup(p->str);
> +list[0] = VIR_STRDUP(p->str);
Hmm. What happens if VIR_STRDUP fails? We still have to check if
list[0] is NULL. Let's make sure this makes sense compared to what the
macro actually does...
> @@ -90,7 +90,7 @@ remoteConfigGetStringList(virConfPtr conf, const char *key,
> char ***list_arg,
> VIR_FREE(list);
> return -1;
> }
> -list[i] = strdup(pp->str);
> +list[i] = VIR_STRDUP(pp->str);
> if (list[i] == NULL) {
I'm assuming this is one of the places that you further touch later in
the series to simplify the cleanup paths to realize that the OOM message
has already been reported by VIR_STRDUP, and that this patch is just
using the new macro even if there are redundant error reports.
...oops, you trimmed out the one part I would have found most useful -
the actual definition of VIR_STRDUP.
/me goes and clones from your repo...
> commit ad304d40005e73e9ec60d26a35983b7e59f2562a
> Author: Michal Privoznik
> Date: Wed Mar 27 18:58:17 2013 +0100
>
> Introduce VIR_STRDUP to replace strdup
>
> diff --git a/src/util/virstring.c b/src/util/virstring.c
> index 5bcaf17..3847811 100644
> --- a/src/util/virstring.c
> +++ b/src/util/virstring.c
> @@ -96,7 +96,7 @@ char **virStringSplit(const char *string,
> if (VIR_RESIZE_N(tokens, maxtokens, ntokens, 1) < 0)
> goto no_memory;
>
> -if (!(tokens[ntokens] = strdup(remainder)))
> +if (!(tokens[ntokens] = VIR_STRDUP(remainder)))
> goto no_memory;
Again, assuming a later patch cleans things up once VIR_STRDUP does the
OOM reporting.
> ntokens++;
> }
> @@ -145,7 +145,7 @@ char *virStringJoin(const char **strings,
> }
> ret = virBufferContentAndReset(&buf);
> if (!ret) {
> -if (!(ret = strdup(""))) {
> +if (!(ret = VIR_STRDUP(""))) {
> virReportOOMError();
> return NULL;
> }
> @@ -501,3 +501,19 @@ vi
Re: [libvirt] [PATCH] Disable static libraries by default
On 04/02/2013 10:09 AM, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" > > Every source file is currently built twice by libtool, once for > the shared library and once for the static library. Static libs > are not commonly packaged by distros and slow down compilation > time by as more than 50% compared to a shared-only build time. > > Time for 'make -j 4': > > shared only: 2 mins 9 secs > shared + static: 3 mins 26 secs > > Time for non-parallel make > > shared only: 3 mins 32 secs > shared + static: 5 mins 41 secs > > Those few people who really want them, can pass --enable-static > to configure Question: should ./autobuild.sh be tweaked to pass --enable-static, just so that we have some test coverage that ensures we don't break things for that setup? -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Disable static libraries by default
On Tue, Apr 02, 2013 at 11:26:29AM -0600, Eric Blake wrote: > On 04/02/2013 10:09 AM, Daniel P. Berrange wrote: > > From: "Daniel P. Berrange" > > > > Every source file is currently built twice by libtool, once for > > the shared library and once for the static library. Static libs > > are not commonly packaged by distros and slow down compilation > > time by as more than 50% compared to a shared-only build time. > > > > Time for 'make -j 4': > > > > shared only: 2 mins 9 secs > > shared + static: 3 mins 26 secs > > > > Time for non-parallel make > > > > shared only: 3 mins 32 secs > > shared + static: 5 mins 41 secs > > > > Those few people who really want them, can pass --enable-static > > to configure > > Question: should ./autobuild.sh be tweaked to pass --enable-static, just > so that we have some test coverage that ensures we don't break things > for that setup? I guess so, though personally I'd just say no to static libraries entirely if there was a way todo that. Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [RFC 2/4] target-i386: Move feature word array outside get_register_name_32()
On Mon, Apr 01, 2013 at 04:46:47PM -0300, Eduardo Habkost wrote:
> The array will be used by the "feature-words" properties to output the
> register enum value.
Oops, the patch subject line is wrong: it was supposed to say "Move
register name array".
>
> Signed-off-by: Eduardo Habkost
> ---
> target-i386/cpu.c | 25 +
> 1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 974a8c6..2020147 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -194,23 +194,24 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS]
> = {
> },
> };
>
> +static const char *reg_names_32[CPU_NB_REGS32] = {
> +[R_EAX] = "EAX",
> +[R_ECX] = "ECX",
> +[R_EDX] = "EDX",
> +[R_EBX] = "EBX",
> +[R_ESP] = "ESP",
> +[R_EBP] = "EBP",
> +[R_ESI] = "ESI",
> +[R_EDI] = "EDI",
> +};
> +
> +
> const char *get_register_name_32(unsigned int reg)
> {
> -static const char *reg_names[CPU_NB_REGS32] = {
> -[R_EAX] = "EAX",
> -[R_ECX] = "ECX",
> -[R_EDX] = "EDX",
> -[R_EBX] = "EBX",
> -[R_ESP] = "ESP",
> -[R_EBP] = "EBP",
> -[R_ESI] = "ESI",
> -[R_EDI] = "EDI",
> -};
> -
> if (reg > CPU_NB_REGS32) {
> return NULL;
> }
> -return reg_names[reg];
> +return reg_names_32[reg];
> }
>
> /* collects per-function cpuid data
> --
> 1.8.1.4
>
>
--
Eduardo
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] v2:Support for adding a static route to a bridge
On 03/15/2013 02:10 PM, Gene Czarcinski wrote: > This patch adds support for adding a static route for > a network. The "via" specifies the gateway's IP > address. Both IPv4 and IPv6 static routes are > supported although it is expected that this > functionality will have more use with IPv6. (First I want to make sure I correctly understand what you're wanting to do, so I'm going to try and explain it in my own words...) >From reading your earlier messages, my understanding is that the aim of this patch is to automatically setup a route to a virtual network whose bridge device has no IP address assigned, and is therefore reachable only via one of the guests, is this correct? In other words, let's say that I have the following two networks defined (using IPv4 and all static IPs for brevity, but the entire discussion is equally applicable to IPv6): netVisibleToHost netHiddenFromHost and you have a guestDirect that has two interfaces: and another guestIndirect that has only one interface: Additionally, the default route on guestDirect points to 192.168.200.1, and the default route on guestIndirect points to 192.168.201.1. (Presumably you don't want to simply add an IP address to netHiddenFromHost because (while it would solve your routing problems) it would violate some security policy you've built into your network topology - namely that all traffic to and from netHiddenFromHost *must* go through guestDirect.) Traffic outbound from guestIndirect would have no problem reaching its destination - it would go across netHiddenFromHost to guestDirect (192.168.201.1), which would know to forward it to the host (192.168.200.1) via netVisibleToHost, and the host presumably knows how to get to anywhere. The problem comes when trying to route the *response* destined for 192.168.201.2 (guestIndirect) - the outside world may know that those packets have to be sent to the host, but the host doesn't have a route for 192.168.201.0/24 - only guestDirect does. So, the solution is to add a route on the *host* that points traffic destined for 192.168.201.0/24 to guestDirect, a.k.a. 192.168.200.2. Since there's no place in /etc/sysconfig/network-scripts/route-* to put static routes with destinations that are only reachable through a transient interface such as the bridge devices created by libvirt, the obvious solution is to cause libvirt to add such a route, and the way you propose to do that is to add an element in the network named "netUnreachable". Am I understanding the issue so far? Assuming that I am, then as far as I can see, the correct place to configure this route isn't in the setup for netHiddenFromHost, but rather in netVisibleToHost - this is more in line with the way static routes are configured in the standard host network config (route-* files), and eliminates the problem that would occur if netHiddenFromHost was started before netVisibleToHost existed (the route would be added pointing at the wrong interface, if at all) So, what I'm proposing is that, to automatically setup the route in the above example, netHiddenFromHost would remain unchanged, and netVisibleToHost would look something like this: netVisibleToHost The route element could also have a family attribute just as does (although it's fairly simple to figure out the family just by trying to parse address and/or gateway). You might instead want to make a subelement of , then validate that the gateway address is directly reachable from the given ip address (i.e. that it's on the same subnet). By putting the configuration here, you could be assured that the interface that will be used for the route will always exist at the time the route is added. Also it is conceptually more similar to the way that the routes in /etc/sysconfig/route-ethX all have gateways that are directly reachable via "ethX". *OR* (following is what I think is a bad idea, but maybe someone can tweak and salvage it :-) Here is an alternate proposal that has the advantage of tying the existence of the static route to the existence of not just the bridge, but of even the guest interface will be the gateway: instead of putting the route in the configuration of the netVisibleToHost network, put it directly in the configuration of the guest interface itself. That way when the guest is started the route will be added, and when the guest is shutdown, the route (which will anyway now be useless) will be removed. That could be added to the definition something like this: If this was done correctly, you could even hotplug a guest interface that would then be immediately used for a route, as well as updating an existing guest interface to add a route. The route added would use the information in the element plus the bridge device the guest's tap device was connected to. I do have several reservations about t
Re: [libvirt] libvirtd segfault
Op woensdag 20 maart 2013 08:42:52 schreef Jim Fehlig: > AL13N wrote: > > Thread 1 (Thread 0x7fdef683b800 (LWP 20522)): > > #0 0x in ?? () > > #1 0x7fdee9a72dc7 in libxl_osevent_occurred_timeout (ctx= > out>, for_libxl=0x7fdedc001608) at libxl_event.c:1039 > > #2 0x7fdee9c9ff87 in libxlTimerCallback (timer=, > > timer_info=0x7fdedc001730) at libxl/libxl_driver.c:259 > > #3 0x7fdef5dd0f1a in virEventPollDispatchTimeouts () at > > util/vireventpoll.c:450 > > #4 virEventPollRunOnce () at util/vireventpoll.c:643 > > #5 0x7fdef5dcf88d in virEventRunDefaultImpl () at util/virevent.c:273 > > #6 0x7fdef5ec96c5 in virNetServerRun (srv=0x1146220) at > > rpc/virnetserver.c:1108 > > #7 0x0040c8e0 in main (argc=, argv= > out>) at libvirtd.c:1481 > > I received a similar report on #xendevel just yesterday. I'll take a > look at this in the next few days after finishing my current project. > > Regards, > Jim restesting with libvirt-1.0.4 confirmed that it's still unfixed -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] virstring: Introduce virVasprintfNOOM and make virVasprintf report OOM
On 04/02/2013 08:22 AM, Michal Privoznik wrote:
> ---
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_domain.c | 4 +---
> src/util/viraudit.c | 2 +-
> src/util/vircommand.c| 4 ++--
> src/util/virerror.c | 2 +-
> src/util/virlog.c| 2 +-
> src/util/virstring.c | 22 --
> src/util/virstring.h | 3 +++
> 8 files changed, 30 insertions(+), 10 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 5060b87..8d1abe7 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1742,6 +1742,7 @@ virStrToLong_ul;
> virStrToLong_ull;
> virTrimSpaces;
> virVasprintf;
> +virVasprintfNOOM;
Bike-shedding: Maybe virVasprintfQuiet works better? I'm a bit worried
that the NOOM suffix will cause more questions than it resolves about
what it is really doing, which is being silent on OOM conditions.
> +++ b/src/qemu/qemu_domain.c
> @@ -1477,10 +1477,8 @@ int qemuDomainAppendLog(virQEMUDriverPtr driver,
> (fd = qemuDomainCreateLog(driver, obj, true)) < 0)
> goto cleanup;
>
> -if (virVasprintf(&message, fmt, argptr) < 0) {
> -virReportOOMError();
> +if (virVasprintf(&message, fmt, argptr) < 0)
> goto cleanup;
> -}
This hunk looks like it is in the wrong patch. Probably meant for 5/5?
> +++ b/src/util/viraudit.c
> @@ -96,7 +96,7 @@ void virAuditSend(const char *filename,
> #endif
>
> va_start(args, fmt);
> -if (virVasprintf(&str, fmt, args) < 0) {
> +if (virVasprintfNOOM(&str, fmt, args) < 0) {
> VIR_WARN("Out of memory while formatting audit message");
> str = NULL;
The str=NULL assignment is redundant with the guarantees of
virVasprintf*(), if you want to clean that up while you are touching
this area of code.
> +++ b/src/util/virerror.c
> @@ -649,7 +649,7 @@ virRaiseErrorFull(const char *filename ATTRIBUTE_UNUSED,
> } else {
> va_list ap;
> va_start(ap, fmt);
> -ignore_value(virVasprintf(&str, fmt, ap));
> +ignore_value(virVasprintfNOOM(&str, fmt, ap));
Given how seldom virVasprintfNOOM (or whatever name we give it) will be
used, it probably doesn't need ATTRIBUTE_RETURN_CHECK. After all, if
you know you don't care about an error being raised, then the compiler
shouldn't make you care about a successful return value either.
> @@ -327,6 +327,23 @@ virVasprintf(char **strp, const char *fmt, va_list list)
> }
>
> /**
> + * virVasprintf
> + *
> + * like glibc's vasprintf but makes sure *strp == NULL on failure and reports
> + * OOM error.
> + */
> +int
> +virVasprintf(char **strp, const char *fmt, va_list list)
> +{
> +int ret;
> +
> +if ((ret = virVasprintfNOOM(strp, fmt, list)) == -1)
s/== -1/< 0/
The idea makes sense, and the choices that you converted over to using
the silent variant look correct, but a v2 might be useful, particularly
after my comments on 2/5 about splitting the series differently. Anyone
else with a better idea for a name?
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] ANNOUNCE: libvirt 0.10.2.4 maintenance release
libvirt 0.10.2.4 maintenance release is now available. This is libvirt 0.10.2 with additional bugfixes that have accumulated upstream since the initial release. This release can be downloaded at: http://libvirt.org/sources/stable_updates/libvirt-0.10.2.4.tar.gz Changes in this version: * esx: Fix and improve esxListAllDomains function * Fix parsing of SELinux ranges without a category * Separate MCS range parsing from MCS range checking * Fix memory leak on OOM in virSecuritySELinuxMCSFind * qemu: Set migration FD blocking * build: further fixes for broken if_bridge.h * build: work around broken kernel header * Fix SELinux security label test * libxl: Fix setting of disk backend * util: Fix mask for 172.16.0.0 private address range * conf: don't fail to parse when parsing a single device * Support custom 'svirt_tcg_t' context for TCG based guests * uml: Report error if inotify fails on driver startup (cherry picked from commit 7b97030ad430eb76fcc333652411208fb702e962) * daemon: Preface polkit error output with 'polkit:' * spec: Fix script warning when uninstalling libvirt-client For info about past maintenance releases, see: http://wiki.libvirt.org/page/Maintenance_Releases Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/5] viralloc: Introduce OOM error reporting to VIR_ALLOC and friends
On 04/02/2013 08:22 AM, Michal Privoznik wrote:
> ---
> HACKING | 4 ++
> docs/hacking.html.in| 4 ++
> po/POTFILES.in | 1 +
> python/libvirt-override.c | 102
> ++--
> src/esx/esx_vi.c| 5 +--
> src/util/viralloc.c | 91 +--
> src/util/viralloc.h | 52 +++---
> src/util/virbuffer.c| 8 ++--
> src/util/virerror.c | 4 +-
> src/util/virthreadpthread.c | 2 +-
> tests/commandhelper.c | 2 +-
> tests/networkxml2conftest.c | 2 +-
> tests/openvzutilstest.c | 2 +-
> tests/test_conf.c | 2 +-
> tests/xmconfigtest.c| 2 +-
> 15 files changed, 188 insertions(+), 95 deletions(-)
>
> diff --git a/HACKING b/HACKING
> index 6230ffd..c511cab 100644
> --- a/HACKING
> +++ b/HACKING
> @@ -595,6 +595,10 @@ size:
>
>
>
> +These memory allocation macros report OOM error automatically. In case, you
> +want to suppress such behavior, use their NOOOM variant, e.g. VIR_ALLOCNOOM,
> +VIR_REALLOC_NNOOM etc.
Three different spellings in one sentence! NOOOM, NOOM, NNOOM. All the
more reason to consider my 3/5 suggestion of a simpler name, such as
VIR_ALLOC_QUIET.
> -if (VIR_ALLOC_N(ret, size) < 0) {
> +if (VIR_ALLOC_NNOOM(ret, size) < 0) {
Oh, I misread your spellings. If anything, have an underscore as part
of the suffix:
VIR_ALLOC_N_NOOM (or VIR_ALLOC_N_QUIET)
so that it is obvious that N (a count to alloc) and NOOM (or QUIET) is
an attribute of what to do on allocation failure, instead of wondering
if NNOOM is a typo.
> +++ b/src/esx/esx_vi.c
> @@ -1769,10 +1769,9 @@ esxVI_Alloc(void **ptrptr, size_t size)
> return -1;
> }
>
> -if (virAllocN(ptrptr, size, 1) < 0) {
> -virReportOOMError();
> +if (virAllocN(ptrptr, size, 1, true, VIR_FROM_THIS,
> + __FILE__, __FUNCTION__, __LINE__) < 0)
> return -1;
> -}
Can we rework this to use VIR_ALLOC_N_NOOM(*ptrptr, size 1)? instead of
the raw call to virAllocN, along with the cleanup to cfg.mk?
> @@ -270,7 +316,8 @@ int virResizeN(void *ptrptr, size_t size, size_t
> *allocptr, size_t count,
> void virShrinkN(void *ptrptr, size_t size, size_t *countptr, size_t toremove)
> {
> if (toremove < *countptr)
> -ignore_value(virReallocN(ptrptr, size, *countptr -= toremove));
> +ignore_value(virReallocN(ptrptr, size, *countptr -= toremove, false,
> + 0, NULL, NULL, 0));
How come you aren't adding location arguments to virResizeN and touching
up VIR_RESIZE_N to pass in location?
In fact, it may be worth splitting this into two patches - one that adds
location information to existing functions, and the second that
introduces new functions and adds the 'report' option.
> @@ -318,7 +365,8 @@ virInsertElementsN(void *ptrptr, size_t size, size_t at,
>
> if (inPlace) {
> *countptr += add;
> -} else if (virExpandN(ptrptr, size, countptr, add) < 0) {
> +} else if (virExpandN(ptrptr, size, countptr, add,
> + false, 0, NULL, NULL, 0) < 0) {
> return -1;
Another function where adding location may make sense.
> @@ -82,7 +90,9 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1);
> *
> * Returns -1 on failure, 0 on success
> */
> -# define VIR_ALLOC(ptr) virAlloc(&(ptr), sizeof(*(ptr)))
> +# define VIR_ALLOC(ptr) virAlloc(&(ptr), sizeof(*(ptr)), true,
> VIR_FROM_THIS, \
> + __FILE__, __FUNCTION__, __LINE__)
> +# define VIR_ALLOCNOOM(ptr) virAlloc(&(ptr), sizeof(*(ptr)), false, 0, NULL,
> NULL, 0)
Hmm. At first, I wondered if still passing VIR_FROM_THIS, __FILE__,
__FUNCTION__, __LINE__ on through to virAlloc, even if they aren't used,
would aid in debugging. But then I realized that if you can stop the
debugger on virAlloc failure, then you can also use the debugger to
determine the callstack. So this seems fine, after all (for the
trailing arguments - the choice of naming is still an open issue...).
> +++ b/tests/openvzutilstest.c
> @@ -101,7 +101,7 @@ testReadNetworkConf(const void *data ATTRIBUTE_UNUSED)
> " \n"
> "\n";
>
> -if (VIR_ALLOC(def) < 0 ||
> +if (VIR_ALLOCNOOM(def) < 0 ||
> !(def->os.type = strdup("exe")) ||
> !(def->os.init = strdup("/sbin/init")))
I thought 2/5 got rid of all raw strdup()? Or maybe the syntax check
rule exempted a bit too much? Again an argument for making things do a
per-use exemption, instead of a per-file exemption, the way we do for
strtol.
The places where you used the new function look sane. Looking forward
to v2 with saner names. Again, splitting into two patches (one to make
viralloc.h preserve locations but with no new functionality, and another
to add the new functionality) might make review easier.
--
Eric Blake eblake redhat com+1-91
[libvirt] [PATCH] qemu: Fix crash when updating media with shared device
Mimic the fix done in 02b9097274d1330c2e1dca7f598880e09b5c2aa0 to fix crash by
accessing an already freed structure. Also copy the explaining comment why the
pointer can't be accessed any more.
---
src/qemu/qemu_driver.c | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 96bf235..552a81b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6073,14 +6073,17 @@ qemuDomainChangeDiskMediaLive(virDomainObjPtr vm,
goto end;
ret = qemuDomainChangeEjectableMedia(driver, vm, disk, orig_disk,
force);
+/* 'disk' must not be accessed now - it has been freed.
+ * 'orig_disk' now points to the new disk, while 'dev_copy'
+ * now points to the old disk */
+
+/* Need to remove the shared disk entry for the original
+ * disk src if the operation is either ejecting or updating.
+ */
if (ret == 0) {
dev->data.disk = NULL;
-/* Need to remove the shared disk entry for the original
- * disk src if the operation is either ejecting or updating.
- */
-if (orig_disk->src && STRNEQ_NULLABLE(orig_disk->src, disk->src))
-ignore_value(qemuRemoveSharedDisk(driver, dev_copy->data.disk,
- vm->def->name));
+ignore_value(qemuRemoveSharedDisk(driver, dev_copy->data.disk,
+ vm->def->name));
}
break;
default:
--
1.8.1.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Fix crash when updating media with shared device
On 04/02/2013 03:18 PM, Peter Krempa wrote: > Mimic the fix done in 02b9097274d1330c2e1dca7f598880e09b5c2aa0 to fix crash by > accessing an already freed structure. Also copy the explaining comment why the > pointer can't be accessed any more. > --- > src/qemu/qemu_driver.c | 15 +-- > 1 file changed, 9 insertions(+), 6 deletions(-) ACK. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Fix crash when updating media with shared device
On 04/02/13 23:32, Eric Blake wrote: On 04/02/2013 03:18 PM, Peter Krempa wrote: Mimic the fix done in 02b9097274d1330c2e1dca7f598880e09b5c2aa0 to fix crash by accessing an already freed structure. Also copy the explaining comment why the pointer can't be accessed any more. --- src/qemu/qemu_driver.c | 15 +-- 1 file changed, 9 insertions(+), 6 deletions(-) ACK. Pushed. Thanks. Peter -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 10/16] Change variable name to be more specific to avoid confusion
Signed-off-by: Dan Walsh
---
bin/virt-sandbox-service | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service
index ca472f5..f32fd4a 100755
--- a/bin/virt-sandbox-service
+++ b/bin/virt-sandbox-service
@@ -551,13 +551,13 @@ PrivateNetwork=false
raise OSError(_("Failed to mount image %s on %s") % (self.image,
self.dest))
def save_config(self):
-config = self.get_config_path()
-if os.path.exists(config):
-os.remove(config)
-self.config.save_to_path(config)
+config_path = self.get_config_path()
+if os.path.exists(config_path):
+os.remove(config_path)
+self.config.save_to_path(config_path)
if selinux is not None:
-selinux.restorecon(config)
-sys.stdout.write(_("Created sandbox config %s\n") % config)
+selinux.restorecon(config_path)
+sys.stdout.write(_("Created sandbox config %s\n") % config_path)
def _get_image_path(self):
mounts = self.config.get_mounts()
--
1.8.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 14/16] Listing running sandbox containers takes a long time using the current protocol.
So I am execing a virsh list command to show all of the running containers.
---
bin/virt-sandbox-service | 17 +++--
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service
index b559cf5..ceb05b3 100755
--- a/bin/virt-sandbox-service
+++ b/bin/virt-sandbox-service
@@ -907,22 +907,27 @@ def usage(parser, msg):
sys.exit(1)
def sandbox_list(args):
+if args.running:
+import libvirt
+conn = libvirt.open("lxc:///")
+running = conn.listAllDomains(0)
+conn.close()
+running.sort()
+for d in running:
+print d.name()
+return
+
import glob
g = glob.glob(CONFIG_PATH + "*.sandbox")
g.sort()
for gc in g:
try:
c = LibvirtSandbox.Config.load_from_path(gc)
-if args.running:
-if c.running():
-print c.get_name()
-else:
-print c.get_name()
+print c.get_name()
except OSError, e:
print "Invalid container %s: %s", (gc, e)
-
def sandbox_reload(args):
config = read_config(args.name)
if isinstance(config, gi.repository.LibvirtSandbox.ConfigInteractive):
--
1.8.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 01/16] Free memory on exit, fixes a problem found by coverity.
Signed-off-by: Dan Walsh
---
bin/virt-sandbox-service-util.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/bin/virt-sandbox-service-util.c b/bin/virt-sandbox-service-util.c
index 4d164d8..430518f 100644
--- a/bin/virt-sandbox-service-util.c
+++ b/bin/virt-sandbox-service-util.c
@@ -194,8 +194,8 @@ static int set_process_label(pid_t pid) {
static int container_execute( GVirSandboxContext *ctx, const gchar *command,
pid_t pid ) {
int ret = EXIT_FAILURE;
-char **argv;
-int argc;
+char **argv = NULL;
+int argc=-1;
if (set_process_label(pid) < 0)
goto cleanup;
@@ -227,6 +227,10 @@ static int container_execute( GVirSandboxContext *ctx,
const gchar *command, pid
}
cleanup:
+if (argc > -1)
+free(argv[0]);
+free(argv);
+
return ret;
}
--
1.8.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 11/16] Change makedirs and makefiles to be internal methods
---
bin/virt-sandbox-service | 22 +++---
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service
index f32fd4a..9f4941b 100755
--- a/bin/virt-sandbox-service
+++ b/bin/virt-sandbox-service
@@ -453,7 +453,7 @@ PrivateNetwork=false
dest = root + "/" + d
self._fix_stat(dest[l:])
-def makedirs(self, d):
+def _makedirs(self, d):
try:
path = "%s%s" % (self.dest, d)
os.makedirs(path)
@@ -461,8 +461,8 @@ PrivateNetwork=false
if not e.errno == errno.EEXIST:
raise e
-def makefile(self, f):
-self.makedirs(os.path.dirname(f))
+def _makefile(self, f):
+self._makedirs(os.path.dirname(f))
try:
path = "%s%s" % (self.dest, f)
fd=open(path, "w")
@@ -476,20 +476,20 @@ PrivateNetwork=false
for d in self.dirs:
shutil.copytree(d, "%s%s" % (self.dest, d), symlinks=True)
for f in self.files:
-self.makedirs(os.path.dirname(f))
+self._makedirs(os.path.dirname(f))
shutil.copy(f, "%s%s" % (self.dest, f))
else:
for d in self.all_dirs:
-self.makedirs(d)
+self._makedirs(d)
for f in self.files:
-self.makedirs(os.path.dirname(f))
-self.makefile(f)
+self._makedirs(os.path.dirname(f))
+self._makefile(f)
for d in self.BIND_SYSTEM_DIRS + self.MAKE_SYSTEM_DIRS:
-self.makedirs(d)
+self._makedirs(d)
for f in self.BIND_SYSTEM_FILES:
-self.makefile(f)
+self._makefile(f)
shutil.copy(self.FUNCTIONS, "%s%s" % (self.dest, self.FUNCTIONS))
@@ -506,8 +506,8 @@ PrivateNetwork=false
unitdir = "/etc/systemd/system"
tgtdir = unitdir + "/multi-user.target.wants"
-self.makedirs(unitdir)
-self.makedirs(tgtdir)
+self._makedirs(unitdir)
+self._makedirs(tgtdir)
os.symlink("/run", self.dest + "/var/run")
for i, src in self.unit_file_list:
--
1.8.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 06/16] Wrap all output strings with _() to make sure we get proper translations.
Signed-off-by: Dan Walsh
---
bin/virt-sandbox-service | 48
1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service
index 0e38577..a064e9a 100755
--- a/bin/virt-sandbox-service
+++ b/bin/virt-sandbox-service
@@ -91,7 +91,7 @@ class Container:
if create:
if config and os.path.exists(config):
-raise ValueError(["Container already exists"])
+raise ValueError([_("Container already exists")])
self.config = LibvirtSandbox.ConfigService.new(name)
return
@@ -123,7 +123,7 @@ class Container:
def __get_sandbox_target(self):
if len(self.unit_file_list) > 1:
-raise ValueError(["Only Valid for single units"])
+raise ValueError([_("Only Valid for single units")])
return "%s_sandbox.target" % self.__get_sandboxed_service()
def __target(self):
@@ -178,8 +178,8 @@ WantedBy=%(TARGET)s
p = Popen(["/usr/bin/systemctl","enable", self.unitfile],stdout=PIPE,
stderr=PIPE)
p.communicate()
if p.returncode and p.returncode != 0:
-raise OSError("Failed to enable %s unit file " % self.unitfile)
-sys.stdout.write("Created unit file %s\n" % self.unitfile)
+raise OSError(_("Failed to enable %s unit file") % self.unitfile)
+sys.stdout.write(_("Created unit file %s\n") % self.unitfile)
def __add_dir(self, newd):
if newd in self.all_dirs:
@@ -229,7 +229,7 @@ WantedBy=%(TARGET)s
def get_name(self):
if self.config:
return self.config.get_name()
-raise ValueError(["Name not configured"])
+raise ValueError([_("Name not configured")])
def set_copy(self, copy):
self.copy = copy
@@ -255,10 +255,10 @@ WantedBy=%(TARGET)s
selabel = self.get_security_label()
if selabel is None:
-raise ValueError(["Missing security label configuration"])
+raise ValueError([_("Missing security label configuration")])
parts = selabel.split(":")
if len(parts) != 5 and len(parts) != 4:
-raise ValueError(["Expected 5 parts in SELinux security label %s"
% parts])
+raise ValueError([_("Expected 5 parts in SELinux security label
%s") % parts])
if len(parts) == 5:
selinux.chcon(self.dest, "system_u:object_r:%s:%s:%s" % (
@@ -276,7 +276,7 @@ WantedBy=%(TARGET)s
def set_name(self, name):
if self.config:
-raise ValueError(["Cannot modify Name"])
+raise ValueError([_("Cannot modify Name")])
self.dest = "%s/%s" % (self.path, self.name)
self.config = LibvirtSandbox.ConfigService.new(name)
@@ -317,7 +317,7 @@ WantedBy=%(TARGET)s
try:
h = mi.next();
except exceptions.StopIteration:
-raise ValueError(["Cannot find package containing %s" % unitfile])
+raise ValueError([_("Cannot find package containing %s") %
unitfile])
for fentry in h.fiFromHeader():
fname = fentry[0]
@@ -337,7 +337,7 @@ WantedBy=%(TARGET)s
try:
h = mi.next();
except exceptions.StopIteration:
-raise ValueError(["Cannot find base package %s" % srcrpmbits[0]])
+raise ValueError([_("Cannot find base package %s") %
srcrpmbits[0]])
for fentry in h.fiFromHeader():
fname = fentry[0]
@@ -528,7 +528,7 @@ PrivateNetwork=false
p = Popen(["/bin/umount", self.dest])
p.communicate()
if p.returncode and p.returncode != 0:
-raise OSError("Failed to unmount image %s from %s " %
(self.image, self.dest))
+raise OSError(_("Failed to unmount image %s from %s") %
(self.image, self.dest))
def __create_image(self):
fd = open(self.image, "w")
@@ -537,12 +537,12 @@ PrivateNetwork=false
p = Popen(["/sbin/mkfs","-F", "-t", "ext4", self.image],stdout=PIPE,
stderr=PIPE)
p.communicate()
if p.returncode and p.returncode != 0:
-raise OSError("Failed to build image %s" % self.image )
+raise OSError(_("Failed to build image %s") % self.image )
p = Popen(["/bin/mount", self.image, self.dest])
p.communicate()
if p.returncode and p.returncode != 0:
-raise OSError("Failed to mount image %s on %s " % (self.image,
self.dest))
+raise OSError(_("Failed to mount image %s on %s") % (self.image,
self.dest))
def save_config(self):
config = self.get_config_path()
@@ -551,7 +551,7 @@ PrivateNetwork=false
self.config.save_to_path(config)
if selinux is not None:
selinux.restorecon(config)
-sys.stdout.write("Created sandbox config %s\n" % config)
+sys.stdout.write(_("Created sandbox config %s\n") % config)
d
[libvirt] [PATCH 05/16] The command option is --copy not --clone
Signed-off-by: Dan Walsh --- bin/virt-sandbox-service-create.pod | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/bin/virt-sandbox-service-create.pod b/bin/virt-sandbox-service-create.pod index 1f82e1d..3fb8ae0 100644 --- a/bin/virt-sandbox-service-create.pod +++ b/bin/virt-sandbox-service-create.pod @@ -4,7 +4,7 @@ virt-sandbox-service create - Create a Security container =head1 SYNOPSIS - virt-sandbox-service [-c URI] create [-h] -u UNIT_FILE [ --clone ] [-p PATH] [-N NETWORK-OPTS] [-s SECURITY-OPTS] [-i SIZE] [-n] NAME + virt-sandbox-service [-c URI] create [-h] -u UNIT_FILE [ --copy ] [-p PATH] [-N NETWORK-OPTS] [-s SECURITY-OPTS] [-i SIZE] [-n] NAME =head1 DESCRIPTION @@ -34,9 +34,9 @@ supported currently). Name of the systemd unit file to be to run within the container. Can be repeated if multiple unit files are required within the sandbox. -=item B<-C>, B<--clone> +=item B<-C>, B<--copy> -Clone content from /etc and /var directories that will be mounted within the container. +Copy content from /etc and /var directories that will be mounted within the container. =item B<-p PATH>, B<--path PATH> -- 1.8.2 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 15/16] Use args.uri rather then hard coding lxc:///
---
bin/virt-sandbox-service | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service
index ceb05b3..1cce6a5 100755
--- a/bin/virt-sandbox-service
+++ b/bin/virt-sandbox-service
@@ -909,7 +909,7 @@ def usage(parser, msg):
def sandbox_list(args):
if args.running:
import libvirt
-conn = libvirt.open("lxc:///")
+conn = libvirt.open(args.uri)
running = conn.listAllDomains(0)
conn.close()
running.sort()
@@ -971,7 +971,7 @@ def fullpath(cmd):
return cmd
def execute(args):
-myexec = [ "virsh", "-c", "lxc:///", "lxc-enter-namespace" ]
+myexec = [ "virsh", "-c", args.uri, "lxc-enter-namespace" ]
#myexec = [ "virt-sandbox-service-util", "execute" ]
if args.noseclabel:
myexec.append("--noseclabel")
--
1.8.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] (no subject)
Most of them effect virt-sandbox-service, with the biggest change beeing the addition of InteractiveContainers. We want to be able to support OpenShift, which requres the abiltiy to run a command within a container environement but does not want to use the systemd/unitfiles. These containers will run a setup script and then a user process to actually run the application. >From Dan Walsh # This line is ignored. From: Dan Walsh Subject: In-Reply-To: -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 03/16] bash_completion scripts have added a new way to do completions, where you place you scripts in /usr/share/bash_completion/completions rather then /etc/bash_completions.d. We sh
Signed-off-by: Dan Walsh --- bin/Makefile.am | 18 +- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/bin/Makefile.am b/bin/Makefile.am index 69af01e..4f98aa4 100644 --- a/bin/Makefile.am +++ b/bin/Makefile.am @@ -5,7 +5,7 @@ libexec_PROGRAMS = virt-sandbox-service-util bin_SCRIPTS = virt-sandbox-service -virtsandboxcompdir = $(sysconfdir)/bash_completion.d/ +virtsandboxcompdir = $(datarootdir)/bash-completion/completions/ virtsandboxcomp_DATA = virt-sandbox-service-bash-completion.sh crondailydir = $(sysconfdir)/cron.daily @@ -82,6 +82,22 @@ virt_sandbox_service_util_LDFLAGS = \ $(WARN_CFLAGS) \ $(NULL) +install-virtsandboxcompDATA: + @$(NORMAL_INSTALL) + @list='$(virtsandboxcomp_DATA)'; test -n "$(virtsandboxcompdir)" || list=; \ + if test -n "$$list"; then \ + echo " $(MKDIR_P) '$(DESTDIR)$(virtsandboxcompdir)'"; \ + $(MKDIR_P) "$(DESTDIR)$(virtsandboxcompdir)" || exit 1; \ + fi; \ + for p in $$list; do \ + if test -f "$$p"; then d=; else d="$(srcdir)/"; fi; \ + echo "$$d$$p"; \ + done | $(am__base_list) | \ + while read files; do \ + echo " $(INSTALL_DATA) $$files '$(DESTDIR)$(virtsandboxcompdir)'"; \ + $(INSTALL_DATA) $$files "$(DESTDIR)$(virtsandboxcompdir)"/virt-sandbox-service || exit $$?; \ + done + install-data-local: $(MKDIR_P) $(DESTDIR)$(sysconfdir)/libvirt-sandbox/services -- 1.8.2 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 13/16] Add InteractiveContainer support. First use case will be OpenShift.
Differentiating on which kind of container to create based off of the
--command == InteractiveContainer
--unitfile == ServiceContainer
Resorted create args to be shown aphabetically except for the --command and
--unitfile which I want to come at the end.
---
bin/virt-sandbox-service | 139 +--
1 file changed, 99 insertions(+), 40 deletions(-)
diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service
index f4d0eff..b559cf5 100755
--- a/bin/virt-sandbox-service
+++ b/bin/virt-sandbox-service
@@ -413,6 +413,45 @@ class Container:
mount = LibvirtSandbox.ConfigMountRam.new(dest, size);
self.config.add_mount(mount)
+class InteractiveContainer(Container):
+def __init__(self, name=None, uri = "lxc:///", path =
Container.DEFAULT_PATH, config=None, create=False):
+Container.__init__(self, name, uri, path, config, create)
+
+if create:
+self.config = LibvirtSandbox.ConfigInteractive.new(name)
+
+def _gen_filesystems(self):
+Container._gen_filesystems(self)
+self.add_bind_mount(self.dest, self.path)
+
+def _create(self):
+#
+# Create an InteractiveContainer
+#
+Container.create(self)
+self._gen_filesystems()
+
+if self.image:
+self._create_image()
+self._umount()
+sys.stdout.write(_("Created sandbox container image %s\n") %
self.image)
+else:
+sys.stdout.write(_("Created sandbox container dir %s\n") %
self.dest)
+self.save_config()
+
+def create(self):
+try:
+self._create()
+except Exception, e:
+try:
+self._delete()
+except Exception, e2:
+pass
+raise e
+
+def set_command(self, command):
+self.config.set_command(command)
+
class ServiceContainer(Container):
IGNORE_DIRS= [ "/var/run/", "/etc/logrotate.d/", "/etc/pam.d" ]
DEFAULT_DIRS = [ "/etc", "/var" ]
@@ -836,19 +875,26 @@ MB = int(100)
def delete(args):
config = read_config(args.name)
-container = ServiceContainer(uri=args.uri, config = config)
+if isinstance(config, gi.repository.LibvirtSandbox.ConfigInteractive):
+container = InteractiveContainer(uri=args.uri, config = config)
+else:
+container = ServiceContainer(uri=args.uri, config = config)
container.delete()
def create(args):
-container = ServiceContainer(name = args.name, uri=args.uri, create = True)
-container.set_copy(args.copy)
+if args.command:
+container = InteractiveContainer(name = args.name, uri=args.uri,
create = True)
+container.set_command(args.command.split())
+else:
+container = ServiceContainer(name = args.name, uri=args.uri, create =
True)
+container.set_copy(args.copy)
+container.set_unit_file_list(args.unitfiles)
for net in args.network:
container.add_network(net)
if args.security:
container.set_security(args.security)
-container.set_unit_file_list(args.unitfiles)
container.set_path(args.path)
-
+container.set_file_type(args.file_type)
if args.imagesize:
container.set_image(args.imagesize)
@@ -879,17 +925,21 @@ def sandbox_list(args):
def sandbox_reload(args):
config = read_config(args.name)
+if isinstance(config, gi.repository.LibvirtSandbox.ConfigInteractive):
+raise ValueError(_("Interactive Containers do not support reload"))
container = ServiceContainer(uri = args.uri, config = config)
container.reload(args.unitfiles)
def start(args):
os.execl("/usr/libexec/virt-sandbox-service-util",
"virt-sandbox-service-util","-s", args.name)
-#container = Container(args.name, args.uri)
+#config = read_config(args.name)
+#container = Container(uri = args.uri, config=config)
#container.start()
def stop(args):
os.execl("/usr/libexec/virt-sandbox-service-util",
"virt-sandbox-service-util","-S", args.name)
-#container = Container(args.name, args.uri)
+#config = read_config(args.name)
+#container = Container(uri = args.uri, config=config)
#container.stop()
def connect(args):
@@ -898,7 +948,8 @@ Connected to %s.
Type 'Ctrl + ]' to detach from the console.
""" % ( args.name )
os.execl("/usr/libexec/virt-sandbox-service-util",
"virt-sandbox-service-util","-a", args.name)
-#container = Container(args.name, args.uri)
+#config = read_config(args.name)
+#container = Container(uri = args.uri, config=config)
#container.connect()
#
@@ -945,25 +996,27 @@ def clone(args):
shutil.copytree(old_path, new_path, symlinks=True)
sys.stdout.write(_("Created sandbox container dir %s\n") % new_path)
-fd = open(container.get_unit_path())
-recs = fd.read()
-fd.close()
-
-fd = open(container.get_unit_path(args.dest), "wx")
-fd.write(recs.repl
