Re: [libvirt] [PATCH]show the autostart status when displaya'virsh dominfo'.
On Wed, May 28, 2008 at 02:18:50PM +0100, Daniel P. Berrange wrote: On Wed, May 28, 2008 at 11:21:05AM +0900, Atsushi SAKAI wrote: Hi, Rich and Dan Thank you for fixing this. I think it should be noted about submitting patch on HACKING file or others. How do you think? (like make check; make syntax-check; make tests) Yes, basically the three things you should run before committing patches are make check make syntax-check make valgrind(in the tests/ sub-dir) The latter checks for memory leaks. Of course I myself forget this often - which is why we have the automated nightly builds, so we never miss the problem for longer than a day Suggested patch to HACKING file. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top ? scripts/Makefile ? scripts/Makefile.in Index: HACKING === RCS file: /data/cvs/libvirt/HACKING,v retrieving revision 1.3 diff -u -p -r1.3 HACKING --- HACKING 23 May 2008 08:24:41 - 1.3 +++ HACKING 2 Jun 2008 09:42:09 - @@ -2,6 +2,46 @@ Libvirt contributor guidelines == +General tips for contributing patches += + +(1) Discuss any large changes on the mailing list first. Post patches +early and listen to feedback. + +(2) Post patches in unified diff format. A command similar to this +should work: + + diff -urp libvirt.orig/ libvirt.modified/ libvirt-myfeature.patch + +or: + + cvs diff -up libvirt-myfeature.patch + +(3) Split large changes into a series of smaller patches, self-contained +if possible, with an explanation of each patch and an explanation of how +the sequence of patches fits together. + +(4) Make sure your patches apply against libvirt CVS. Developers +only follow CVS and don't care much about released versions. + +(5) Run the automated tests on your code before submitting any changes. +In particular, configure with compile warnings set to -Werror: + + ./configure --enable-compile-warnings=error + +and run the tests: + + make check + make syntax-check + make -C tests valgrind + +The latter test checks for memory leaks. + +(6) Update tests and/or documentation, particularly if you are adding +a new feature or changing the output of a program. + + + Code indentation Libvirt's C source code generally adheres to some basic code-formatting @@ -198,4 +238,4 @@ complexity it's best to stick to the fol Of particular note: *DO NOT* include libvirt/libvirt.h or libvirt/virterror.h. It is included by internal.h already and there are some special reasons why you cannot include these files -explicitly. \ No newline at end of file +explicitly. -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH]show the autostart status when displaya'virsh dominfo'.
On Mon, Jun 02, 2008 at 10:39:36AM +0100, Richard W.M. Jones wrote: On Wed, May 28, 2008 at 02:18:50PM +0100, Daniel P. Berrange wrote: On Wed, May 28, 2008 at 11:21:05AM +0900, Atsushi SAKAI wrote: Hi, Rich and Dan Thank you for fixing this. I think it should be noted about submitting patch on HACKING file or others. How do you think? (like make check; make syntax-check; make tests) Yes, basically the three things you should run before committing patches are make check make syntax-check make valgrind(in the tests/ sub-dir) The latter checks for memory leaks. Of course I myself forget this often - which is why we have the automated nightly builds, so we never miss the problem for longer than a day Suggested patch to HACKING file. Nice writeup ! +1 Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ [EMAIL PROTECTED] | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH]show the autostart status when displaya'virshdominfo'.
Hi, Rich Thank you for your patch. The document discribes more detail than I expected :-). Thanks Atsushi SAKAI Richard W.M. Jones [EMAIL PROTECTED] wrote: On Wed, May 28, 2008 at 02:18:50PM +0100, Daniel P. Berrange wrote: On Wed, May 28, 2008 at 11:21:05AM +0900, Atsushi SAKAI wrote: Hi, Rich and Dan Thank you for fixing this. I think it should be noted about submitting patch on HACKING file or others. How do you think? (like make check; make syntax-check; make tests) Yes, basically the three things you should run before committing patches are make check make syntax-check make valgrind(in the tests/ sub-dir) The latter checks for memory leaks. Of course I myself forget this often - which is why we have the automated nightly builds, so we never miss the problem for longer than a day Suggested patch to HACKING file. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH]show the autostart status when displaya'virsh dominfo'.
On Mon, Jun 02, 2008 at 07:03:15AM -0400, Daniel Veillard wrote: On Mon, Jun 02, 2008 at 10:39:36AM +0100, Richard W.M. Jones wrote: On Wed, May 28, 2008 at 02:18:50PM +0100, Daniel P. Berrange wrote: On Wed, May 28, 2008 at 11:21:05AM +0900, Atsushi SAKAI wrote: Hi, Rich and Dan Thank you for fixing this. I think it should be noted about submitting patch on HACKING file or others. How do you think? (like make check; make syntax-check; make tests) Yes, basically the three things you should run before committing patches are make check make syntax-check make valgrind(in the tests/ sub-dir) The latter checks for memory leaks. Of course I myself forget this often - which is why we have the automated nightly builds, so we never miss the problem for longer than a day Suggested patch to HACKING file. Nice writeup ! +1 Committed this. It's probably not perfect, so feel free to fix any problems ... Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://et.redhat.com/~rjones/virt-df/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] PATCH: Switch remote daemon to use memory alloc APIs
On Fri, May 30, 2008 at 11:02:22AM -0400, Daniel Veillard wrote: On Fri, May 30, 2008 at 03:37:06PM +0100, Daniel P. Berrange wrote: THis patch switches over the remote daemon to use the memory allocation APIs. This required exporting the 4 apis in the .so. I discovered along the way that none of the remote RPC dispatcher impls checked for malloc failure, so I've had to add that in too. Looks fine, this really cleans things up, especially reallocs if (getsockname(sock-fd, (struct sockaddr *)(sa), salen) 0) -return -1; +goto cleanup; Hum, changes the semantic but it looks like it will avoid leaking file descriptors too.. Yes, that is correct - we were leaking FDs. It was a minor issue though because the daemon will shortly exit once control returns Regards, Daniel -- |: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] PATCH: Switch all remaining code to memory alloc APIs
On Fri, May 30, 2008 at 12:13:42PM -0400, Daniel Veillard wrote:
On Fri, May 30, 2008 at 04:01:39PM +0100, Daniel P. Berrange wrote:
This patch switches all remaining code over to use the memory allocation
APIs, with exception of virsh which is going to be slightly more complex
It was mostly a straight conversion - there were only a few places which
weren't checking for failure corecttly - the most notable being sexpr.c.
[...]
-void *stack, *stacktop;
+char *stack, *stacktop;
/* allocate a stack for the container */
-stack = malloc(stacksize);
-if (!stack) {
+if (VIR_ALLOC_N(stack, stacksize) 0) {
hum, interesting side effect ... we must type stuff with the new macros.
Yes, that is correct - the macros use sizeof() to automatically determine
the size of the alloc needed. Although GCC treats sizeof(void) as being
the same as sizeof(char), this is not required by the C standard - it is
technically 'undefined behaviour'. So its safest to just switchto using
a char * for the stack here. An earlier function dealing with stacks in
this same file was already using char * too.
@@ -1659,8 +1659,7 @@
/* The allocated memory to cpumap must be 'sizeof(uint64_t)' byte *
* for Xen, and also nr_cpus must be 'sizeof(uint64_t) * 8'
*/
if (maplen 8) {
-new = calloc(1, sizeof(uint64_t));
-if (!new) {
+if (VIR_ALLOC_N(new, sizeof(uint64_t)) 0) {
That one worried me, but that works but because we have unsigned char *new
Yeah, I was undecided whether to use sizeof(uint64_t) here, or just hardcode
the value 8 to match the line earlier.
--- a/src/xmlrpc.c Fri May 30 10:36:42 2008 -0400
+++ b/src/xmlrpc.c Fri May 30 10:55:44 2008 -0400
@@ -12,6 +12,7 @@
Hum, i think that's dead code anyway, no ?
Yes, although you might end up using it for VMWare driver if you use the
webservices API ?
@@ -47,9 +48,8 @@
static xmlRpcValuePtr xmlRpcValueNew(xmlRpcValueType type)
{
-xmlRpcValuePtr ret = malloc(sizeof(*ret));
-
-if (!ret)
+xmlRpcValuePtr ret = NULL;
+if (VIR_ALLOC(ret) 0)
I don't think we need to set ret to NULL, do we ? VIR_ALLOC always
initialize.
Yes, that is redundant.
Dan.
--
|: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] PATCH: Switch all remaining code to memory alloc APIs
On Mon, Jun 02, 2008 at 01:49:13PM +0100, Daniel P. Berrange wrote: On Fri, May 30, 2008 at 12:13:42PM -0400, Daniel Veillard wrote: --- a/src/xmlrpc.cFri May 30 10:36:42 2008 -0400 +++ b/src/xmlrpc.cFri May 30 10:55:44 2008 -0400 @@ -12,6 +12,7 @@ Hum, i think that's dead code anyway, no ? Yes, although you might end up using it for VMWare driver if you use the webservices API ? Sigh if only VMWare had used the XML-RPC mechnism, but no they went the bloated SOAP way ... Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ [EMAIL PROTECTED] | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] PATCH: Switch all remaining code to memory alloc APIs
Daniel P. Berrange [EMAIL PROTECTED] wrote:
This patch switches all remaining code over to use the memory allocation
APIs, with exception of virsh which is going to be slightly more complex
It was mostly a straight conversion - there were only a few places which
weren't checking for failure corecttly - the most notable being sexpr.c.
...
Nice work (but tedious to review!).
I went through the whole thing this time.
Comments below.
diff -r ff6b92c70738 src/conf.c
...
@@ -897,15 +897,16 @@
fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR );
if (fd 0) {
+char *tmp = virBufferContentAndReset(buf);
virConfError(NULL, VIR_ERR_WRITE_FAILED, _(failed to open file),
0);
-free(virBufferContentAndReset(buf));
+VIR_FREE(tmp);
return -1;
}
...
diff -r ff6b92c70738 src/remote_internal.c
--- a/src/remote_internal.c Fri May 30 10:36:42 2008 -0400
+++ b/src/remote_internal.c Fri May 30 10:55:44 2008 -0400
...
@@ -3749,14 +3744,13 @@
for (ninteract = 0 ; interact[ninteract].id != 0 ; ninteract++)
; /* empty */
-*cred = calloc(ninteract, sizeof(*cred));
-if (!*cred)
+if (VIR_ALLOC_N(*cred, ninteract) 0)
return -1;
Cool! You fixed a subtle bug right there.
The old code should have read like this:
*cred = calloc(ninteract, sizeof(**cred));
Looks like it would have caused heap corruption, since sizeof(*cred)
is smaller than sizeof(**cred).
...
diff -r ff6b92c70738 src/storage_backend_logical.c
--- a/src/storage_backend_logical.c Fri May 30 10:36:42 2008 -0400
+++ b/src/storage_backend_logical.c Fri May 30 10:55:44 2008 -0400
...
@@ -266,7 +264,7 @@
memset(zeros, 0, sizeof(zeros));
/* XXX multiple pvs */
-if ((vgargv = malloc(sizeof(char*) * (1))) == NULL) {
+if (VIR_ALLOC_N(vgargv, 1) 0) {
virStorageReportError(conn, VIR_ERR_NO_MEMORY, %s, _(command
line));
That can be just
if (VIR_ALLOC(vgargv) 0) {
...
diff -r ff6b92c70738 src/xen_unified.c
--- a/src/xen_unified.c Fri May 30 10:36:42 2008 -0400
+++ b/src/xen_unified.c Fri May 30 10:55:44 2008 -0400
@@ -40,6 +40,7 @@
#include xm_internal.h
#include xml.h
#include util.h
+#include memory.h
#define DEBUG(fmt,...) VIR_DEBUG(__FILE__, fmt,__VA_ARGS__)
#define DEBUG0(msg) VIR_DEBUG(__FILE__, %s, msg)
@@ -172,15 +173,12 @@
if (xenUnifiedNodeGetInfo(dom-conn, nodeinfo) 0)
return(NULL);
-cpulist = calloc(nb_cpu, sizeof(*cpulist));
-if (cpulist == NULL)
+if (VIR_ALLOC_N(cpulist, nb_cpu) 0)
goto done;
-cpuinfo = malloc(sizeof(*cpuinfo) * nb_vcpu);
-if (cpuinfo == NULL)
+if (VIR_ALLOC_N(cpuinfo, nb_vcpu) 0)
goto done;
cpumaplen = VIR_CPU_MAPLEN(VIR_NODEINFO_MAXCPUS(nodeinfo));
-cpumap = (unsigned char *) calloc(nb_vcpu, cpumaplen);
-if (cpumap == NULL)
+if (VIR_ALLOC_N(cpumap, nb_vcpu * cpumaplen) 0)
goto done;
At first I thought it didn't matter that the product wasn't
checked for overflow, but then I spent a couple minutes trying
to find if/where nb_vcpu was guaranteed to be small enough
that we don't have to worry. There may well be code to ensure
that, but if so, it's too far from this point of use for my taste,
so I think it's best to add an explicit overflow check here, i.e.,
if (xalloc_oversized(nb_vcpu, cpumaplen) ||
VIR_ALLOC_N(cpumap, nb_vcpu * cpumaplen) 0)
goto done;
...
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] kvm 32bit guest on 64bit host
Hi,
I want to manage a 32 bit i686 guest on an x86_64 host with kvm and libvirt.
KVM is perfectly capable of doing this (look e.g. at
http://kvm.qumranet.com/kvmwiki/Guest_Support_Status) but libvirt did not
allow it.
So I created the attached patch to allow it and make it easy to extend the
capabilities of kvm and kqemu later on.
I did not add anything I'm not sure about, so I did not at 64bit guest on
32bit host (though the page linked above seems to indicate that it works) and
did not change the capabilities of kqemu at all (because I don't have any
experience with it and never used it).
Please consider merging.
Kind regards,
Gerd
--
Address (better: trap) for people I really don't want to get mail from:
[EMAIL PROTECTED]
diff -r -u libvirt-0.4.2.orig/src/qemu_conf.c libvirt-0.4.2/src/qemu_conf.c
--- libvirt-0.4.2.orig/src/qemu_conf.c 2008-05-29 21:25:01.0 +0200
+++ libvirt-0.4.2/src/qemu_conf.c 2008-06-03 01:01:19.0 +0200
@@ -293,6 +293,11 @@
int nflags;
};
+struct hypervisor_capabilities {
+const char *hostarch;
+const char *guestarch;
+};
+
/* Feature flags for the architecture info */
static const struct qemu_feature_flags const arch_info_i686_flags [] = {
{ pae, 1, 0 },
@@ -329,6 +334,17 @@
/usr/bin/xenner, arch_info_x86_64_flags, 2 },
};
+static const struct hypervisor_capabilities const kvm_capa[] = {
+{ i686, i686 },
+{ x86_64, i686 },
+{ x86_64, x86_64 },
+};
+
+static const struct hypervisor_capabilities const kqemu_capa[] = {
+{ i686, i686 },
+{ x86_64, x86_64 },
+};
+
static int
qemudCapsInitGuest(virCapsPtr caps,
const char *hostmachine,
@@ -358,25 +374,33 @@
NULL) == NULL)
return -1;
-/* If guest host match, then we can accelerate */
-if (STREQ(info-arch, hostmachine)) {
-if (access(/dev/kqemu, F_OK) == 0
-virCapabilitiesAddGuestDomain(guest,
- kqemu,
- NULL,
- NULL,
- 0,
- NULL) == NULL)
-return -1;
+/* If guest / host combo is listed as supported, we can accelerate */
+for (i = 0; i sizeof(kqemu_capa)/sizeof(kqemu_capa[0]); i++) {
+if (STREQ(hostmachine, kqemu_capa[i].hostarch)
+STREQ(info-arch, kqemu_capa[i].guestarch)) {
+if (access(/dev/kqemu, F_OK) == 0
+virCapabilitiesAddGuestDomain(guest,
+ kqemu,
+ NULL,
+ NULL,
+ 0,
+ NULL) == NULL)
+return -1;
+}
+}
-if (access(/dev/kvm, F_OK) == 0
-virCapabilitiesAddGuestDomain(guest,
- kvm,
- /usr/bin/qemu-kvm,
- NULL,
- 0,
- NULL) == NULL)
-return -1;
+for (i = 0; i sizeof(kvm_capa)/sizeof(kvm_capa[0]); i++) {
+if (STREQ(hostmachine, kvm_capa[i].hostarch)
+STREQ(info-arch, kvm_capa[i].guestarch)) {
+if (access(/dev/kvm, F_OK) == 0
+virCapabilitiesAddGuestDomain(guest,
+ kvm,
+ /usr/bin/qemu-kvm,
+ NULL,
+ 0,
+ NULL) == NULL)
+return -1;
+}
}
} else {
if (virCapabilitiesAddGuestDomain(guest,
Only in libvirt-0.4.2/src: qemu_conf.c~
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Support for passing arbitrary qemu/kvm admin console commands?
I'm looking at building a framework which makes use of KVM's pseudo-migration support for snapshot management. To my knowledge, this functionality is not presently available through libvirt. Is it possible to send arbitrary qemu/kvm admin console commands to a VM started and controlled by libvirt, or does any requirement for funtionality not presently exposed through libvirt conflict with its use? Thanks! -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
