Re: [libvirt] [PATCH] util: fix crash when starting macvtap interfaces
On 04/26/2012 04:46 AM, Eric Blake wrote: On 04/25/2012 02:01 PM, Laine Stump wrote: This patch resolves https://bugzilla.redhat.com/show_bug.cgi?id=815270 The function virNetDevMacVLanVPortProfileRegisterCallback() takes an arg virtPortProfile, and was checking it for non-NULL before using it. However, the prototype for virNetDevMacVLanPortProfileRegisterCallback had marked that arg with ATTRIBUTE_NONNULL(). Contrary to what one may think, ATTRIBUTE_NONNULL() does not provide any guarantee that an arg marked as such really is always non-null; the only effect to the code generated by gcc, is that gcc *assumes* it is non-NULL; this results in, for example, the check for a non-NULL value being optimized out. (Unfortunately, this code removal only occurs when optimization is enabled, and I am in the habit of doing local builds with optimization off to ease debugging, so the bug didn't show up in my earlier local testing). In general, virPortProfile might always be NULL, so it shouldn't be marked as ATTRIBUTE_NONNULL. One other function prototype made this same error, so this patch fixes it as well. Might be worth linking to http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17308 --- src/util/virnetdevmacvlan.h |4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) ACK. What an insidious bug. As Laine and I discussed on IRC, I'm half wondering if we should just do: #ifdef STATIC_ANALYSIS /* attributes supported */ # define ATTRIBUTE_NONNULL(n) __attribute__((__nonnull__(n))) #else # define ATTRIBUTE_NONNULL(n) /* empty, due to gcc lameness */ #endif so that our code will be pessimized under normal compiles, but _at least_ places where we have bugs with improper use of the attribute won't cause gcc to miscompile things; but still let us get NULL checking when running clang or Coverity. I also wonder if this has been detected by Coverity (checking a nonnull parameter for NULL is dead code, which Coverity does tend to flag), and we just haven't been following Coverity closely enough to notice. Eric, I ran Coverity on current commit 'f78024b util: fix crash when starting macvtap interfaces', Coverity hasn't complain this issue, although I also enabled '--security' checkers in Coverity. -- 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] util: fix crash when starting macvtap interfaces
On 04/25/2012 04:46 PM, Eric Blake wrote: On 04/25/2012 02:01 PM, Laine Stump wrote: This patch resolves https://bugzilla.redhat.com/show_bug.cgi?id=815270 The function virNetDevMacVLanVPortProfileRegisterCallback() takes an arg virtPortProfile, and was checking it for non-NULL before using it. However, the prototype for virNetDevMacVLanPortProfileRegisterCallback had marked that arg with ATTRIBUTE_NONNULL(). Contrary to what one may think, ATTRIBUTE_NONNULL() does not provide any guarantee that an arg marked as such really is always non-null; the only effect to the code generated by gcc, is that gcc *assumes* it is non-NULL; this results in, for example, the check for a non-NULL value being optimized out. (Unfortunately, this code removal only occurs when optimization is enabled, and I am in the habit of doing local builds with optimization off to ease debugging, so the bug didn't show up in my earlier local testing). In general, virPortProfile might always be NULL, so it shouldn't be marked as ATTRIBUTE_NONNULL. One other function prototype made this same error, so this patch fixes it as well. Might be worth linking to http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17308 Oops. I pushed before I noticed this comment. --- src/util/virnetdevmacvlan.h |4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) ACK. What an insidious bug. As Laine and I discussed on IRC, I'm half wondering if we should just do: #ifdef STATIC_ANALYSIS /* attributes supported */ # define ATTRIBUTE_NONNULL(n) __attribute__((__nonnull__(n))) #else # define ATTRIBUTE_NONNULL(n) /* empty, due to gcc lameness */ #endif so that our code will be pessimized under normal compiles, but _at least_ places where we have bugs with improper use of the attribute won't cause gcc to miscompile things; but still let us get NULL checking when running clang or Coverity. Or the patch that will be in the next reply to your mail? (STATIC_ANALYSIS is always defined, but could be 0 or 1) I also wonder if this has been detected by Coverity (checking a nonnull parameter for NULL is dead code, which Coverity does tend to flag), and we just haven't been following Coverity closely enough to notice. It's a fairly recent change, so very likely nobody has run Coverity against it yet. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: fix crash when starting macvtap interfaces
On 04/26/2012 02:12 AM, Alex Jia wrote: On 04/26/2012 04:46 AM, Eric Blake wrote: On 04/25/2012 02:01 PM, Laine Stump wrote: This patch resolves https://bugzilla.redhat.com/show_bug.cgi?id=815270 The function virNetDevMacVLanVPortProfileRegisterCallback() takes an arg virtPortProfile, and was checking it for non-NULL before using it. However, the prototype for virNetDevMacVLanPortProfileRegisterCallback had marked that arg with ATTRIBUTE_NONNULL(). Contrary to what one may think, ATTRIBUTE_NONNULL() does not provide any guarantee that an arg marked as such really is always non-null; the only effect to the code generated by gcc, is that gcc *assumes* it is non-NULL; this results in, for example, the check for a non-NULL value being optimized out. (Unfortunately, this code removal only occurs when optimization is enabled, and I am in the habit of doing local builds with optimization off to ease debugging, so the bug didn't show up in my earlier local testing). In general, virPortProfile might always be NULL, so it shouldn't be marked as ATTRIBUTE_NONNULL. One other function prototype made this same error, so this patch fixes it as well. Might be worth linking to http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17308 --- src/util/virnetdevmacvlan.h |4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) ACK. What an insidious bug. As Laine and I discussed on IRC, I'm half wondering if we should just do: #ifdef STATIC_ANALYSIS /* attributes supported */ # define ATTRIBUTE_NONNULL(n) __attribute__((__nonnull__(n))) #else # define ATTRIBUTE_NONNULL(n) /* empty, due to gcc lameness */ #endif so that our code will be pessimized under normal compiles, but _at least_ places where we have bugs with improper use of the attribute won't cause gcc to miscompile things; but still let us get NULL checking when running clang or Coverity. I also wonder if this has been detected by Coverity (checking a nonnull parameter for NULL is dead code, which Coverity does tend to flag), and we just haven't been following Coverity closely enough to notice. Eric, I ran Coverity on current commit 'f78024b util: fix crash when starting macvtap interfaces', Coverity hasn't complain this issue, although I also enabled '--security' checkers in Coverity. The interesting run would be *before* this commit. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] build: make ATTRIBUTE_NONNULL() a NOP unless STATIC_ANALYSIS is on
The ATTRIBUTE_NONNULL(m) macro normally resolves to the gcc builtin __attribute__((__nonnull__(m))). The effect of this in gcc is unfortunately only to make gcc believe that m can never possibly be NULL, *not* to add in any checks to guarantee that it isn't ever NULL (i.e. it is an optimization aid, *not* something to verify code correctness.) - see the following gcc bug report for more details: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17308 Static source analyzers such as clang and coverity apparently can use ATTRIBUTE_NONNULL(), though, to detect dead code (in the case that the arg really is guaranteed non-NULL), as well as situations where an obviously NULL arg is given to the function. https://bugzilla.redhat.com/show_bug.cgi?id=815270 is a good example of a bug caused by erroneous application of ATTRIBUTE_NONNULL(). Several people spent a long time staring at this code and not finding the problem, because the problem wasn't in the function itself, but in the prototype that specified ATTRIBUTE_NONNULL() for an arg that actually *wasn't* always non-NULL, and caused a segv when dereferenced (even though the code that dereferenced the pointer was inside an if() that checked for a NULL pointer, that code was optimized out by gcc). There may be some very small gain to be had from the optimizations that can be inferred from ATTRIBUTE_NONNULL(), but it seems safer to err on the side of generating code that behaves as expected, while turning on the attribute for static analyzers. (dissenting opinions welcome :-) --- src/internal.h |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/internal.h b/src/internal.h index ef81cda..83f468d 100644 --- a/src/internal.h +++ b/src/internal.h @@ -182,7 +182,7 @@ # endif # ifndef ATTRIBUTE_NONNULL -# if __GNUC_PREREQ (3, 3) +# if __GNUC_PREREQ (3, 3) STATIC_ANALYSIS #define ATTRIBUTE_NONNULL(m) __attribute__((__nonnull__(m))) # else #define ATTRIBUTE_NONNULL(m) -- 1.7.10 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] refactor Libvirt.Domain.get_cpu_stats
Remove parameter nr_pcpus. Add another parameter total of type bool
to indicate wheter to get total cpu statistic or per_cpu statistics.
---
examples/get_cpu_stats.ml | 50 ++--
libvirt/libvirt.ml |2 +-
libvirt/libvirt.mli |2 +-
libvirt/libvirt_c_oneoffs.c | 89 +-
4 files changed, 117 insertions(+), 26 deletions(-)
diff --git a/examples/get_cpu_stats.ml b/examples/get_cpu_stats.ml
index 3541720..6355071 100644
--- a/examples/get_cpu_stats.ml
+++ b/examples/get_cpu_stats.ml
@@ -18,32 +18,40 @@ let () =
let domname = Sys.argv.(1) in
let conn = C.connect_readonly () in
-
-let nr_pcpus =
- let info = C.get_node_info conn in
- C.maxcpus_of_node_info info in
-
-let stats =
- let dom = D.lookup_by_name conn domname in
- D.get_cpu_stats dom nr_pcpus 0 in
+let dom = D.lookup_by_name conn domname in
+
+let stats = D.get_cpu_stats dom false 0 in
+let total_stats = D.get_cpu_stats dom true 0 in
+
+let print_params n params =
+ List.iter (
+fun (name, value) -
+ printf %s= name;
+ match value with
+ | D.TypedFieldInt32 i - printf %ld i
+ | D.TypedFieldUInt32 i - printf %ld i
+ | D.TypedFieldInt64 i - printf %Ld i
+ | D.TypedFieldUInt64 i - printf %Ld i
+ | D.TypedFieldFloat f - printf %g f
+ | D.TypedFieldBool b - printf %b b
+ | D.TypedFieldString s - printf %S s
+ ) params in
Array.iteri (
fun n params -
printf pCPU %d: n;
-List.iter (
- fun (name, value) -
-printf %s= name;
-match value with
-| D.TypedFieldInt32 i - printf %ld i
-| D.TypedFieldUInt32 i - printf %ld i
-| D.TypedFieldInt64 i - printf %Ld i
-| D.TypedFieldUInt64 i - printf %Ld i
-| D.TypedFieldFloat f - printf %g f
-| D.TypedFieldBool b - printf %b b
-| D.TypedFieldString s - printf %S s
-) params;
+print_params n params;
+printf \n
+) stats;
+
+Array.iteri (
+ fun n params -
+printf total:;
+print_params n params;
printf \n
-) stats
+) total_stats
+
+
with
Libvirt.Virterror err -
eprintf error: %s\n (Libvirt.Virterror.to_string err)
diff --git a/libvirt/libvirt.ml b/libvirt/libvirt.ml
index 7a32071..102aec7 100644
--- a/libvirt/libvirt.ml
+++ b/libvirt/libvirt.ml
@@ -417,7 +417,7 @@ struct
external set_vcpus : [`W] t - int - unit =
ocaml_libvirt_domain_set_vcpus
external pin_vcpu : [`W] t - int - string - unit =
ocaml_libvirt_domain_pin_vcpu
external get_vcpus : [`R] t - int - int - int * vcpu_info array * string
= ocaml_libvirt_domain_get_vcpus
- external get_cpu_stats : [`R] t - int - int - typed_param list array =
ocaml_libvirt_domain_get_cpu_stats
+ external get_cpu_stats : [`R] t - bool - int - typed_param list array =
ocaml_libvirt_domain_get_cpu_stats
external get_max_vcpus : [`R] t - int =
ocaml_libvirt_domain_get_max_vcpus
external attach_device : [`W] t - xml - unit =
ocaml_libvirt_domain_attach_device
external detach_device : [`W] t - xml - unit =
ocaml_libvirt_domain_detach_device
diff --git a/libvirt/libvirt.mli b/libvirt/libvirt.mli
index 9782406..17892a1 100644
--- a/libvirt/libvirt.mli
+++ b/libvirt/libvirt.mli
@@ -559,7 +559,7 @@ sig
for a domain. See the libvirt documentation for details
of the array and bitmap returned from this function.
*)
- val get_cpu_stats : [`R] t - int - int - typed_param list array
+ val get_cpu_stats : [`R] t - bool - int - typed_param list array
(** [get_pcpu_stats dom nr_pcpu] returns the physical CPU stats
for a domain. See the libvirt documentation for details.
*)
diff --git a/libvirt/libvirt_c_oneoffs.c b/libvirt/libvirt_c_oneoffs.c
index 135b934..12ad634 100644
--- a/libvirt/libvirt_c_oneoffs.c
+++ b/libvirt/libvirt_c_oneoffs.c
@@ -532,18 +532,26 @@ extern int virDomainGetCPUStats (virDomainPtr domain,
#endif
CAMLprim value
-ocaml_libvirt_domain_get_cpu_stats (value domv, value nr_pcpusv, value flagsv)
+ocaml_libvirt_domain_get_cpu_stats (value domv, value totalv, value flagsv)
{
#ifdef HAVE_VIRDOMAINGETCPUSTATS
- CAMLparam3 (domv, nr_pcpusv, flagsv);
+ CAMLparam3 (domv, totalv, flagsv);
CAMLlocal5 (cpustats, param_head, param_node, typed_param,
typed_param_value);
CAMLlocal1 (v);
virDomainPtr dom = Domain_val (domv);
virConnectPtr conn = Connect_domv (domv);
- int nr_pcpus = Int_val (nr_pcpusv);
+ int get_total = totalv == Val_true ? 1 : 0;
int flags = Int_val (flagsv);
virTypedParameterPtr params;
int r, cpu, ncpus, nparams, i, j, pos;
+ int nr_pcpus;
+
+ if (get_total)
+goto do_total;
+
+ /* get number of pcpus */
+ NONBLOCKING (nr_pcpus = virDomainGetCPUStats(dom, NULL, 0, 0, 0, flags));
+ CHECK_ERROR
[libvirt] [PATCH 1/2] add parameter flags to D.get_cpu_stats()
---
examples/get_cpu_stats.ml |2 +-
libvirt/libvirt.ml |2 +-
libvirt/libvirt.mli |2 +-
libvirt/libvirt_c_oneoffs.c |9 +
4 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/examples/get_cpu_stats.ml b/examples/get_cpu_stats.ml
index 79d5c3c..3541720 100644
--- a/examples/get_cpu_stats.ml
+++ b/examples/get_cpu_stats.ml
@@ -25,7 +25,7 @@ let () =
let stats =
let dom = D.lookup_by_name conn domname in
- D.get_cpu_stats dom nr_pcpus in
+ D.get_cpu_stats dom nr_pcpus 0 in
Array.iteri (
fun n params -
diff --git a/libvirt/libvirt.ml b/libvirt/libvirt.ml
index 53c5bb4..7a32071 100644
--- a/libvirt/libvirt.ml
+++ b/libvirt/libvirt.ml
@@ -417,7 +417,7 @@ struct
external set_vcpus : [`W] t - int - unit =
ocaml_libvirt_domain_set_vcpus
external pin_vcpu : [`W] t - int - string - unit =
ocaml_libvirt_domain_pin_vcpu
external get_vcpus : [`R] t - int - int - int * vcpu_info array * string
= ocaml_libvirt_domain_get_vcpus
- external get_cpu_stats : [`R] t - int - typed_param list array =
ocaml_libvirt_domain_get_cpu_stats
+ external get_cpu_stats : [`R] t - int - int - typed_param list array =
ocaml_libvirt_domain_get_cpu_stats
external get_max_vcpus : [`R] t - int =
ocaml_libvirt_domain_get_max_vcpus
external attach_device : [`W] t - xml - unit =
ocaml_libvirt_domain_attach_device
external detach_device : [`W] t - xml - unit =
ocaml_libvirt_domain_detach_device
diff --git a/libvirt/libvirt.mli b/libvirt/libvirt.mli
index 0913a63..9782406 100644
--- a/libvirt/libvirt.mli
+++ b/libvirt/libvirt.mli
@@ -559,7 +559,7 @@ sig
for a domain. See the libvirt documentation for details
of the array and bitmap returned from this function.
*)
- val get_cpu_stats : [`R] t - int - typed_param list array
+ val get_cpu_stats : [`R] t - int - int - typed_param list array
(** [get_pcpu_stats dom nr_pcpu] returns the physical CPU stats
for a domain. See the libvirt documentation for details.
*)
diff --git a/libvirt/libvirt_c_oneoffs.c b/libvirt/libvirt_c_oneoffs.c
index 3d42b73..135b934 100644
--- a/libvirt/libvirt_c_oneoffs.c
+++ b/libvirt/libvirt_c_oneoffs.c
@@ -532,20 +532,21 @@ extern int virDomainGetCPUStats (virDomainPtr domain,
#endif
CAMLprim value
-ocaml_libvirt_domain_get_cpu_stats (value domv, value nr_pcpusv)
+ocaml_libvirt_domain_get_cpu_stats (value domv, value nr_pcpusv, value flagsv)
{
#ifdef HAVE_VIRDOMAINGETCPUSTATS
- CAMLparam2 (domv, nr_pcpusv);
+ CAMLparam3 (domv, nr_pcpusv, flagsv);
CAMLlocal5 (cpustats, param_head, param_node, typed_param,
typed_param_value);
CAMLlocal1 (v);
virDomainPtr dom = Domain_val (domv);
virConnectPtr conn = Connect_domv (domv);
int nr_pcpus = Int_val (nr_pcpusv);
+ int flags = Int_val (flagsv);
virTypedParameterPtr params;
int r, cpu, ncpus, nparams, i, j, pos;
/* get percpu information */
- NONBLOCKING (nparams = virDomainGetCPUStats(dom, NULL, 0, 0, 1, 0));
+ NONBLOCKING (nparams = virDomainGetCPUStats(dom, NULL, 0, 0, 1, flags));
CHECK_ERROR (nparams 0, conn, virDomainGetCPUStats);
if ((params = malloc(sizeof(*params) * nparams * 128)) == NULL)
@@ -556,7 +557,7 @@ ocaml_libvirt_domain_get_cpu_stats (value domv, value
nr_pcpusv)
while (cpu nr_pcpus) {
ncpus = nr_pcpus - cpu 128 ? 128 : nr_pcpus - cpu;
-NONBLOCKING (r = virDomainGetCPUStats(dom, params, nparams, cpu, ncpus,
0));
+NONBLOCKING (r = virDomainGetCPUStats(dom, params, nparams, cpu, ncpus,
flags));
CHECK_ERROR (r 0, conn, virDomainGetCPUStats);
for (i = 0; i ncpus; i++) {
--
1.7.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] add parameter flags to D.get_cpu_stats()
On Thu, Apr 26, 2012 at 04:09:05PM +0800, Hu Tao wrote: --- examples/get_cpu_stats.ml |2 +- libvirt/libvirt.ml |2 +- libvirt/libvirt.mli |2 +- libvirt/libvirt_c_oneoffs.c |9 + 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/examples/get_cpu_stats.ml b/examples/get_cpu_stats.ml index 79d5c3c..3541720 100644 --- a/examples/get_cpu_stats.ml +++ b/examples/get_cpu_stats.ml @@ -25,7 +25,7 @@ let () = let stats = let dom = D.lookup_by_name conn domname in - D.get_cpu_stats dom nr_pcpus in + D.get_cpu_stats dom nr_pcpus 0 in This needs to be a list of (the OCaml equivalent of) virTypedParameterFlags. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming blog: http://rwmj.wordpress.com Fedora now supports 80 OCaml packages (the OPEN alternative to F#) http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] blockjob: fix block-stream bandwidth race
On Thu, Apr 26, 2012 at 12:04 AM, Eric Blake ebl...@redhat.com wrote: With RHEL 6.2, virDomainBlockPull(dom, dev, bandwidth, 0) has a race with non-zero bandwidth: there is a window between the block_stream and block_job_set_speed monitor commands where an unlimited amount of data was let through, defeating the point of a throttle. This race was first identified in commit a9d3495e, and libvirt was able to reduce the size of the window for that race. In the meantime, the qemu developers decided to fix things properly; per this message: https://lists.gnu.org/archive/html/qemu-devel/2012-04/msg03793.html the fix will be in qemu 1.1, and changes block-job-set-speed to use a different parameter name, as well as adding a new optional parameter to block-stream, which eliminates the race altogether. Since our documentation already mentioned that we can refuse a non-zero bandwidth for some hypervisors, I think the best solution is to do just that for RHEL 6.2 qemu, so that the race is obvious to the user. Meanwhile the code must be fixed to honor actual qemu 1.1 naming. * src/qemu/qemu_monitor.h (enum BLOCK_JOB_CMD): Drop unused value. * src/qemu/qemu_driver.c (qemuDomainBlockJobImpl): Reject bandwidth during pull with too-old qemu. * src/qemu/qemu_monitor_json.c (qemuMonitorJSONBlockJob): Support difference between RHEL 6.2 and qemu 1.1 block pull. * src/libvirt.c (virDomainBlockPull, virDomainBlockRebase): Document this. --- src/libvirt.c | 6 +++- src/qemu/qemu_driver.c | 8 -- src/qemu/qemu_monitor.h | 3 +- src/qemu/qemu_monitor_json.c | 53 +++-- 4 files changed, 40 insertions(+), 30 deletions(-) Reviewed-by: Stefan Hajnoczi stefa...@linux.vnet.ibm.com -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/4] qemu: Avoid bogus error at the end of tunnelled migration
On 04/25/2012 02:07 PM, Jiri Denemark wrote: Once qemu monitor reports migration has completed, we just closed our end of the pipe and let migration tunnel die. This generated bogus error in case we did so before the thread saw EOF on the pipe and migration was aborted even though it was in fact successful. With this patch we first wake up the tunnel thread and once it has read all data from the pipe and finished the stream we close the filedescriptor. A small additional bonus of this patch is that real errors reported inside qemuMigrationIOFunc are not overwritten by virStreamAbort any more. --- ACK. Peter -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv3] keepalive: Add ability to disable keepalive messages
On 04/25/2012 04:41 PM, Jiri Denemark wrote: On Wed, Apr 25, 2012 at 16:18:07 +0200, Peter Krempa wrote: The docs for virConnectSetKeepAlive() advertise that this function should be able to disable keepalives on negative or zero interval time. This patch removes the check that prohibited this and adds code to disable keepalives on negative/zero interval. * src/libvirt.c: virConnectSetKeepAlive(): - remove check for negative values * src/rpc/virnetclient.c * src/rpc/virnetclient.h: - add virNetClientKeepAliveStop() to disable keepalive messages * src/remote/remote_driver.c: remoteSetKeepAlive(): -add ability to disable keepalives --- ACK Jirka Thanks; pushed. Peter -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu_agent: Report error class at least
On 12.04.2012 16:37, Michal Privoznik wrote: Currently, qemu GA is not providing 'desc' field for errors like we are used to from qemu monitor. Therefore, we fall back to this general 'unknown error' string. However, GA is reporting 'class' which is not perfect, but much more helpful than generic error string. Thus we should fall back to class firstly and if even no class is presented, then we can fall back to that generic string. Before this patch: virsh # dompmsuspend --target mem f16 error: Domain f16 could not be suspended error: internal error unable to execute QEMU command 'guest-suspend-ram': unknown QEMU command error After this patch: virsh # dompmsuspend --target mem f16 error: Domain f16 could not be suspended error: internal error unable to execute QEMU command 'guest-suspend-ram': The command has not been found --- src/qemu/qemu_agent.c | 37 +++-- 1 files changed, 35 insertions(+), 2 deletions(-) Ping? -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] build: Fix version of gettext macros
[trimming cc's] On 04/25/2012 09:18 PM, Alex Jia wrote: Hello Eric, I still met this issue on latest upstream HEAD(f78024b) when compiling libvirt: Making all in po make[2]: Entering directory `/home/ajia/Workspace/libvirt/po' *** error: gettext infrastructure mismatch: using a Makefile.in.in from gettext version 0.17 but the autoconf macros are from gettext version 0.18 make[2]: *** [check-macro-version] Error 1 Per my message in commit bae13129c: If you have already built in the window where libvirt required 0.18, be aware that incremental updates may run into problems: this is because 'autopoint --force' will not downgrade m4/po.m4 back to an older version, but it must be downgraded back to 0.17 levels to work with this patch. You may either manually remove that file then rerun bootstrap, or it may prove easier to just clean up all non-git files to start from a clean slate. -- Eric Blake ebl...@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] util: fix crash when starting macvtap interfaces
On 04/26/2012 01:44 AM, Alex Jia wrote: As Laine and I discussed on IRC, I'm half wondering if we should just do: #ifdef STATIC_ANALYSIS /* attributes supported */ # define ATTRIBUTE_NONNULL(n) __attribute__((__nonnull__(n))) #else # define ATTRIBUTE_NONNULL(n) /* empty, due to gcc lameness */ #endif so that our code will be pessimized under normal compiles, but _at least_ places where we have bugs with improper use of the attribute won't cause gcc to miscompile things; but still let us get NULL checking when running clang or Coverity. I also wonder if this has been detected by Coverity (checking a nonnull parameter for NULL is dead code, which Coverity does tend to flag), and we just haven't been following Coverity closely enough to notice. Eric, I ran Coverity on current commit 'f78024b util: fix crash when starting macvtap interfaces', Coverity hasn't complain this issue, although I also enabled '--security' checkers in Coverity. The interesting run would be *before* this commit. I reran coverity on commit 'bae1312 build: fix bootstrap on RHEL', however, Coverity hasn't also complain the issue, for details, please refer to attachment. Hmm - maybe it's worth a bug report to the Coverity folks, as that would be a very nice static check to add. How does clang fare? Meanwhile, it looks like we've got a lot of cleanup to do - there are some real bugs in that Coverity report. -- Eric Blake ebl...@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] build: make ATTRIBUTE_NONNULL() a NOP unless STATIC_ANALYSIS is on
On 04/26/2012 12:56 AM, Laine Stump wrote: The ATTRIBUTE_NONNULL(m) macro normally resolves to the gcc builtin __attribute__((__nonnull__(m))). The effect of this in gcc is unfortunately only to make gcc believe that m can never possibly be NULL, *not* to add in any checks to guarantee that it isn't ever NULL (i.e. it is an optimization aid, *not* something to verify code correctness.) - see the following gcc bug report for more details: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17308 Static source analyzers such as clang and coverity apparently can use ATTRIBUTE_NONNULL(), though, to detect dead code (in the case that the arg really is guaranteed non-NULL), as well as situations where an obviously NULL arg is given to the function. https://bugzilla.redhat.com/show_bug.cgi?id=815270 is a good example of a bug caused by erroneous application of ATTRIBUTE_NONNULL(). Several people spent a long time staring at this code and not finding the problem, because the problem wasn't in the function itself, but in the prototype that specified ATTRIBUTE_NONNULL() for an arg that actually *wasn't* always non-NULL, and caused a segv when dereferenced (even though the code that dereferenced the pointer was inside an if() that checked for a NULL pointer, that code was optimized out by gcc). There may be some very small gain to be had from the optimizations that can be inferred from ATTRIBUTE_NONNULL(), but it seems safer to err on the side of generating code that behaves as expected, while turning on the attribute for static analyzers. (dissenting opinions welcome :-) None from me :) --- src/internal.h |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/internal.h b/src/internal.h index ef81cda..83f468d 100644 --- a/src/internal.h +++ b/src/internal.h @@ -182,7 +182,7 @@ # endif # ifndef ATTRIBUTE_NONNULL -# if __GNUC_PREREQ (3, 3) +# if __GNUC_PREREQ (3, 3) STATIC_ANALYSIS #define ATTRIBUTE_NONNULL(m) __attribute__((__nonnull__(m))) ACK. -- Eric Blake ebl...@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 v5 1/9] pvs: add driver skeleton
On 04/20/2012 08:01 PM, Dmitry Guryanov wrote: Add driver, which can report node info only. changes: * add me to AUTHORS * fix indent in preprocessor directives in pvs_driver.h * remove unneded include * remove pvs_driver.c from po/POTFILES.in Signed-off-by: Dmitry Guryanovdgurya...@parallels.com --- AUTHORS |1 + cfg.mk |1 + configure.ac| 23 docs/drvpvs.html.in | 28 + include/libvirt/virterror.h |1 + libvirt.spec.in |7 + mingw32-libvirt.spec.in |6 + src/Makefile.am | 21 src/conf/domain_conf.c |3 +- src/conf/domain_conf.h |1 + src/driver.h|1 + src/libvirt.c | 12 ++ src/pvs/pvs_driver.c| 271 +++ src/pvs/pvs_driver.h| 51 src/util/virterror.c|3 + 15 files changed, 429 insertions(+), 1 deletions(-) create mode 100644 docs/drvpvs.html.in create mode 100644 src/pvs/pvs_driver.c create mode 100644 src/pvs/pvs_driver.h diff --git a/AUTHORS b/AUTHORS index eee4998..f9c1da2 100644 --- a/AUTHORS +++ b/AUTHORS @@ -232,6 +232,7 @@ Patches have also been contributed by: Stefan Baderstefan.ba...@canonical.com MATSUDA Daikimatsuda...@intellilink.co.jp Jan Kiszkajan.kis...@siemens.com + Dmitry Guryanovdgurya...@parallels.com There is a conflict here, should I resend this patch series ? git am 0001-pvs-add-driver-skeleton.patch Applying: pvs: add driver skeleton error: patch failed: AUTHORS:232 error: AUTHORS: patch does not apply Patch failed at 0001 pvs: add driver skeleton When you have resolved this problem run git am --resolved. If you would prefer to skip this patch, instead run git am --skip. To restore the original branch and stop patching run git am --abort. [send patches to get your name here] diff --git a/cfg.mk b/cfg.mk index fb4df2f..c5d7f9b 100644 --- a/cfg.mk +++ b/cfg.mk @@ -513,6 +513,7 @@ msg_gen_function += PHYP_ERROR msg_gen_function += VIR_ERROR . -- Dmitry Guryanov -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] qemu: Use common helper when probing qemu capabilities
QEMU binary is called several times when we probe different kinds of
capabilities the binary supports. This patch introduces new common
helper so that all probes use a consistent way of invoking qemu.
---
src/qemu/qemu_capabilities.c | 59 ++---
src/qemu/qemu_capabilities.h |5 +++
src/qemu/qemu_driver.c |9 +-
3 files changed, 50 insertions(+), 23 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 3d1fb43..6e5165b 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -289,6 +289,7 @@ qemuCapsParseMachineTypesStr(const char *output,
int
qemuCapsProbeMachineTypes(const char *binary,
+ virBitmapPtr qemuCaps,
virCapsGuestMachinePtr **machines,
int *nmachines)
{
@@ -306,10 +307,9 @@ qemuCapsProbeMachineTypes(const char *binary,
return -1;
}
-cmd = virCommandNewArgList(binary, -M, ?, NULL);
-virCommandAddEnvPassCommon(cmd);
+cmd = qemuCapsProbeCommand(binary, qemuCaps);
+virCommandAddArgList(cmd, -M, ?, NULL);
virCommandSetOutputBuffer(cmd, output);
-virCommandClearCaps(cmd);
/* Ignore failure from older qemu that did not understand '-M ?'. */
if (virCommandRun(cmd, status) 0)
@@ -599,12 +599,9 @@ qemuCapsProbeCPUModels(const char *qemu,
return 0;
}
-cmd = virCommandNewArgList(qemu, -cpu, ?, NULL);
-if (qemuCapsGet(qemuCaps, QEMU_CAPS_NODEFCONFIG))
-virCommandAddArg(cmd, -nodefconfig);
-virCommandAddEnvPassCommon(cmd);
+cmd = qemuCapsProbeCommand(qemu, qemuCaps);
+virCommandAddArgList(cmd, -cpu, ?, NULL);
virCommandSetOutputBuffer(cmd, output);
-virCommandClearCaps(cmd);
if (virCommandRun(cmd, NULL) 0)
goto cleanup;
@@ -730,7 +727,8 @@ qemuCapsInitGuest(virCapsPtr caps,
info-wordsize, binary,
binary_mtime,
old_caps, machines, nmachines);
if (probe
-qemuCapsProbeMachineTypes(binary, machines, nmachines) 0)
+qemuCapsProbeMachineTypes(binary, qemuCaps,
+ machines, nmachines) 0)
goto error;
}
@@ -798,7 +796,8 @@ qemuCapsInitGuest(virCapsPtr caps,
kvmbin, binary_mtime,
old_caps, machines,
nmachines);
if (probe
-qemuCapsProbeMachineTypes(kvmbin, machines, nmachines)
0)
+qemuCapsProbeMachineTypes(kvmbin, qemuCaps,
+ machines, nmachines) 0)
goto error;
}
@@ -1366,17 +1365,16 @@ qemuCapsExtractDeviceStr(const char *qemu,
* understand '-device name,?', and always exits with status 1 for
* the simpler '-device ?', so this function is really only useful
* if -help includes device driver,?. */
-cmd = virCommandNewArgList(qemu,
- -device, ?,
- -device, pci-assign,?,
- -device, virtio-blk-pci,?,
- -device, virtio-net-pci,?,
- -device, scsi-disk,?,
- NULL);
-virCommandAddEnvPassCommon(cmd);
+cmd = qemuCapsProbeCommand(qemu, flags);
+virCommandAddArgList(cmd,
+ -device, ?,
+ -device, pci-assign,?,
+ -device, virtio-blk-pci,?,
+ -device, virtio-net-pci,?,
+ -device, scsi-disk,?,
+ NULL);
/* qemu -help goes to stdout, but qemu -device ? goes to stderr. */
virCommandSetErrorBuffer(cmd, output);
-virCommandClearCaps(cmd);
if (virCommandRun(cmd, NULL) 0)
goto cleanup;
@@ -1485,10 +1483,9 @@ int qemuCapsExtractVersionInfo(const char *qemu, const
char *arch,
return -1;
}
-cmd = virCommandNewArgList(qemu, -help, NULL);
-virCommandAddEnvPassCommon(cmd);
+cmd = qemuCapsProbeCommand(qemu, NULL);
+virCommandAddArgList(cmd, -help, NULL);
virCommandSetOutputBuffer(cmd, help);
-virCommandClearCaps(cmd);
if (virCommandRun(cmd, NULL) 0)
goto cleanup;
@@ -1628,3 +1625,21 @@ qemuCapsGet(virBitmapPtr caps,
else
return b;
}
+
+
+virCommandPtr
+qemuCapsProbeCommand(const char *qemu,
+ virBitmapPtr qemuCaps)
+{
+virCommandPtr cmd = virCommandNew(qemu);
+
+if (qemuCaps) {
+if (qemuCapsGet(qemuCaps, QEMU_CAPS_NODEFCONFIG))
+virCommandAddArg(cmd, -nodefconfig);
+}
+
+virCommandAddEnvPassCommon(cmd);
+virCommandClearCaps(cmd);
+
+return cmd;
+}
diff --git
[libvirt] [PATCH 2/2] qemu: Add support for -no-user-config
Thanks to this new option we are now able to use modern CPU models (such
as Westmere) defined in external configuration file.
---
src/qemu/qemu_capabilities.c |7 ++-
src/qemu/qemu_capabilities.h |1 +
src/qemu/qemu_command.c | 11 ++-
src/qemu/qemu_driver.c |2 +-
4 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 6e5165b..a3c87d1 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -161,6 +161,7 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST,
block-job-async,
scsi-cd,
ide-cd,
+ no-user-config,
);
struct qemu_feature_flags {
@@ -1082,6 +1083,8 @@ qemuCapsComputeCmdFlags(const char *help,
}
if (strstr(help, -nodefconfig))
qemuCapsSet(flags, QEMU_CAPS_NODEFCONFIG);
+if (strstr(help, -no-user-config))
+qemuCapsSet(flags, QEMU_CAPS_NO_USER_CONFIG);
/* The trailing ' ' is important to avoid a bogus match */
if (strstr(help, -rtc ))
qemuCapsSet(flags, QEMU_CAPS_RTC);
@@ -1634,7 +1637,9 @@ qemuCapsProbeCommand(const char *qemu,
virCommandPtr cmd = virCommandNew(qemu);
if (qemuCaps) {
-if (qemuCapsGet(qemuCaps, QEMU_CAPS_NODEFCONFIG))
+if (qemuCapsGet(qemuCaps, QEMU_CAPS_NO_USER_CONFIG))
+virCommandAddArg(cmd, -no-user-config);
+else if (qemuCapsGet(qemuCaps, QEMU_CAPS_NODEFCONFIG))
virCommandAddArg(cmd, -nodefconfig);
}
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 7a6c5a0..0e0899e 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -129,6 +129,7 @@ enum qemuCapsFlags {
QEMU_CAPS_BLOCKJOB_ASYNC = 91, /* qemu 1.1 block-job-cancel */
QEMU_CAPS_SCSI_CD= 92, /* -device scsi-cd */
QEMU_CAPS_IDE_CD = 93, /* -device ide-cd */
+QEMU_CAPS_NO_USER_CONFIG = 94, /* -no-user-config */
QEMU_CAPS_LAST, /* this must always be the last item */
};
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 45cd417..e847060 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4237,11 +4237,12 @@ qemuBuildCommandLine(virConnectPtr conn,
virCommandAddArg(cmd, -nographic);
if (qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
-if (qemuCapsGet(qemuCaps, QEMU_CAPS_NODEFCONFIG))
-virCommandAddArg(cmd,
- -nodefconfig); /* Disable global config files */
-virCommandAddArg(cmd,
- -nodefaults); /* Disable default guest devices */
+/* Disable global config files and default devices */
+if (qemuCapsGet(qemuCaps, QEMU_CAPS_NO_USER_CONFIG))
+virCommandAddArg(cmd, -no-user-config);
+else if (qemuCapsGet(qemuCaps, QEMU_CAPS_NODEFCONFIG))
+virCommandAddArg(cmd, -nodefconfig);
+virCommandAddArg(cmd, -nodefaults);
}
/* Serial graphics adapter */
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index bcc3947..0345d89 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4868,7 +4868,7 @@ qemudCanonicalizeMachineDirect(virDomainDefPtr def, char
**canonical)
int i, nmachines = 0;
/* XXX we should be checking emulator capabilities and pass them instead
- * of NULL so that -nodefconfig is properly added when
+ * of NULL so that -nodefconfig or -no-user-config is properly added when
* probing machine types. Luckily, qemu does not support specifying new
* machine types in its configuration files yet, which means passing this
* additional parameter makes no difference now.
--
1.7.8.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] qemu: Add support for -no-user-config
Eduardo submitted patches[1] for qemu implementing -no-user-config as a better alternative to all-or-nothing -nodefconfig. With this new option, we are finally able to use modern CPU models defined in qemu's configuration file without allowing user-supplied qemu configuration to mess up with qemu processes started by libvirt. The qemu patches are not committed upstream yet (and this patchset won't be committed either until qemu support is finished) but they are very close and are expected to make it into qemu-1.1. [1] https://www.redhat.com/archives/libvir-list/2012-April/msg01293.html Jiri Denemark (2): qemu: Use common helper when probing qemu capabilities qemu: Add support for -no-user-config src/qemu/qemu_capabilities.c | 64 +++-- src/qemu/qemu_capabilities.h |6 src/qemu/qemu_command.c | 11 --- src/qemu/qemu_driver.c |9 +- 4 files changed, 62 insertions(+), 28 deletions(-) -- 1.7.8.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/4] Fix tunnelled migration
On Wed, Apr 25, 2012 at 14:07:44 +0200, Jiri Denemark wrote: All bugs fixed by the following patches were spotted while testing tunnelled migration. However, the first three of them may also be hit in other scenarios. Jiri Denemark (4): qemu: Preserve original error during migration rpc: Discard non-blocking calls only when necessary qemu: Fix detection of failed migration qemu: Avoid bogus error at the end of tunnelled migration src/qemu/qemu_migration.c | 171 + src/rpc/virnetclient.c| 21 +++--- 2 files changed, 151 insertions(+), 41 deletions(-) Thanks for the review. I pushed this series. Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] blockjob: fix block-stream bandwidth race
On Wed, Apr 25, 2012 at 17:04:25 -0600, Eric Blake wrote:
With RHEL 6.2, virDomainBlockPull(dom, dev, bandwidth, 0) has a race
with non-zero bandwidth: there is a window between the block_stream
and block_job_set_speed monitor commands where an unlimited amount
of data was let through, defeating the point of a throttle.
This race was first identified in commit a9d3495e, and libvirt was
able to reduce the size of the window for that race. In the meantime,
the qemu developers decided to fix things properly; per this message:
https://lists.gnu.org/archive/html/qemu-devel/2012-04/msg03793.html
the fix will be in qemu 1.1, and changes block-job-set-speed to use
a different parameter name, as well as adding a new optional parameter
to block-stream, which eliminates the race altogether.
Since our documentation already mentioned that we can refuse a non-zero
bandwidth for some hypervisors, I think the best solution is to do
just that for RHEL 6.2 qemu, so that the race is obvious to the user.
Meanwhile the code must be fixed to honor actual qemu 1.1 naming.
OK, this seems reasonable, but I'm concerned about backward
compatibility. What if someone is happily using this now with a non-zero
bandwidth even though it's racy? With this change, such usage will fail.
...
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index f3cdcdd..dc02964 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -530,8 +530,7 @@ typedef enum {
BLOCK_JOB_ABORT = 0,
BLOCK_JOB_INFO = 1,
BLOCK_JOB_SPEED = 2,
-BLOCK_JOB_SPEED_INTERNAL = 3,
-BLOCK_JOB_PULL = 4,
+BLOCK_JOB_PULL = 3,
} BLOCK_JOB_CMD;
...
Why do we use explicit numbering when this is only used internally?
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] blockjob: fix block-stream bandwidth race
On 04/26/2012 09:00 AM, Jiri Denemark wrote:
On Wed, Apr 25, 2012 at 17:04:25 -0600, Eric Blake wrote:
With RHEL 6.2, virDomainBlockPull(dom, dev, bandwidth, 0) has a race
with non-zero bandwidth: there is a window between the block_stream
and block_job_set_speed monitor commands where an unlimited amount
of data was let through, defeating the point of a throttle.
This race was first identified in commit a9d3495e, and libvirt was
able to reduce the size of the window for that race. In the meantime,
the qemu developers decided to fix things properly; per this message:
https://lists.gnu.org/archive/html/qemu-devel/2012-04/msg03793.html
the fix will be in qemu 1.1, and changes block-job-set-speed to use
a different parameter name, as well as adding a new optional parameter
to block-stream, which eliminates the race altogether.
Since our documentation already mentioned that we can refuse a non-zero
bandwidth for some hypervisors, I think the best solution is to do
just that for RHEL 6.2 qemu, so that the race is obvious to the user.
Meanwhile the code must be fixed to honor actual qemu 1.1 naming.
OK, this seems reasonable, but I'm concerned about backward
compatibility. What if someone is happily using this now with a non-zero
bandwidth even though it's racy? With this change, such usage will fail.
Since there is no upstream qemu release with the race present, the
_only_ time you can get a user affected by this change in semantics is
if you are using qemu on RHEL 6.2; furthermore, that version of qemu
only supported block pull for the QED file format, which is not very
popular yet. And given that only RHEL 6.2 qemu is affected, anyone that
uses the RHEL 6.2 libvirt will not have any regression so long as this
patch is not backported to that version of libvirt. I think it is a
small enough set of people to be worth the tightened semantics.
...
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index f3cdcdd..dc02964 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -530,8 +530,7 @@ typedef enum {
BLOCK_JOB_ABORT = 0,
BLOCK_JOB_INFO = 1,
BLOCK_JOB_SPEED = 2,
-BLOCK_JOB_SPEED_INTERNAL = 3,
-BLOCK_JOB_PULL = 4,
+BLOCK_JOB_PULL = 3,
} BLOCK_JOB_CMD;
...
Why do we use explicit numbering when this is only used internally?
No particular reason; I can trim that when pushing, if you'd like.
--
Eric Blake ebl...@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/2] qemu: Use common helper when probing qemu capabilities
On 04/26/2012 08:28 AM, Jiri Denemark wrote: QEMU binary is called several times when we probe different kinds of capabilities the binary supports. This patch introduces new common helper so that all probes use a consistent way of invoking qemu. --- src/qemu/qemu_capabilities.c | 59 ++--- src/qemu/qemu_capabilities.h |5 +++ src/qemu/qemu_driver.c |9 +- 3 files changed, 50 insertions(+), 23 deletions(-) Nice. Just today, I hit a bug where I wish we were caching qemu capabilities better, and by filtering things into a common entry point, I'm hoping it makes future edits able to take advantage of that one location, rather than chasing down multiple callers. https://bugzilla.redhat.com/show_bug.cgi?id=816674 At any rate, this patch looks correct: ACK. -- Eric Blake ebl...@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 2/2] qemu: Add support for -no-user-config
On 04/26/2012 08:28 AM, Jiri Denemark wrote: Thanks to this new option we are now able to use modern CPU models (such as Westmere) defined in external configuration file. --- I agree with your decision to not push this patch until we have a documented qemu pull request incorporating the qemu side of things, to ensure that we will match qemu 1.1 semantics. src/qemu/qemu_capabilities.c |7 ++- src/qemu/qemu_capabilities.h |1 + src/qemu/qemu_command.c | 11 ++- src/qemu/qemu_driver.c |2 +- 4 files changed, 14 insertions(+), 7 deletions(-) When we _do_ get ready to push this, please add a file to tests/qemuhelpdata corresponding to qemu 1.1, so that we can prove that we properly detect the new flag bit according to -help output. ACK to the code that you do have, though. -- Eric Blake ebl...@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] util: fix crash when starting macvtap interfaces
Hello Eric, Unfortunately, Clang hasn't also complain the issue like Coverity on commit 'bae1312 build: fix bootstrap on RHEL'. Regards, Alex - Original Message - From: Eric Blake ebl...@redhat.com To: Alex Jia a...@redhat.com Cc: Laine Stump la...@laine.org, libvir-list@redhat.com Sent: Thursday, April 26, 2012 8:55:18 PM Subject: Re: [libvirt] [PATCH] util: fix crash when startingmacvtap interfaces On 04/26/2012 01:44 AM, Alex Jia wrote: As Laine and I discussed on IRC, I'm half wondering if we should just do: #ifdef STATIC_ANALYSIS /* attributes supported */ # define ATTRIBUTE_NONNULL(n) __attribute__((__nonnull__(n))) #else # define ATTRIBUTE_NONNULL(n) /* empty, due to gcc lameness */ #endif so that our code will be pessimized under normal compiles, but _at least_ places where we have bugs with improper use of the attribute won't cause gcc to miscompile things; but still let us get NULL checking when running clang or Coverity. I also wonder if this has been detected by Coverity (checking a nonnull parameter for NULL is dead code, which Coverity does tend to flag), and we just haven't been following Coverity closely enough to notice. Eric, I ran Coverity on current commit 'f78024b util: fix crash when starting macvtap interfaces', Coverity hasn't complain this issue, although I also enabled '--security' checkers in Coverity. The interesting run would be *before* this commit. I reran coverity on commit 'bae1312 build: fix bootstrap on RHEL', however, Coverity hasn't also complain the issue, for details, please refer to attachment. Hmm - maybe it's worth a bug report to the Coverity folks, as that would be a very nice static check to add. How does clang fare? Meanwhile, it looks like we've got a lot of cleanup to do - there are some real bugs in that Coverity report. -- Eric Blake ebl...@redhat.com+1-919-301-3266 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] blockjob: fix block-stream bandwidth race
On 04/26/2012 03:05 AM, Stefan Hajnoczi wrote: On Thu, Apr 26, 2012 at 12:04 AM, Eric Blake ebl...@redhat.com wrote: With RHEL 6.2, virDomainBlockPull(dom, dev, bandwidth, 0) has a race with non-zero bandwidth: there is a window between the block_stream and block_job_set_speed monitor commands where an unlimited amount of data was let through, defeating the point of a throttle. This race was first identified in commit a9d3495e, and libvirt was able to reduce the size of the window for that race. In the meantime, the qemu developers decided to fix things properly; per this message: https://lists.gnu.org/archive/html/qemu-devel/2012-04/msg03793.html the fix will be in qemu 1.1, and changes block-job-set-speed to use a different parameter name, as well as adding a new optional parameter to block-stream, which eliminates the race altogether. Since our documentation already mentioned that we can refuse a non-zero bandwidth for some hypervisors, I think the best solution is to do just that for RHEL 6.2 qemu, so that the race is obvious to the user. Meanwhile the code must be fixed to honor actual qemu 1.1 naming. * src/qemu/qemu_monitor.h (enum BLOCK_JOB_CMD): Drop unused value. * src/qemu/qemu_driver.c (qemuDomainBlockJobImpl): Reject bandwidth during pull with too-old qemu. * src/qemu/qemu_monitor_json.c (qemuMonitorJSONBlockJob): Support difference between RHEL 6.2 and qemu 1.1 block pull. * src/libvirt.c (virDomainBlockPull, virDomainBlockRebase): Document this. --- src/libvirt.c|6 +++- src/qemu/qemu_driver.c |8 -- src/qemu/qemu_monitor.h |3 +- src/qemu/qemu_monitor_json.c | 53 +++-- 4 files changed, 40 insertions(+), 30 deletions(-) Reviewed-by: Stefan Hajnoczi stefa...@linux.vnet.ibm.com Thanks. However, I found a bug: unsigned long bandwidth, ... +bandwidth *= 1024ULL * 1024ULL; /* Scale MiB to bytes */ This can overflow, and silently wrap around. v2 coming up. -- Eric Blake ebl...@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] libvirt stable releases
Hi all, An idea we've kicked around for awhile in Red Hat/Fedora land is doing official libvirt stable releases, but nothing ever took shape. The idea was brought up again recently and I've offered to help get something going. I've pushed an upstream v0.9.11-maint branch with a bunch of patches cherry-picked to libvirt 0.9.11. Shortly I'll be cutting a 0.9.11.1 and pushing it to the website, like other releases. Why 0.9.11? Because that's what we will be shipping in Fedora 17 :) Typically our policy with fedora is to stick with one libvirt version for the length of a release. Cutting stable releases should save us from having to backport patches, and hopefully get us more bug fixes than backporting only what our users report. So I don't plan on doing a similar branch for 0.9.12 or 0.9.13, but will probably do a branch in 6 months time for whatever libvirt version we are shipping in Fedora 18. While my primary motiviation at the moment is making Fedora maintenance easier, I hope that other distros will use the stable releases as well, where we can all benefit from each others QA and attention. Any feedback appreciated! Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] building error
On 04/20/2012 04:50 AM, Daniel Veillard wrote: On Fri, Apr 20, 2012 at 02:26:31PM +0800, Wen Congyang wrote: When I build libvirt, I meet the following error message sometimes: make[4]: Entering directory `/home/wency/rpmbuild/BUILD/libvirt-0.9.11/docs' GENlibvirt-api.xml GENlibvirt-qemu-api.xml GENhtml/index.html ./libvirt-api.xml:2450: parser error : AttValue: ' expected function name='virConnectDomainEventDeregister' file='libvi hum ... libvirt-api.xml is generated by docs/apibuild.py that's the first time I heard of a truncated output from that program. [...] unable to parse ./libvirt-api.xml make[4]: *** [html/index.html] Error 6 make[4]: *** Waiting for unfinished jobs If I rebuild it without anything change, the building will success. It may be a problem of running make with a default parallel seting and a bug in docs/Makefile.am where ./libvirt-api.xml would not be properly serialized with the building of html/index.html. The weird thing is that from your output they seems to be serialized but I would definitely investigate in that direction. I just checked from git head here and that seems to work fine Daniel I just encountered what looks like the same problem. At the time I was running make -j4 in the libvirt directory, and also building a Fedora rawhide kernel, so the system was loaded down fairly well: make[4]: Entering directory `/home/laine/devel/libvirt/docs' GENlibvirt-qemu-api.xml devhelp/index.html GENhtml/index.html ./libvirt-api.xml:2686: parser error : AttValue: ' expected arg name='conn' type='virConnectPtr' info='pointer to the hypervisor con ^ ./libvirt-api.xml:2686: parser error : attributes construct error arg name='conn' type='virConnectPtr' info='pointer to the hypervisor con ^ ./libvirt-api.xml:2686: parser error : Couldn't find end of Start Tag arg line 2686 arg name='conn' type='virConnectPtr' info='pointer to the hypervisor con ^ ./libvirt-api.xml:2686: parser error : Premature end of data in tag function line 2683 arg name='conn' type='virConnectPtr' info='pointer to the hypervisor con ^ ./libvirt-api.xml:2686: parser error : Premature end of data in tag symbols line 1136 arg name='conn' type='virConnectPtr' info='pointer to the hypervisor con ^ ./libvirt-api.xml:2686: parser error : Premature end of data in tag api line 2 arg name='conn' type='virConnectPtr' info='pointer to the hypervisor con ^ unable to parse ./libvirt-api.xml make[4]: *** [devhelp/index.html] Error 6 make[4]: *** Waiting for unfinished jobs ./libvirt-api.xml:3112: parser error : CData section not finished Populate a disk image with data from its backing i The ^ ./libvirt-api.xml:3112: parser error : Premature end of data in tag info line 3098 The ^ ./libvirt-api.xml:3112: parser error : Premature end of data in tag function line 3097 The ^ ./libvirt-api.xml:3112: parser error : Premature end of data in tag symbols line 1136 The ^ ./libvirt-api.xml:3112: parser error : Premature end of data in tag api line 2 The ^ unable to parse ./libvirt-api.xml make[4]: *** [html/index.html] Error 6 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] SR-IOV VLANs
How should libvirt deal with different SR-IOV VLAN scenarios? Hardware with full SR-IOV and ACS support is increasingly available and in production. I just took delivery of such a server this week. It would be nice if the software can fully take advantage of the hardware. == Scenario A - Transparent VLAN == In the first scenario a /single/ VLAN can be tied to a a VF and the tagging and stripping of the VLAN tag happens outside of the view of the VM. Since the VLAN management happens outside the VM there isn't any security concerns from a malicious VM. Today this can configured with the ip command: ip link set pf vf vfN vlan vlanID But, unless I'm reading it wrong, libvirtd will blow away any pre-configured transparent VLAN in qemuDomainHostdevNetConfigReplace == Scenario B - VM VLAN tagging == In the second scenario, tagged frames are delivered to the VM and the VM uses them like normal. There are security considerations here. You don't want a malicious VM to access arbitrary VLANs. There should be a way to have a whitelist of allowed VLAN tags that can be enforced per VF at lower layer than the VM. Unfortunately it doesn't seem like hardware today supports this granularity. The Intel x540 datasheet indicates that a VLAN filter can be programmed with up to 8 VLANs, but I'm not sure if that is per VF or a global limit. With Emulex hardware, running: ip link set pf vf vfN vlan 4095 allows the VM with the given VF to configure any VLAN. Likewise, the Intel ixgbe driver has ixgbe_set_vlan_anti_spoofing (and ixgbe_set_mac_anti_spoofing) on a per VF basis. It isn't clear to me if IFLA_VF_SPOOFCHK toggles both or just the mac anti spoofing. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2] blockjob: fix block-stream bandwidth race
With RHEL 6.2, virDomainBlockPull(dom, dev, bandwidth, 0) has a race
with non-zero bandwidth: there is a window between the block_stream
and block_job_set_speed monitor commands where an unlimited amount
of data was let through, defeating the point of a throttle.
This race was first identified in commit a9d3495e, and libvirt was
able to reduce the size of the window for that race. In the meantime,
the qemu developers decided to fix things properly; per this message:
https://lists.gnu.org/archive/html/qemu-devel/2012-04/msg03793.html
the fix will be in qemu 1.1, and changes block-job-set-speed to use
a different parameter name, as well as adding a new optional parameter
to block-stream, which eliminates the race altogether.
Since our documentation already mentioned that we can refuse a non-zero
bandwidth for some hypervisors, I think the best solution is to do
just that for RHEL 6.2 qemu, so that the race is obvious to the user
(anyone using stock RHEL 6.2 binaries won't have this patch, and anyone
building their own libvirt with this patch for RHEL can also rebuild
qemu to get the modern semantics, so it is no real loss in behavior).
Meanwhile the code must be fixed to honor actual qemu 1.1 naming.
Rename the parameter to 'modern', since the naming difference now
covers more than just 'async' block-job-cancel. And while at it,
fix an unchecked integer overflow.
* src/qemu/qemu_monitor.h (enum BLOCK_JOB_CMD): Drop unused value,
rename enum to match conventions.
* src/qemu/qemu_monitor.c (qemuMonitorBlockJob): Reflect enum rename.
* src/qemu_qemu_monitor_json.h (qemuMonitorJSONBlockJob): Likewise.
* src/qemu/qemu_monitor_json.c (qemuMonitorJSONBlockJob): Likewise,
and support difference between RHEL 6.2 and qemu 1.1 block pull.
* src/qemu/qemu_driver.c (qemuDomainBlockJobImpl): Reject
bandwidth during pull with too-old qemu.
* src/libvirt.c (virDomainBlockPull, virDomainBlockRebase):
Document this.
---
v2: fix integer overflow, improve variable naming
src/libvirt.c|8 -
src/qemu/qemu_driver.c |8 +++--
src/qemu/qemu_monitor.c | 25 +-
src/qemu/qemu_monitor.h | 15 +-
src/qemu/qemu_monitor_json.c | 59 +++--
src/qemu/qemu_monitor_json.h |6 ++--
6 files changed, 72 insertions(+), 49 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c
index b01ebba..cfd7711 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -18145,7 +18145,9 @@ error:
* The maximum bandwidth (in Mbps) that will be used to do the copy can be
* specified with the bandwidth parameter. If set to 0, libvirt will choose a
* suitable default. Some hypervisors do not support this feature and will
- * return an error if bandwidth is not 0.
+ * return an error if bandwidth is not 0; in this case, it might still be
+ * possible for a later call to virDomainBlockJobSetSpeed() to succeed.
+ * The actual speed can be determined with virDomainGetBlockJobInfo().
*
* This is shorthand for virDomainBlockRebase() with a NULL base.
*
@@ -18263,7 +18265,9 @@ error:
* The maximum bandwidth (in Mbps) that will be used to do the copy can be
* specified with the bandwidth parameter. If set to 0, libvirt will choose a
* suitable default. Some hypervisors do not support this feature and will
- * return an error if bandwidth is not 0.
+ * return an error if bandwidth is not 0; in this case, it might still be
+ * possible for a later call to virDomainBlockJobSetSpeed() to succeed.
+ * The actual speed can be determined with virDomainGetBlockJobInfo().
*
* When @base is NULL and @flags is 0, this is identical to
* virDomainBlockPull().
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c3555ca..d3aa34d 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11654,6 +11654,11 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char
*path, const char *base,
_(partial block pull not supported with this
QEMU binary));
goto cleanup;
+} else if (mode == BLOCK_JOB_PULL bandwidth) {
+qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
+_(setting bandwidth at start of block pull not
+ supported with this QEMU binary));
+goto cleanup;
}
device = qemuDiskPathToAlias(vm, path, idx);
@@ -11676,9 +11681,6 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char
*path, const char *base,
* relying on qemu to do this. */
ret = qemuMonitorBlockJob(priv-mon, device, base, bandwidth, info, mode,
async);
-if (ret == 0 mode == BLOCK_JOB_PULL bandwidth)
-ret = qemuMonitorBlockJob(priv-mon, device, NULL, bandwidth, NULL,
- BLOCK_JOB_SPEED_INTERNAL, async);
qemuDomainObjExitMonitorWithDriver(driver, vm);
if (ret 0)
goto endjob;
diff --git a/src/qemu/qemu_monitor.c
[libvirt] [PATCH] util: fix error messages in virNetlinkEventServiceStart
Some of the error messages in this function should have been
virReportSystemError (since they have an errno they want to log), but
were mistakenly written as netlinkError, which expects a libvirt error
code instead. The result was that when one of the errors was
encountered, No error message provided would be printed instead of
something meaningful (see
https://bugzilla.redhat.com/show_bug.cgi?id=816465 for an example).
---
src/util/virnetlink.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
index 7017275..b2e9d51 100644
--- a/src/util/virnetlink.c
+++ b/src/util/virnetlink.c
@@ -238,8 +238,8 @@ virNetlinkEventCallback(int watch,
if (length == 0)
return;
if (length 0) {
-netlinkError(errno,
- %s, _(nl_recv returned with error));
+virReportSystemError(errno,
+ %s, _(nl_recv returned with error));
return;
}
@@ -349,28 +349,28 @@ virNetlinkEventServiceStart(void)
srv-netlinknh = nl_handle_alloc();
if (!srv-netlinknh) {
-netlinkError(errno,
-%s, _(cannot allocate nlhandle for virNetlinkEvent
server));
+virReportSystemError(errno,
+ %s, _(cannot allocate nlhandle for
virNetlinkEvent server));
goto error_locked;
}
if (nl_connect(srv-netlinknh, NETLINK_ROUTE) 0) {
-netlinkError(errno,
-%s, _(cannot connect to netlink socket));
+virReportSystemError(errno,
+ %s, _(cannot connect to netlink socket));
goto error_server;
}
fd = nl_socket_get_fd(srv-netlinknh);
if (fd 0) {
-netlinkError(errno,
- %s, _(cannot get netlink socket fd));
+virReportSystemError(errno,
+ %s, _(cannot get netlink socket fd));
goto error_server;
}
if (nl_socket_set_nonblocking(srv-netlinknh)) {
-netlinkError(errno, %s,
- _(cannot set netlink socket nonblocking));
+virReportSystemError(errno, %s,
+ _(cannot set netlink socket nonblocking));
goto error_server;
}
--
1.7.10
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] libvirt stable releases
On 04/26/2012 12:39 PM, Cole Robinson wrote: Hi all, An idea we've kicked around for awhile in Red Hat/Fedora land is doing official libvirt stable releases, but nothing ever took shape. The idea was brought up again recently and I've offered to help get something going. I've pushed an upstream v0.9.11-maint branch with a bunch of patches cherry-picked to libvirt 0.9.11. Shortly I'll be cutting a 0.9.11.1 and pushing it to the website, like other releases. How often do you plan to cut releases on the current maint branch? Once a month or so? What's the preferred method for marking a patch as a candidate for inclusion on the branch? Right now, it looks like we are using cherry-pick -x to populate the branch; maybe someday it would be worth swapping over to the style used by the kernel where you base candidate patches directly off the stable branch, then merge the branch into master for development, so that master is always a superset of all commits in stable; but that implies using a merge paradigm whereas our current style is that mainline is linear history. Food for thought, but certainly not anything worth changing right away until we have more experience with how popular the stable branch turns out to be. Why 0.9.11? Because that's what we will be shipping in Fedora 17 :) Typically our policy with fedora is to stick with one libvirt version for the length of a release. Cutting stable releases should save us from having to backport patches, and hopefully get us more bug fixes than backporting only what our users report. So I don't plan on doing a similar branch for 0.9.12 or 0.9.13, but will probably do a branch in 6 months time for whatever libvirt version we are shipping in Fedora 18. While my primary motiviation at the moment is making Fedora maintenance easier, I hope that other distros will use the stable releases as well, where we can all benefit from each others QA and attention. Any feedback appreciated! Seems like a nice idea; it will be interesting to see who else picks up on it. -- Eric Blake ebl...@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] util: fix error messages in virNetlinkEventServiceStart
On 04/26/2012 01:09 PM, Laine Stump wrote: Some of the error messages in this function should have been virReportSystemError (since they have an errno they want to log), but were mistakenly written as netlinkError, which expects a libvirt error code instead. The result was that when one of the errors was encountered, No error message provided would be printed instead of something meaningful (see https://bugzilla.redhat.com/show_bug.cgi?id=816465 for an example). --- src/util/virnetlink.c | 20 ++-- 1 file changed, 10 insertions(+), 10 deletions(-) ACK. -- Eric Blake ebl...@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 1/2] nwfilter: address coverity findings
This patch addresses the following coverity findings:
/libvirt/src/conf/nwfilter_params.c:157:
deref_parm: Directly dereferencing parameter val.
/libvirt/src/conf/nwfilter_params.c:473:
negative_returns: Using variable iterIndex as an index to array
res-iter.
/libvirt/src/nwfilter/nwfilter_ebiptables_driver.c:2891:
unchecked_value: No check of the return value of virAsprintf(protostr,
-d 01:80:c2:00:00:00 ).
/libvirt/src/nwfilter/nwfilter_ebiptables_driver.c:2894:
unchecked_value: No check of the return value of virAsprintf(protostr,
-p 0x%04x , l3_protocols[protoidx].attr).
/libvirt/src/nwfilter/nwfilter_ebiptables_driver.c:3590:
var_deref_op: Dereferencing null variable inst.
---
src/conf/nwfilter_params.c|5 -
src/nwfilter/nwfilter_ebiptables_driver.c | 10 +++---
2 files changed, 11 insertions(+), 4 deletions(-)
Index: libvirt-acl/src/conf/nwfilter_params.c
===
--- libvirt-acl.orig/src/conf/nwfilter_params.c
+++ libvirt-acl/src/conf/nwfilter_params.c
@@ -154,6 +154,9 @@ virNWFilterVarValueGetNthValue(virNWFilt
{
const char *res = NULL;
+if (!val)
+return NULL;
+
switch (val-valType) {
case NWFILTER_VALUE_TYPE_SIMPLE:
if (idx == 0)
@@ -467,7 +470,7 @@ virNWFilterVarCombIterCreate(virNWFilter
res-nIter++;
break;
case VIR_NWFILTER_VAR_ACCESS_LAST:
-break;
+goto err_exit;
}
if (virNWFilterVarCombIterAddVariable(res-iter[iterIndex],
Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c
===
--- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.c
+++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c
@@ -2878,6 +2878,7 @@ ebtablesCreateTmpSubChain(ebiptablesRule
char chainPrefix = (incoming) ? CHAINPREFIX_HOST_IN_TEMP
: CHAINPREFIX_HOST_OUT_TEMP;
char *protostr = NULL;
+int r = 0;
PRINT_ROOT_CHAIN(rootchain, chainPrefix, ifname);
PRINT_CHAIN(chain, chainPrefix, ifname,
@@ -2888,14 +2889,14 @@ ebtablesCreateTmpSubChain(ebiptablesRule
protostr = strdup();
break;
case L2_PROTO_STP_IDX:
-virAsprintf(protostr, -d NWFILTER_MAC_BGA );
+r = virAsprintf(protostr, -d NWFILTER_MAC_BGA );
break;
default:
-virAsprintf(protostr, -p 0x%04x , l3_protocols[protoidx].attr);
+r = virAsprintf(protostr, -p 0x%04x ,
l3_protocols[protoidx].attr);
break;
}
-if (!protostr) {
+if (!protostr || r 0) {
virReportOOMError();
return -1;
}
@@ -3589,6 +3590,9 @@ ebiptablesApplyNewRules(const char *ifna
int nEbtChains = 0;
char *errmsg = NULL;
+if (inst == NULL)
+nruleInstances = 0;
+
if (!chains_in_set || !chains_out_set) {
virReportOOMError();
goto exit_free_sets;
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: fix error messages in virNetlinkEventServiceStart
On 04/26/2012 03:14 PM, Eric Blake wrote: On 04/26/2012 01:09 PM, Laine Stump wrote: Some of the error messages in this function should have been virReportSystemError (since they have an errno they want to log), but were mistakenly written as netlinkError, which expects a libvirt error code instead. The result was that when one of the errors was encountered, No error message provided would be printed instead of something meaningful (see https://bugzilla.redhat.com/show_bug.cgi?id=816465 for an example). --- src/util/virnetlink.c | 20 ++-- 1 file changed, 10 insertions(+), 10 deletions(-) ACK. Pushed, Thanks! -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] libvirt stable releases
On 04/26/2012 03:12 PM, Eric Blake wrote: On 04/26/2012 12:39 PM, Cole Robinson wrote: Hi all, An idea we've kicked around for awhile in Red Hat/Fedora land is doing official libvirt stable releases, but nothing ever took shape. The idea was brought up again recently and I've offered to help get something going. I've pushed an upstream v0.9.11-maint branch with a bunch of patches cherry-picked to libvirt 0.9.11. Shortly I'll be cutting a 0.9.11.1 and pushing it to the website, like other releases. How often do you plan to cut releases on the current maint branch? Once a month or so? For now I figured I'd just wing it. I want to at least every 2 weeks look over master commits for suitable backports, and then just judge from there. Not sure it's worth the effort to cut regular releases if no compelling bug fixes are accumulating. What's the preferred method for marking a patch as a candidate for inclusion on the branch? I figured for now I could just scrape the commits manually. If I'm missing things or being overly patch happy maybe we can figure out some process, but at this point it seems overkill. But if there's a bug fix that people think it's worth accelerating a stable release for, just CC me and comment to that effect. Right now, it looks like we are using cherry-pick -x to populate the branch; maybe someday it would be worth swapping over to the style used by the kernel where you base candidate patches directly off the stable branch, then merge the branch into master for development, so that master is always a superset of all commits in stable; but that implies using a merge paradigm whereas our current style is that mainline is linear history. Food for thought, but certainly not anything worth changing right away until we have more experience with how popular the stable branch turns out to be. Yeah one of my main goals at the start is making the process as simple as possible and have near zero impact on current libvirt git and release processes. Agreed that seeing how it turns out might necessitate more process but we'll just have to see. - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] ANNOUNCE: Stable release libvirt-0.9.11.1
I'm pleased to announce the release of libvirt-0.9.11.1. This is libvirt-0.9.11 with additional bugfixes that have accumulated upstream since the initial release. This release can be downloaded at: http://libvirt.org/sources/libvirt-0.9.11.1.tar.gz For a complete list of changes since libvirt-0.9.11 GA, please see: http://wiki.libvirt.org/page/Stable_Releases Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] libvirt stable releases
On 04/26/2012 03:12 PM, Eric Blake wrote: On 04/26/2012 12:39 PM, Cole Robinson wrote: Hi all, An idea we've kicked around for awhile in Red Hat/Fedora land is doing official libvirt stable releases, but nothing ever took shape. The idea was brought up again recently and I've offered to help get something going. I've pushed an upstream v0.9.11-maint branch with a bunch of patches cherry-picked to libvirt 0.9.11. Shortly I'll be cutting a 0.9.11.1 and pushing it to the website, like other releases. How often do you plan to cut releases on the current maint branch? Once a month or so? What's the preferred method for marking a patch as a candidate for inclusion on the branch? The Fedora-specific method we've used lately of having a bugzilla record that includes the upstream commit ID and is marked as POST seems to work well, but doesn't directly apply here, because the upstream bugzilla component doesn't have multiple releases to select from in the BZ entries, and anyway it seems like a lot of extra bookkeeping. It would of course be simplest for people to just directly cherry-pick what they wanted into the branch, but that's sure to lead to patch bloat on the stable branches. Maybe each stable release could have a -maint-candidate branch that anyone is allowed to cherry-pick into, and a designated person could periodically go through and pick from there into the -maint branch (Nah, that could possibly lead to 2 rounds of merge conflict resolution, which doesn't help anybody, and could lead to new bugs being introduced by botched merges). How about just an email to the list with a particular subject (e.g. [libvirt 0.9.11-maint pull request] commit id nn) with the body having the reason for wanting the patch pulled into the maint release; these emails would be serviced by some smaller group of people (or maybe one designated person per release). Just random ideas - I have no practical experience with any of these setups, so they may all be equally flawed :-) Right now, it looks like we are using cherry-pick -x to populate the branch; maybe someday it would be worth swapping over to the style used by the kernel where you base candidate patches directly off the stable branch, then merge the branch into master for development, so that master is always a superset of all commits in stable; but that implies using a merge paradigm whereas our current style is that mainline is linear history. Food for thought, but certainly not anything worth changing right away until we have more experience with how popular the stable branch turns out to be. I guess my problem with doing it that way (stable branch first, then merge to master) is that you can never go back after the fact and decide that something put into master really should also be added to a maintenance release. And it seems to me like the majority of fixes on branches would be of that nature. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] nwfilter: address more coverity findings
This patch addresses the following coverity findings:
/libvirt/src/conf/nwfilter_params.c:390:
var_assigned: Assigning: varValue = null return value from
virHashLookup.
/libvirt/src/conf/nwfilter_params.c:392:
dereference: Dereferencing a pointer that might be null varValue when
calling virNWFilterVarValueGetNthValue.
/libvirt/src/conf/nwfilter_params.c:399:
dereference: Dereferencing a pointer that might be null tmp when
calling virNWFilterVarValueGetNthValue.
---
src/conf/nwfilter_params.c | 16 +++-
1 file changed, 15 insertions(+), 1 deletion(-)
Index: libvirt-acl/src/conf/nwfilter_params.c
===
--- libvirt-acl.orig/src/conf/nwfilter_params.c
+++ libvirt-acl/src/conf/nwfilter_params.c
@@ -30,7 +30,7 @@
#include datatypes.h
#include nwfilter_params.h
#include domain_conf.h
-
+#include logging.h
#define VIR_FROM_THIS VIR_FROM_NWFILTER
@@ -391,14 +391,28 @@ virNWFilterVarCombIterEntryAreUniqueEntr
const char *value;
varValue = virHashLookup(hash-hashTable, cie-varNames[0]);
+if (!varValue) {
+/* caller's error */
+VIR_ERROR(_(%s: hash lookup resulted in NULL pointer), __func__);
+return true;
+}
value = virNWFilterVarValueGetNthValue(varValue, cie-curValue);
+if (!value) {
+VIR_ERROR(_(%s: Lookup of value at index %u resulted in a NULL
+ pointer), __func__, cie-curValue);
+return true;
+}
for (i = 0; i cie-curValue; i++) {
if (STREQ(value, virNWFilterVarValueGetNthValue(varValue, i))) {
bool isSame = true;
for (j = 1; j cie-nVarNames; j++) {
tmp = virHashLookup(hash-hashTable, cie-varNames[j]);
+if (!tmp) {
+/* should never occur to step on a NULL here */
+return true;
+}
if (!STREQ(virNWFilterVarValueGetNthValue(tmp,
cie-curValue),
virNWFilterVarValueGetNthValue(tmp, i))) {
isSame = false;
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: improve errors related to offline domains
https://bugzilla.redhat.com/show_bug.cgi?id=816662 pointed out
that attempting 'virsh blockpull' on an offline domain gave a
misleading error message about qemu lacking support for the
operation, even when qemu was specifically updated to support it.
The real problem is that we have several capabilities that are
only determined when starting a domain, and therefore are still
clear when first working with an inactive domain (namely, any
capability set by qemuMonitorJSONCheckCommands).
While this patch was able to hoist an existing check in one of the
three culprits, it had to add redundant checks in the other two
places (because you always have to check for an active domain after
obtaining a VM job lock, but the capability bits were being checked
prior to obtaining the job lock).
Someday it would be nice to patch libvirt to cache the set of
capabilities per qemu binary (as determined by inode and timestamp),
rather than re-probing the binary every time a domain is started,
and to teach the cache how to query the monitor during the one
time the probe is made rather than having to wait until a guest
is started; then, a capability probe would succeed even for offline
guests because it just refers to the cache, and the single check for
an active domain after grabbing the job lock would be sufficient.
But since that will involve a lot more coding, I'm happy to go
with this simpler solution for an immediate solution.
* src/qemu/qemu_driver.c (qemuDomainPMSuspendForDuration)
(qemuDomainSnapshotCreateXML, qemuDomainBlockJobImpl): Check for
offline state before checking an online-only cap.
---
src/qemu/qemu_driver.c | 24 ++--
1 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d3aa34d..3a1c1c4 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10389,6 +10389,12 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
goto cleanup;
if (flags VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) {
+if (!virDomainObjIsActive(vm)) {
+qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
+_(disk snapshots of inactive domains not
+ implemented yet));
+goto cleanup;
+}
if (virDomainSnapshotAlignDisks(def,
VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL,
false) 0)
@@ -10443,12 +10449,6 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
* makes sense, such as checking that qemu-img recognizes the
* snapshot name in at least one of the domain's disks? */
} else if (flags VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) {
-if (!virDomainObjIsActive(vm)) {
-qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
-_(disk snapshots of inactive domains not
- implemented yet));
-goto cleanup;
-}
if (qemuDomainSnapshotCreateDiskActive(domain-conn, driver,
vm, snap, flags) 0)
goto cleanup;
@@ -11642,6 +11642,12 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char
*path, const char *base,
_(no domain with matching uuid '%s'), uuidstr);
goto cleanup;
}
+if (!virDomainObjIsActive(vm)) {
+qemuReportError(VIR_ERR_OPERATION_INVALID, %s,
+_(domain is not running));
+goto endjob;
+}
+
priv = vm-privateData;
if (qemuCapsGet(priv-qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC)) {
async = true;
@@ -12637,6 +12643,12 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom,
priv = vm-privateData;
+if (!virDomainObjIsActive(vm)) {
+qemuReportError(VIR_ERR_OPERATION_INVALID,
+%s, _(domain is not running));
+goto endjob;
+}
+
if (!qemuCapsGet(priv-qemuCaps, QEMU_CAPS_WAKEUP)
(target == VIR_NODE_SUSPEND_TARGET_MEM ||
target == VIR_NODE_SUSPEND_TARGET_HYBRID)) {
--
1.7.7.6
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] [TCK] nwfilter: Adapt test program and cases to recent iptables
Anyone have an ACK or comments?
Stefan
On 04/23/2012 08:21 AM, Stefan Berger wrote:
Recent iptables fixes a lot of issues with missing spaces and
other information that was previously not reported properly. To make
the test program and test cases work on old and newer installations
of iptables tools, some adjustments need to be made.
Fix a 'file not found error' when running this tool from the shell
directly.
---
scripts/nwfilter/nwfilter2vmtest.sh|6 +++---
scripts/nwfilter/nwfilterxml2fwallout/conntrack-test.fwall | 12 ++--
2 files changed, 9 insertions(+), 9 deletions(-)
Index: libvirt-tck/scripts/nwfilter/nwfilter2vmtest.sh
===
--- libvirt-tck.orig/scripts/nwfilter/nwfilter2vmtest.sh
+++ libvirt-tck/scripts/nwfilter/nwfilter2vmtest.sh
@@ -9,7 +9,7 @@ VIRSH=virsh
# For each line starting with uri=, remove the prefix and set the hold
# space to the rest of the line. Then at file end, print the hold
# space, which is effectively the last uri= line encountered.
-uri=$(sed -n '/^uri[ ]*=[ ]*/ {
+[ -r $LIBVIRT_TCK_CONFIG ] uri=$(sed -n '/^uri[ ]*=[ ]*/ {
s///
h
}
@@ -147,12 +147,12 @@ checkExpectedOutput() {
break
fi
-diff ${tmpfile} ${tmpfile2}/dev/null
+diff -w ${tmpfile} ${tmpfile2}/dev/null
if [ $? -ne 0 ]; then
if [ $(($flags $FLAG_VERBOSE)) -ne 0 ]; then
echo FAIL ${xmlfile} : ${cmd}
-diff ${tmpfile} ${tmpfile2}
+diff -w ${tmpfile} ${tmpfile2}
fi
failctr=$(($failctr + 1))
if [ $(($flags $FLAG_WAIT)) -ne 0 ]; then
Index: libvirt-tck/scripts/nwfilter/nwfilterxml2fwallout/conntrack-test.fwall
===
--- libvirt-tck.orig/scripts/nwfilter/nwfilterxml2fwallout/conntrack-test.fwall
+++ libvirt-tck/scripts/nwfilter/nwfilterxml2fwallout/conntrack-test.fwall
@@ -1,18 +1,18 @@
-#iptables -L FI-vnet0 -n
+#iptables -L FI-vnet0 -n | sed 's|#conn/|#conn src/|'
Chain FI-vnet0 (1 references)
target prot opt source destination
-DROP icmp -- 0.0.0.0/00.0.0.0/0 #conn/32 1
-DROP tcp -- 0.0.0.0/00.0.0.0/0 #conn/32 2
+DROP icmp -- 0.0.0.0/00.0.0.0/0 #conn src/32 1
+DROP tcp -- 0.0.0.0/00.0.0.0/0 #conn src/32 2
RETURN all -- 0.0.0.0/00.0.0.0/0 state
NEW,ESTABLISHED ctdir REPLY
#iptables -L FO-vnet0 -n
Chain FO-vnet0 (1 references)
target prot opt source destination
ACCEPT all -- 0.0.0.0/00.0.0.0/0 state
ESTABLISHED ctdir ORIGINAL
-#iptables -L HI-vnet0 -n
+#iptables -L HI-vnet0 -n | sed 's|#conn/|#conn src/|'
Chain HI-vnet0 (1 references)
target prot opt source destination
-DROP icmp -- 0.0.0.0/00.0.0.0/0 #conn/32 1
-DROP tcp -- 0.0.0.0/00.0.0.0/0 #conn/32 2
+DROP icmp -- 0.0.0.0/00.0.0.0/0 #conn src/32 1
+DROP tcp -- 0.0.0.0/00.0.0.0/0 #conn src/32 2
RETURN all -- 0.0.0.0/00.0.0.0/0 state
NEW,ESTABLISHED ctdir REPLY
#iptables -L libvirt-host-in -n | grep vnet0 | tr -s
HI-vnet0 all -- 0.0.0.0/0 0.0.0.0/0 [goto] PHYSDEV match --physdev-in vnet0
--
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] [PATCHv6 6/8] blockjob: implement block copy for qemu
On 04/23/2012 08:49 PM, Eric Blake wrote:
Minimal patch to wire up all the pieces in the previous patches
to actually enable a block copy job. By minimal, I mean that
qemu creates the file (that is, no REUSE_EXT flag support yet),
SELinux must be disabled, a lock manager is not informed, and
the audit logs aren't updated. But those will be added as
improvements in future patches.
* src/qemu/qemu_driver.c (qemuDomainBlockCopy): New function.
(qemuDomainBlockRebase): Call it when appropriate.
---
v6: no real changes from v5
src/qemu/qemu_driver.c | 124
+++-
1 files changed, 122 insertions(+), 2 deletions(-)
Squash this in to deal with the recent change to block-stream taking a
speed argument, since that fix deletes BLOCK_JOB_SPEED_INTERNAL.
diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
index 4b5c8ad..045c8da 100644
--- i/src/qemu/qemu_driver.c
+++ w/src/qemu/qemu_driver.c
@@ -12005,6 +12005,12 @@ qemuDomainBlockCopy(virDomainPtr dom, const
char *path,
_(block copy is not supported with this QEMU
binary));
goto cleanup;
}
+if (bandwidth) {
+qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
+_(setting initial speed of block copy is not
+ supported with this QEMU binary));
+goto cleanup;
+}
if (vm-persistent) {
/* XXX if qemu ever lets us start a new domain with mirroring
* already active, we can relax this; but for now, the risk of
@@ -12040,9 +12046,6 @@ qemuDomainBlockCopy(virDomainPtr dom, const char
*path,
/* Actually start the mirroring */
qemuDomainObjEnterMonitorWithDriver(driver, vm);
ret = qemuMonitorDriveMirror(priv-mon, device, dest, format, flags);
-if (ret == 0 bandwidth != 0)
-ret = qemuMonitorBlockJob(priv-mon, device, NULL, bandwidth, NULL,
- BLOCK_JOB_SPEED_INTERNAL, true);
qemuDomainObjExitMonitorWithDriver(driver, vm);
endjob:
--
Eric Blake ebl...@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: improve errors related to offline domains
On 04/26/2012 03:50 PM, Eric Blake wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=816662 pointed out
that attempting 'virsh blockpull' on an offline domain gave a
misleading error message about qemu lacking support for the
operation, even when qemu was specifically updated to support it.
The real problem is that we have several capabilities that are
only determined when starting a domain, and therefore are still
clear when first working with an inactive domain (namely, any
capability set by qemuMonitorJSONCheckCommands).
[...]
* src/qemu/qemu_driver.c (qemuDomainPMSuspendForDuration)
(qemuDomainSnapshotCreateXML, qemuDomainBlockJobImpl): Check for
offline state before checking an online-only cap.
---
src/qemu/qemu_driver.c | 24 ++--
1 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d3aa34d..3a1c1c4 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10389,6 +10389,12 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
goto cleanup;
if (flags VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) {
+if (!virDomainObjIsActive(vm)) {
+qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
+_(disk snapshots of inactive domains not
+ implemented yet));
+goto cleanup;
+}
if (virDomainSnapshotAlignDisks(def,
VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL,
false) 0)
@@ -10443,12 +10449,6 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
* makes sense, such as checking that qemu-img recognizes the
* snapshot name in at least one of the domain's disks? */
} else if (flags VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) {
-if (!virDomainObjIsActive(vm)) {
-qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
-_(disk snapshots of inactive domains not
- implemented yet));
-goto cleanup;
-}
if (qemuDomainSnapshotCreateDiskActive(domain-conn, driver,
vm, snap, flags) 0)
goto cleanup;
So this moves the check to an earlier place.
@@ -11642,6 +11642,12 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char
*path, const char *base,
_(no domain with matching uuid '%s'), uuidstr);
goto cleanup;
}
+if (!virDomainObjIsActive(vm)) {
+qemuReportError(VIR_ERR_OPERATION_INVALID, %s,
+_(domain is not running));
+goto endjob;
+}
+
priv = vm-privateData;
if (qemuCapsGet(priv-qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC)) {
async = true;
'goto cleanup' here -- same comment as below.
@@ -12637,6 +12643,12 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom,
priv = vm-privateData;
+if (!virDomainObjIsActive(vm)) {
+qemuReportError(VIR_ERR_OPERATION_INVALID,
+%s, _(domain is not running));
+goto endjob;
+}
+
Nit, you could move this above the priv =. Neither endjob nor cleanup
need this priv.
But 'goto endjob' is wrong (replace with 'goto cleanup'), since only
later (~line 12655) the
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) 0)
goto cleanup;
is run.
Stefan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv6 9/8] blockjob: allow speed setting in block copy
Similar to the recent race fix for 'block-stream', it is possible
to set the speed of a block copy job up front thanks to an optional
'speed' parameter to 'drive-mirror'.
* src/qemu/qemu_monitor_json.c (qemuMonitorJSONDriveMirror): Set
speed at job start.
* src/qemu/qemu_monitor_json.h (qemuMonitorJSONDriveMirror): Add
parameter.
* src/qemu/qemu_monitor.h (qemuMonitorDriveMirror): Likewise.
* src/qemu/qemu_driver.c (qemuDomainBlockCopy): Adjust caller.
* src/qemu/qemu_monitor.c (qemuMonitorDriveMirror): Likewise.
---
v6: new patch. I haven't actually seen a qemu patch implementing this
yet, but it is a logical extension of the recent block-stream patch.
src/qemu/qemu_driver.c |3 ++-
src/qemu/qemu_monitor.c | 22 +-
src/qemu/qemu_monitor.h |1 +
src/qemu/qemu_monitor_json.c |5 -
src/qemu/qemu_monitor_json.h |1 +
5 files changed, 25 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 3a9e2d0..994393a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12108,7 +12108,8 @@ qemuDomainBlockCopy(virDomainPtr dom, const char *path,
/* Actually start the mirroring */
qemuDomainObjEnterMonitorWithDriver(driver, vm);
-ret = qemuMonitorDriveMirror(priv-mon, device, dest, format, flags);
+ret = qemuMonitorDriveMirror(priv-mon, device, dest, format, bandwidth,
+ flags);
virDomainAuditDisk(vm, NULL, dest, mirror, ret = 0);
qemuDomainObjExitMonitorWithDriver(driver, vm);
if (ret 0) {
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index fc84146..0c2ab48 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2685,19 +2685,31 @@ qemuMonitorDiskSnapshot(qemuMonitorPtr mon,
virJSONValuePtr actions,
return ret;
}
-/* Start a drive-mirror block job. */
+/* Start a drive-mirror block job. bandwidth is in MB/sec. */
int
qemuMonitorDriveMirror(qemuMonitorPtr mon,
const char *device, const char *file,
- const char *format, unsigned int flags)
+ const char *format, unsigned long bandwidth,
+ unsigned int flags)
{
int ret = -1;
+unsigned long long speed;
+
+VIR_DEBUG(mon=%p, device=%s, file=%s, format=%s, bandwidth=%luM,
flags=%x,
+ mon, device, file, NULLSTR(format), bandwidth, flags);
-VIR_DEBUG(mon=%p, device=%s, file=%s, format=%s, flags=%x,
- mon, device, file, NULLSTR(format), flags);
+/* Convert bandwidth MiB to bytes */
+if (bandwidth ULLONG_MAX / 1024 / 1024) {
+qemuReportError(VIR_ERR_OVERFLOW,
+_(bandwidth must be less than %llu),
+ULLONG_MAX / 1024 / 1024);
+return -1;
+}
+speed = bandwidth * 1024ULL * 1024ULL;
if (mon-json)
-ret = qemuMonitorJSONDriveMirror(mon, device, file, format, flags);
+ret = qemuMonitorJSONDriveMirror(mon, device, file, format, speed,
+ flags);
else
qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
_(drive-mirror requires JSON monitor));
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 9ff8578..9dd0302 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -514,6 +514,7 @@ int qemuMonitorDriveMirror(qemuMonitorPtr mon,
const char *device,
const char *file,
const char *format,
+ unsigned long bandwidth,
unsigned int flags)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
int qemuMonitorDriveReopen(qemuMonitorPtr mon,
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 7b459ff..51b19cd 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -3204,10 +3204,12 @@ cleanup:
return ret;
}
+/* speed is in bytes/sec */
int
qemuMonitorJSONDriveMirror(qemuMonitorPtr mon,
const char *device, const char *file,
- const char *format, unsigned int flags)
+ const char *format, unsigned long long speed,
+ unsigned int flags)
{
int ret = -1;
virJSONValuePtr cmd;
@@ -3219,6 +3221,7 @@ qemuMonitorJSONDriveMirror(qemuMonitorPtr mon,
s:device, device,
s:target, file,
b:full, !shallow,
+ U:speed, speed,
s:mode,
reuse ? existing : absolute-paths,
format ? s:format : NULL, format,
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
Re: [libvirt] [PATCH] qemu: improve errors related to offline domains
On 04/26/2012 02:11 PM, Stefan Berger wrote:
On 04/26/2012 03:50 PM, Eric Blake wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=816662 pointed out
that attempting 'virsh blockpull' on an offline domain gave a
misleading error message about qemu lacking support for the
operation, even when qemu was specifically updated to support it.
The real problem is that we have several capabilities that are
only determined when starting a domain, and therefore are still
clear when first working with an inactive domain (namely, any
capability set by qemuMonitorJSONCheckCommands).
@@ -12637,6 +12643,12 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom,
priv = vm-privateData;
+if (!virDomainObjIsActive(vm)) {
+qemuReportError(VIR_ERR_OPERATION_INVALID,
+%s, _(domain is not running));
+goto endjob;
+}
+
Nit, you could move this above the priv =. Neither endjob nor cleanup
need this priv.
But 'goto endjob' is wrong (replace with 'goto cleanup'),
D'oh - too much copy-and-paste! You're obviously right (and this is why
we do code review).
--
Eric Blake ebl...@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/2] nwfilter: address coverity findings
On 04/26/2012 01:17 PM, Stefan Berger wrote:
This patch addresses the following coverity findings:
/libvirt/src/conf/nwfilter_params.c:157:
deref_parm: Directly dereferencing parameter val.
/libvirt/src/conf/nwfilter_params.c:473:
negative_returns: Using variable iterIndex as an index to array
res-iter.
/libvirt/src/nwfilter/nwfilter_ebiptables_driver.c:2891:
unchecked_value: No check of the return value of virAsprintf(protostr,
-d 01:80:c2:00:00:00 ).
/libvirt/src/nwfilter/nwfilter_ebiptables_driver.c:2894:
unchecked_value: No check of the return value of virAsprintf(protostr,
-p 0x%04x , l3_protocols[protoidx].attr).
/libvirt/src/nwfilter/nwfilter_ebiptables_driver.c:3590:
var_deref_op: Dereferencing null variable inst.
---
src/conf/nwfilter_params.c|5 -
src/nwfilter/nwfilter_ebiptables_driver.c | 10 +++---
2 files changed, 11 insertions(+), 4 deletions(-)
Nice little grab-bag of patches.
if (virNWFilterVarCombIterAddVariable(res-iter[iterIndex],
Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c
===
--- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.c
+++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c
@@ -2878,6 +2878,7 @@ ebtablesCreateTmpSubChain(ebiptablesRule
char chainPrefix = (incoming) ? CHAINPREFIX_HOST_IN_TEMP
: CHAINPREFIX_HOST_OUT_TEMP;
char *protostr = NULL;
+int r = 0;
No need for r.
PRINT_ROOT_CHAIN(rootchain, chainPrefix, ifname);
PRINT_CHAIN(chain, chainPrefix, ifname,
@@ -2888,14 +2889,14 @@ ebtablesCreateTmpSubChain(ebiptablesRule
protostr = strdup();
break;
case L2_PROTO_STP_IDX:
-virAsprintf(protostr, -d NWFILTER_MAC_BGA );
+r = virAsprintf(protostr, -d NWFILTER_MAC_BGA );
Here, I'd just write:
ignore_value(virAsprintf(...));
break;
default:
-virAsprintf(protostr, -p 0x%04x , l3_protocols[protoidx].attr);
+r = virAsprintf(protostr, -p 0x%04x ,
and here,
l3_protocols[protoidx].attr);
break;
}
-if (!protostr) {
because virAsprintf guarantees that if it returned 0, then protostr is
NULL and you have an OOM situation. In other words, our code is already
correct, and we just need the ignore_value() wrapper to shut up the
static analyzer.
The other hunks were right.
ACK with the cleanup mentioned, I'm okay if you push without posting a v2.
--
Eric Blake ebl...@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/2] [TCK] nwfilter: Adapt test program and cases to recent iptables
On 04/23/2012 06:21 AM, Stefan Berger wrote: Recent iptables fixes a lot of issues with missing spaces and other information that was previously not reported properly. To make the test program and test cases work on old and newer installations of iptables tools, some adjustments need to be made. Fix a 'file not found error' when running this tool from the shell directly. --- scripts/nwfilter/nwfilter2vmtest.sh|6 +++--- scripts/nwfilter/nwfilterxml2fwallout/conntrack-test.fwall | 12 ++-- 2 files changed, 9 insertions(+), 9 deletions(-) ACK. -- Eric Blake ebl...@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 2/2] nwfilter: address more coverity findings
On 04/26/2012 01:46 PM, Stefan Berger wrote:
This patch addresses the following coverity findings:
/libvirt/src/conf/nwfilter_params.c:390:
var_assigned: Assigning: varValue = null return value from
virHashLookup.
/libvirt/src/conf/nwfilter_params.c:392:
dereference: Dereferencing a pointer that might be null varValue when
calling virNWFilterVarValueGetNthValue.
/libvirt/src/conf/nwfilter_params.c:399:
dereference: Dereferencing a pointer that might be null tmp when
calling virNWFilterVarValueGetNthValue.
---
src/conf/nwfilter_params.c | 16 +++-
1 file changed, 15 insertions(+), 1 deletion(-)
Nice to have tools that help us catch bugs.
@@ -391,14 +391,28 @@ virNWFilterVarCombIterEntryAreUniqueEntr
const char *value;
varValue = virHashLookup(hash-hashTable, cie-varNames[0]);
+if (!varValue) {
+/* caller's error */
+VIR_ERROR(_(%s: hash lookup resulted in NULL pointer),
__func__);
VIR_ERROR already appends __func__ to the resulting message. This
should be:
VIR_ERROR(%s, _(hash lookup resulted in NULL pointer));
value = virNWFilterVarValueGetNthValue(varValue, cie-curValue);
+if (!value) {
+VIR_ERROR(_(%s: Lookup of value at index %u resulted in a NULL
+ pointer), __func__, cie-curValue);
And again, this should be:
VIR_ERROR(_(Lookup of value at index %u resulted in a NULL pointer),
cie-curValue);
ACK with those fixes; I'm okay if you push without posting a v2.
--
Eric Blake ebl...@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] [PATCHv6 9/8] blockjob: allow speed setting in block copy
On 04/26/2012 04:15 PM, Eric Blake wrote:
Similar to the recent race fix for 'block-stream', it is possible
to set the speed of a block copy job up front thanks to an optional
'speed' parameter to 'drive-mirror'.
* src/qemu/qemu_monitor_json.c (qemuMonitorJSONDriveMirror): Set
speed at job start.
* src/qemu/qemu_monitor_json.h (qemuMonitorJSONDriveMirror): Add
parameter.
* src/qemu/qemu_monitor.h (qemuMonitorDriveMirror): Likewise.
* src/qemu/qemu_driver.c (qemuDomainBlockCopy): Adjust caller.
* src/qemu/qemu_monitor.c (qemuMonitorDriveMirror): Likewise.
---
v6: new patch. I haven't actually seen a qemu patch implementing this
yet, but it is a logical extension of the recent block-stream patch.
Hm, not sure what this means compared to what you write on the top...
-VIR_DEBUG(mon=%p, device=%s, file=%s, format=%s, flags=%x,
- mon, device, file, NULLSTR(format), flags);
+/* Convert bandwidth MiB to bytes */
+if (bandwidth ULLONG_MAX / 1024 / 1024) {
ULLONG_MAX / (1024 * 1024) -- but yours is also fine...
ACK.
Stefan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv6 9/8] blockjob: allow speed setting in block copy
On 04/26/2012 02:38 PM, Stefan Berger wrote: On 04/26/2012 04:15 PM, Eric Blake wrote: Similar to the recent race fix for 'block-stream', it is possible to set the speed of a block copy job up front thanks to an optional 'speed' parameter to 'drive-mirror'. * src/qemu/qemu_monitor_json.c (qemuMonitorJSONDriveMirror): Set speed at job start. * src/qemu/qemu_monitor_json.h (qemuMonitorJSONDriveMirror): Add parameter. * src/qemu/qemu_monitor.h (qemuMonitorDriveMirror): Likewise. * src/qemu/qemu_driver.c (qemuDomainBlockCopy): Adjust caller. * src/qemu/qemu_monitor.c (qemuMonitorDriveMirror): Likewise. --- v6: new patch. I haven't actually seen a qemu patch implementing this yet, but it is a logical extension of the recent block-stream patch. Hm, not sure what this means compared to what you write on the top... It means: Paolo proposed 'drive-mirror' to upstream qemu prior to Stefan Hajnoczi's patch to 'block-stream'. qemu 1.1 has included Stefan's patch, but Paolo's is still in limbo, and needs to be rebased if it will even make qemu 1.1. But it is a logical extension to assume that when Paolo rebases his drive-mirror patches, that he will also add an optional 'speed' parameter; so as to avoid the same data race as was just fixed in 'block-stream'. And until I can point to an actual qemu commit with 'drive-mirror' (at all, not just 'drive-mirror' with speed), this patch is stalled just like the other 8 patches in this series. -- Eric Blake ebl...@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] udevadm settle can take too long
Osier Yang wrote:
On 2012年04月24日 03:47, Guido Günther wrote:
Hi,
On Sun, Apr 22, 2012 at 02:41:54PM -0400, Jim Paris wrote:
Hi,
http://bugs.debian.org/663931 is a bug I'm hitting, where virt-manager
times out on the initial connection to libvirt.
I reassigned the bug back to libvirt. I still wonder what triggers this
though for some users but not for others?
Cheers,
-- Guido
The basic problem is that, while checking storage volumes,
virt-manager causes libvirt to call udevadm settle. There's an
interaction where libvirt's earlier use of network namespaces (to probe
LXC features) had caused some uevents to be sent that get filtered out
before they reach udev. This confuses udevadm settle a bit, and so
it sits there waiting for a 2-3 minute built-in timeout before returning.
Eventually libvirtd prints:
2012-04-22 18:22:18.678+: 30503: warning : virKeepAliveTimer:182 :
No response from client 0x7feec4003630 after 5 keepalive messages in 30
seconds
and virt-manager prints:
2012-04-22 18:22:18.931+: 30647: warning : virKeepAliveSend:128 :
Failed to send keepalive response to client 0x25004e0
and the connection gets dropped.
One workaround could be to specify a shorter timeout when doing the
settle. The patch appended below allows virt-manager to work,
although the connection still has to wait for the 10 second timeout
before it succeeds. I don't know what a better solution would be,
though. It seems the udevadm behavior might not be considered a bug
from the udev/kernel point of view:
https://lkml.org/lkml/2012/4/22/60
I'm using Linux 3.2.14 with libvirt 0.9.11. You can trigger the
udevadm issue using a program I posted at the Debian bug report link
above.
-jim
From 17e5b9ebab76acb0d711e8bc308023372fbc4180 Mon Sep 17 00:00:00 2001
From: Jim Parisj...@jtan.com
Date: Sun, 22 Apr 2012 14:35:47 -0400
Subject: [PATCH] shorten udevadmin settle timeout
Otherwise, udevadmin settle can take so long that connections from
e.g. virt-manager will get closed.
---
src/util/util.c |4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c
index 6e041d6..dfe458e 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -2593,9 +2593,9 @@ virFileFindMountPoint(const char *type
ATTRIBUTE_UNUSED)
void virFileWaitForDevices(void)
{
# ifdef UDEVADM
-const char *const settleprog[] = { UDEVADM, settle, NULL };
+const char *const settleprog[] = { UDEVADM, settle, --timeout,
10, NULL };
Though I don't have a good idea to fix it either, I guess this
change could cause lvremove to fail again for the udev race.
See BZs:
https://bugzilla.redhat.com/show_bug.cgi?id=702260
https://bugzilla.redhat.com/show_bug.cgi?id=570359
It seems that those bugs were caused by something like
1. open(lv, O_RDWR)
2. close(lv)
3. system(lvremove ...)
where udev would fire off a command between 2 and 3 that caused 3 to
fail. Adding udevadm settle as step 2.5 is a good way to wait for
that command to finish, but:
- it doesn't necessarily fix the issue; something could easily re-open
the device between 2.5 and 3 and cause the same failure.
- the race condition sounds like it was a short window, and sometimes
the original sequence would still work even without the settle.
That would suggest to me that a timeout of 10s is still plenty long.
A few thoughts:
- For lvremove: can we try a short timeout (3 seconds), then if the
lvremove still fails, try again with the default udevadm timeout
(120 seconds)?
- Even in that case, we need to fix libvirtd to not kill the
connection after 30 seconds when it's libvirtd's fault that the
connection is blocked for so long anyway.
- When connecting with virt-manager, is the udevadm settle really
necessary? We're not calling lvremove.
Thanks,
-jim
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] [TCK] nwfilter: Add test cases for ipset
On 04/23/2012 06:20 AM, Stefan Berger wrote:
Add test cases for the ipset extension.
Since ipset may not be available on all system, the first line of the XML
file containing the test filter has been extended with a specially formatted
XML comment containing a command line test for whether the test case can be
run at all. The format of that line is:
!-- #command line test# --
If the tests in this line don't succeed, the test case is skipped.
Seems like a slick idea.
- ${VIRSH} nwfilter-define ${xmlfile} /dev/null
+ # Check whether we can run this test at all
+ cmd=`sed -n '1,1 s/^\!--[ ^I]*#\(.*\)#[ ^I]*--/\1/p' ${xmlfile}`
Use $(), not `` (since we're already using $(()), we don't have to worry
about Solaris /bin/sh, but might as well stick to the preferred POSIX
shell interface).
1,1 as a sed address selection is redundant; you could shorten it to 1.
In sed, ^I does NOT mean tab, but the two characters ^ and I. Use a
literal tab instead (and to avoid space-tab warnings, list the bracket
expression as [tab-space], as in '[ ]*').
+ if [ -n ${cmd} ]; then
+eval ${cmd} 21 1/dev/null
This says output any errors from command to our stdout, and to ignore
normal output of $cmd. Is that what you meant, or did you want to
ignore both output and errors from $cmd, in which case you should swap
the redirection operators?
Otherwise, it looks okay to me.
--
Eric Blake ebl...@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] [libvirt-TCK][PATCH] use 'raw' format as the format of backing file of qcow2 image
On 02/28/2012 03:38 AM, Guannan Ren wrote:
If we don't explicitly specify the format of backing file,
it should use raw by default, if so, libvirt's security drivers
should *not* grant access to the last.img file the guest
should not see the last.img data. That is the purpose of testing.
---
scripts/qemu/205-qcow2-double-backing-file.t |1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/scripts/qemu/205-qcow2-double-backing-file.t
b/scripts/qemu/205-qcow2-double-backing-file.t
index d3a5e33..ad54c7c 100644
--- a/scripts/qemu/205-qcow2-double-backing-file.t
+++ b/scripts/qemu/205-qcow2-double-backing-file.t
@@ -114,7 +114,6 @@ SKIP: {
my $volmainxml = $tck-generic_volume(tck-main, qcow2, 1024*1024*50)
-backing_file($pathback)
- -backing_format(qcow2)
-allocation(0)-as_xml;
Just noticing this (sorry for the delay); ACK.
--
Eric Blake ebl...@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] macvtap: fix a typo
Below patch fixes the following coverity findings
Error: OVERRUN_STATIC:
/libvirt/src/qemu/qemu_command.c:152:
overrun-buffer-val: Overrunning static array net-mac of size 6 bytes
by passing it as an argument to a function which indexes it at byte
position 15.
/libvirt/src/util/virnetdevmacvlan.c:948:
access_dbuff_const: Calling
virNetDevMacVLanVPortProfileRegisterCallback indexes array
macaddress at byte position 15.
/libvirt/src/util/virnetdevmacvlan.c:773:
access_dbuff_const: Calling memcpy indexes array macaddress with
index 16UL at byte position 15.
Error: OVERRUN_STATIC:
/libvirt/src/qemu/qemu_migration.c:2744:
overrun-buffer-val: Overrunning static array net-mac of size 6 bytes
by passing it as an argument to a function which indexes it at byte
position 15.
/libvirt/src/util/virnetdevmacvlan.c:773:
access_dbuff_const: Calling memcpy indexes array macaddress with
index 16UL at byte position 15.
Error: OVERRUN_STATIC:
/libvirt/src/qemu/qemu_driver.c:435:
overrun-buffer-val: Overrunning static array net-mac of size 6 bytes
by passing it as an argument to a function which indexes it at byte
position 15.
/libvirt/src/util/virnetdevmacvlan.c:1036:
access_dbuff_const: Calling
virNetDevMacVLanVPortProfileRegisterCallback indexes array
macaddress at byte position 15.
/libvirt/src/util/virnetdevmacvlan.c:773:
access_dbuff_const: Calling memcpy indexes array macaddress with
index 16UL at byte position 15.
---
src/util/virnetdevmacvlan.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Index: libvirt-acl/src/util/virnetdevmacvlan.c
===
--- libvirt-acl.orig/src/util/virnetdevmacvlan.c
+++ libvirt-acl/src/util/virnetdevmacvlan.c
@@ -435,7 +435,7 @@ static const uint32_t modeMap[VIR_NETDEV
struct virNetlinkCallbackData {
char *cr_ifname;
virNetDevVPortProfilePtr virtPortProfile;
-unsigned char macaddress[VIR_UUID_BUFLEN];
+unsigned char macaddress[VIR_MAC_BUFLEN];
char *linkdev;
int vf;
unsigned char vmuuid[VIR_UUID_BUFLEN];
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] macvtap: fix a typo
On 04/26/2012 03:16 PM, Stefan Berger wrote:
Below patch fixes the following coverity findings
Error: OVERRUN_STATIC:
/libvirt/src/qemu/qemu_command.c:152:
overrun-buffer-val: Overrunning static array net-mac of size 6 bytes
by passing it as an argument to a function which indexes it at byte
position 15.
Index: libvirt-acl/src/util/virnetdevmacvlan.c
===
--- libvirt-acl.orig/src/util/virnetdevmacvlan.c
+++ libvirt-acl/src/util/virnetdevmacvlan.c
@@ -435,7 +435,7 @@ static const uint32_t modeMap[VIR_NETDEV
struct virNetlinkCallbackData {
char *cr_ifname;
virNetDevVPortProfilePtr virtPortProfile;
-unsigned char macaddress[VIR_UUID_BUFLEN];
+unsigned char macaddress[VIR_MAC_BUFLEN];
Yep, any client that uses sizeof() was picking up 10 bytes of garbage. ACK.
--
Eric Blake ebl...@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: Stable release libvirt-0.9.11.2
Okay, I screwed up the tarball for the first stable release, due to not building it from a fresh checkout :/ No changes for this one except a version bump and dist rebuild. This release can be downloaded at: http://libvirt.org/sources/libvirt-0.9.11.2.tar.gz Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: improve errors related to offline domains
On 04/26/2012 02:17 PM, Eric Blake wrote:
On 04/26/2012 02:11 PM, Stefan Berger wrote:
On 04/26/2012 03:50 PM, Eric Blake wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=816662 pointed out
that attempting 'virsh blockpull' on an offline domain gave a
misleading error message about qemu lacking support for the
operation, even when qemu was specifically updated to support it.
The real problem is that we have several capabilities that are
only determined when starting a domain, and therefore are still
clear when first working with an inactive domain (namely, any
capability set by qemuMonitorJSONCheckCommands).
@@ -12637,6 +12643,12 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom,
priv = vm-privateData;
+if (!virDomainObjIsActive(vm)) {
+qemuReportError(VIR_ERR_OPERATION_INVALID,
+%s, _(domain is not running));
+goto endjob;
+}
+
Nit, you could move this above the priv =. Neither endjob nor cleanup
need this priv.
But 'goto endjob' is wrong (replace with 'goto cleanup'),
D'oh - too much copy-and-paste! You're obviously right (and this is why
we do code review).
Pushed with the two labels fixed, per your ACK on IRC.
--
Eric Blake ebl...@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] [Libvirt-announce] ANNOUNCE: Stable release libvirt-0.9.11.2
On 27/04/2012, at 8:04 AM, Cole Robinson wrote: Okay, I screwed up the tarball for the first stable release, due to not building it from a fresh checkout :/ No changes for this one except a version bump and dist rebuild. This release can be downloaded at: http://libvirt.org/sources/libvirt-0.9.11.2.tar.gz Should there be entries for 0.9.11.1 and 0.9.11.2 on the News page? http://libvirt.org/news.html + Justin Thanks, Cole ___ Libvirt-announce mailing list libvirt-annou...@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-announce -- Aeolus Community Manager http://www.aeolusproject.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Files too long for tar archive
In building the libvirt-0.9.11.2 stable tarball, I saw these errors: tar: libvirt-0.9.11.2/tests/nodeinfodata/linux-nodeinfo-sysfs-test-1/cpu/cpu0/topology/thread_siblings_list: file name is too long (max 99); not dumped tar: libvirt-0.9.11.2/tests/nodeinfodata/linux-nodeinfo-sysfs-test-1/cpu/cpu0/topology/physical_package_id: file name is too long (max 99); not dumped tar: libvirt-0.9.11.2/tests/nodeinfodata/linux-nodeinfo-sysfs-test-1/cpu/cpu1/topology/thread_siblings_list: file name is too long (max 99); not dumped tar: libvirt-0.9.11.2/tests/nodeinfodata/linux-nodeinfo-sysfs-test-1/cpu/cpu1/topology/physical_package_id: file name is too long (max 99); not dumped tar: Exiting with failure status due to previous errors As indicated the files were not in the tarball. Can we just update the tar format in configure or does that come with caveats? Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] udevadm settle can take too long
[ CC to Cole ]
Osier Yang wrote:
On 2012年04月24日 03:47, Guido Günther wrote:
Hi,
On Sun, Apr 22, 2012 at 02:41:54PM -0400, Jim Paris wrote:
Hi,
http://bugs.debian.org/663931 is a bug I'm hitting, where virt-manager
times out on the initial connection to libvirt.
I reassigned the bug back to libvirt. I still wonder what triggers this
though for some users but not for others?
Cheers,
-- Guido
The basic problem is that, while checking storage volumes,
virt-manager causes libvirt to call udevadm settle. There's an
interaction where libvirt's earlier use of network namespaces (to probe
LXC features) had caused some uevents to be sent that get filtered out
before they reach udev. This confuses udevadm settle a bit, and so
it sits there waiting for a 2-3 minute built-in timeout before returning.
Eventually libvirtd prints:
2012-04-22 18:22:18.678+: 30503: warning : virKeepAliveTimer:182 : No
response from client 0x7feec4003630 after 5 keepalive messages in 30 seconds
and virt-manager prints:
2012-04-22 18:22:18.931+: 30647: warning : virKeepAliveSend:128 : Failed
to send keepalive response to client 0x25004e0
and the connection gets dropped.
One workaround could be to specify a shorter timeout when doing the
settle. The patch appended below allows virt-manager to work,
although the connection still has to wait for the 10 second timeout
before it succeeds. I don't know what a better solution would be,
though. It seems the udevadm behavior might not be considered a bug
from the udev/kernel point of view:
https://lkml.org/lkml/2012/4/22/60
I'm using Linux 3.2.14 with libvirt 0.9.11. You can trigger the
udevadm issue using a program I posted at the Debian bug report link
above.
-jim
From 17e5b9ebab76acb0d711e8bc308023372fbc4180 Mon Sep 17 00:00:00 2001
From: Jim Parisj...@jtan.com
Date: Sun, 22 Apr 2012 14:35:47 -0400
Subject: [PATCH] shorten udevadmin settle timeout
Otherwise, udevadmin settle can take so long that connections from
e.g. virt-manager will get closed.
---
src/util/util.c |4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c
index 6e041d6..dfe458e 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -2593,9 +2593,9 @@ virFileFindMountPoint(const char *type ATTRIBUTE_UNUSED)
void virFileWaitForDevices(void)
{
# ifdef UDEVADM
-const char *const settleprog[] = { UDEVADM, settle, NULL };
+const char *const settleprog[] = { UDEVADM, settle, --timeout, 10,
NULL };
Though I don't have a good idea to fix it either, I guess this
change could cause lvremove to fail again for the udev race.
See BZs:
https://bugzilla.redhat.com/show_bug.cgi?id=702260
https://bugzilla.redhat.com/show_bug.cgi?id=570359
It seems that those bugs were caused by something like
1. open(lv, O_RDWR)
2. close(lv)
3. system(lvremove ...)
where udev would fire off a command between 2 and 3 that caused 3 to
fail. Adding udevadm settle as step 2.5 is a good way to wait for
that command to finish, but:
- it doesn't necessarily fix the issue; something could easily re-open
the device between 2.5 and 3 and cause the same failure.
Right.
- the race condition sounds like it was a short window, and sometimes
the original sequence would still work even without the settle.
That would suggest to me that a timeout of 10s is still plenty long.
A few thoughts:
- For lvremove: can we try a short timeout (3 seconds), then if the
lvremove still fails, try again with the default udevadm timeout
(120 seconds)?
- Even in that case, we need to fix libvirtd to not kill the
connection after 30 seconds when it's libvirtd's fault that the
connection is blocked for so long anyway.
perhaps we need a timeout property for the client connection,
but not hardcode to 30s.
- When connecting with virt-manager, is the udevadm settle really
necessary? We're not calling lvremove.
virt-manager's hung should be caused by pool refresh, which
uses udevadm settle to wait for the new devices show up. So
it doesn't relates with lvremove.
Except logical storage, storage type of disk, scsi, and
mpath uses udevadm settle too. And node device driver.
Generally the pool refresh will be involked when libvirtd starts,
and surely another case is it's involked explicitly. :-) I.e.
virt-manager can't be hung if it doesn't intent to refresh the
pool. And thus I guess the situation will be much worse if pools
of disk, logical, scsi, mpath exists all together.
I'm wondering if virt-manager try to refresh the pools when
it starts, or when user request to check storage explicitly,
(e.g. clicking some button). It should be improved if it's the
first case IMHO, (let the user get the connection, and refresh
the pool when neccessary could be better).
I'd agree with that introducing timeout argument for udevadm
settle will be better, but hardcode a timeout in
virFileWaitForDevices is not good, as we can see, it's used
many
Re: [libvirt] Files too long for tar archive
On 2012年04月27日 08:17, Cole Robinson wrote: In building the libvirt-0.9.11.2 stable tarball, I saw these errors: tar: libvirt-0.9.11.2/tests/nodeinfodata/linux-nodeinfo-sysfs-test-1/cpu/cpu0/topology/thread_siblings_list: file name is too long (max 99); not dumped tar: libvirt-0.9.11.2/tests/nodeinfodata/linux-nodeinfo-sysfs-test-1/cpu/cpu0/topology/physical_package_id: file name is too long (max 99); not dumped tar: libvirt-0.9.11.2/tests/nodeinfodata/linux-nodeinfo-sysfs-test-1/cpu/cpu1/topology/thread_siblings_list: file name is too long (max 99); not dumped tar: libvirt-0.9.11.2/tests/nodeinfodata/linux-nodeinfo-sysfs-test-1/cpu/cpu1/topology/physical_package_id: file name is too long (max 99); not dumped tar: Exiting with failure status due to previous errors As indicated the files were not in the tarball. Can we just update the tar format in configure or does that come with caveats? quote tar-ustar selects the ustar format defined by POSIX 1003.1-1988. This format is believed to be old enough to be portable. /quote Using 'tar-ustar'? Regards, Osier -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-TCK][PATCH] use 'raw' format as the format of backing file of qcow2 image
On 04/27/2012 05:15 AM, Eric Blake wrote:
On 02/28/2012 03:38 AM, Guannan Ren wrote:
If we don't explicitly specify the format of backing file,
it should use raw by default, if so, libvirt's security drivers
should *not* grant access to the last.img file the guest
should not see the last.img data. That is the purpose of testing.
---
scripts/qemu/205-qcow2-double-backing-file.t |1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/scripts/qemu/205-qcow2-double-backing-file.t
b/scripts/qemu/205-qcow2-double-backing-file.t
index d3a5e33..ad54c7c 100644
--- a/scripts/qemu/205-qcow2-double-backing-file.t
+++ b/scripts/qemu/205-qcow2-double-backing-file.t
@@ -114,7 +114,6 @@ SKIP: {
my $volmainxml = $tck-generic_volume(tck-main, qcow2, 1024*1024*50)
-backing_file($pathback)
- -backing_format(qcow2)
-allocation(0)-as_xml;
Just noticing this (sorry for the delay); ACK.
That's great, thanks
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
