[Qemu-devel] [PATCH 1/1] Remove debugging messages.

[Qemu-devel] [PATCH 1/1] Remove debugging messages.

[Richard W.M. Jones]

qemu probably needs some sort of --verbose or --debug option for when
you really want to see everything that's going on.

In the meantime, this patch removes some debugging messages when KVM
is not available.

Rich.

[Qemu-devel] [PATCH] Remove debugging messages.

[Richard W.M. Jones]

From: "Richard W.M. Jones" <rjones@redhat.com>

When using qemu -machine accel=kvm:tcg, if KVM is not available you
get a load of debugging output:

  Could not access KVM kernel module: No such file or directory
  failed to initialize KVM: No such file or directory
  Back to tcg accelerator.

Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
---
 kvm-all.c |    1 -
 vl.c      |   14 --------------
 2 files changed, 0 insertions(+), 15 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index cbc2532..35d698b 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -713,7 +713,6 @@ int kvm_init(void)
     s->vmfd = -1;
     s->fd = qemu_open("/dev/kvm", O_RDWR);
     if (s->fd == -1) {
-        fprintf(stderr, "Could not access KVM kernel module: %m\n");
         ret = -errno;
         goto err;
     }
diff --git a/vl.c b/vl.c
index fcd7395..83a08b6 100644
--- a/vl.c
+++ b/vl.c
@@ -1922,7 +1922,6 @@ static int configure_accelerator(void)
     char buf[10];
     int i, ret;
     bool accel_initalised = 0;
-    bool init_failed = 0;
 
     QemuOptsList *list = qemu_find_opts("machine");
     if (!QTAILQ_EMPTY(&list->head)) {
@@ -1944,15 +1943,6 @@ static int configure_accelerator(void)
                 *(accel_list[i].allowed) = 1;
                 ret = accel_list[i].init();
                 if (ret < 0) {
-                    init_failed = 1;
-                    if (!accel_list[i].available()) {
-                        printf("%s not supported for this target\n",
-                               accel_list[i].name);
-                    } else {
-                        fprintf(stderr, "failed to initialize %s: %s\n",
-                                accel_list[i].name,
-                                strerror(-ret));
-                    }
                     *(accel_list[i].allowed) = 0;
                 } else {
                     accel_initalised = 1;
@@ -1970,10 +1960,6 @@ static int configure_accelerator(void)
         exit(1);
     }
 
-    if (init_failed) {
-        fprintf(stderr, "Back to %s accelerator.\n", accel_list[i].name);
-    }
-
     return !accel_initalised;
 }
 
-- 
1.7.5.2

Re: [Qemu-devel] [PATCH] Remove debugging messages.

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2607 bytes --]

On 2011-07-19 21:23, Richard W.M. Jones wrote:
> From: "Richard W.M. Jones" <rjones@redhat.com>
> 
> When using qemu -machine accel=kvm:tcg, if KVM is not available you
> get a load of debugging output:
> 
>   Could not access KVM kernel module: No such file or directory
>   failed to initialize KVM: No such file or directory
>   Back to tcg accelerator.
> 
> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> ---
>  kvm-all.c |    1 -
>  vl.c      |   14 --------------
>  2 files changed, 0 insertions(+), 15 deletions(-)
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index cbc2532..35d698b 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -713,7 +713,6 @@ int kvm_init(void)
>      s->vmfd = -1;
>      s->fd = qemu_open("/dev/kvm", O_RDWR);
>      if (s->fd == -1) {
> -        fprintf(stderr, "Could not access KVM kernel module: %m\n");
>          ret = -errno;
>          goto err;
>      }
> diff --git a/vl.c b/vl.c
> index fcd7395..83a08b6 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1922,7 +1922,6 @@ static int configure_accelerator(void)
>      char buf[10];
>      int i, ret;
>      bool accel_initalised = 0;
> -    bool init_failed = 0;
>  
>      QemuOptsList *list = qemu_find_opts("machine");
>      if (!QTAILQ_EMPTY(&list->head)) {
> @@ -1944,15 +1943,6 @@ static int configure_accelerator(void)
>                  *(accel_list[i].allowed) = 1;
>                  ret = accel_list[i].init();
>                  if (ret < 0) {
> -                    init_failed = 1;
> -                    if (!accel_list[i].available()) {
> -                        printf("%s not supported for this target\n",
> -                               accel_list[i].name);
> -                    } else {
> -                        fprintf(stderr, "failed to initialize %s: %s\n",
> -                                accel_list[i].name,
> -                                strerror(-ret));
> -                    }
>                      *(accel_list[i].allowed) = 0;
>                  } else {
>                      accel_initalised = 1;
> @@ -1970,10 +1960,6 @@ static int configure_accelerator(void)
>          exit(1);
>      }
>  
> -    if (init_failed) {
> -        fprintf(stderr, "Back to %s accelerator.\n", accel_list[i].name);
> -    }
> -
>      return !accel_initalised;
>  }
>  

I agree the current situation is unfortunate.

However, I think dumping some error information is useful when either no
alternatives were specified or some verbose mode was selected. Maybe add
an option "verbose=on|off" to -machine?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH] Remove debugging messages.

[Michael Tokarev]

19.07.2011 23:23, Richard W.M. Jones wrote:
> From: "Richard W.M. Jones" <rjones@redhat.com>
> 
> When using qemu -machine accel=kvm:tcg, if KVM is not available you
> get a load of debugging output:
> 
>   Could not access KVM kernel module: No such file or directory
>   failed to initialize KVM: No such file or directory
>   Back to tcg accelerator.
> 
> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> ---
>  kvm-all.c |    1 -
>  vl.c      |   14 --------------
>  2 files changed, 0 insertions(+), 15 deletions(-)
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index cbc2532..35d698b 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -713,7 +713,6 @@ int kvm_init(void)
>      s->vmfd = -1;
>      s->fd = qemu_open("/dev/kvm", O_RDWR);
>      if (s->fd == -1) {
> -        fprintf(stderr, "Could not access KVM kernel module: %m\n");

Don't do this.  qemu error reporting sucks badly already,
and this very error message gives us at least some details
(the %m part) about what's missing.  There's no other place
where this information is printed in case this open fails.
We already have to resort to strace in way too many places... :(

Thanks,

/mjt

Re: [Qemu-devel] [PATCH] Remove debugging messages.

[Anthony Liguori]

On 07/19/2011 02:23 PM, Richard W.M. Jones wrote:
> From: "Richard W.M. Jones"<rjones@redhat.com>
>
> When using qemu -machine accel=kvm:tcg, if KVM is not available you
> get a load of debugging output:
>
>    Could not access KVM kernel module: No such file or directory
>    failed to initialize KVM: No such file or directory
>    Back to tcg accelerator.

If you ask for kvm, and it can't be initialized, then shouldn't we alert 
the user about this?

Regards,

Anthony Liguori

>
> Signed-off-by: Richard W.M. Jones<rjones@redhat.com>
> ---
>   kvm-all.c |    1 -
>   vl.c      |   14 --------------
>   2 files changed, 0 insertions(+), 15 deletions(-)
>
> diff --git a/kvm-all.c b/kvm-all.c
> index cbc2532..35d698b 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -713,7 +713,6 @@ int kvm_init(void)
>       s->vmfd = -1;
>       s->fd = qemu_open("/dev/kvm", O_RDWR);
>       if (s->fd == -1) {
> -        fprintf(stderr, "Could not access KVM kernel module: %m\n");
>           ret = -errno;
>           goto err;
>       }
> diff --git a/vl.c b/vl.c
> index fcd7395..83a08b6 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1922,7 +1922,6 @@ static int configure_accelerator(void)
>       char buf[10];
>       int i, ret;
>       bool accel_initalised = 0;
> -    bool init_failed = 0;
>
>       QemuOptsList *list = qemu_find_opts("machine");
>       if (!QTAILQ_EMPTY(&list->head)) {
> @@ -1944,15 +1943,6 @@ static int configure_accelerator(void)
>                   *(accel_list[i].allowed) = 1;
>                   ret = accel_list[i].init();
>                   if (ret<  0) {
> -                    init_failed = 1;
> -                    if (!accel_list[i].available()) {
> -                        printf("%s not supported for this target\n",
> -                               accel_list[i].name);
> -                    } else {
> -                        fprintf(stderr, "failed to initialize %s: %s\n",
> -                                accel_list[i].name,
> -                                strerror(-ret));
> -                    }
>                       *(accel_list[i].allowed) = 0;
>                   } else {
>                       accel_initalised = 1;
> @@ -1970,10 +1960,6 @@ static int configure_accelerator(void)
>           exit(1);
>       }
>
> -    if (init_failed) {
> -        fprintf(stderr, "Back to %s accelerator.\n", accel_list[i].name);
> -    }
> -
>       return !accel_initalised;
>   }
>

Re: [Qemu-devel] [PATCH] Remove debugging messages.

[Richard W.M. Jones]

On Tue, Jul 19, 2011 at 11:49:22PM +0400, Michael Tokarev wrote:
> 19.07.2011 23:23, Richard W.M. Jones wrote:
> > From: "Richard W.M. Jones" <rjones@redhat.com>
> > 
> > When using qemu -machine accel=kvm:tcg, if KVM is not available you
> > get a load of debugging output:
> > 
> >   Could not access KVM kernel module: No such file or directory
> >   failed to initialize KVM: No such file or directory
> >   Back to tcg accelerator.
> > 
> > Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> > ---
> >  kvm-all.c |    1 -
> >  vl.c      |   14 --------------
> >  2 files changed, 0 insertions(+), 15 deletions(-)
> > 
> > diff --git a/kvm-all.c b/kvm-all.c
> > index cbc2532..35d698b 100644
> > --- a/kvm-all.c
> > +++ b/kvm-all.c
> > @@ -713,7 +713,6 @@ int kvm_init(void)
> >      s->vmfd = -1;
> >      s->fd = qemu_open("/dev/kvm", O_RDWR);
> >      if (s->fd == -1) {
> > -        fprintf(stderr, "Could not access KVM kernel module: %m\n");
> 
> Don't do this.  qemu error reporting sucks badly already,
> and this very error message gives us at least some details
> (the %m part) about what's missing.  There's no other place
> where this information is printed in case this open fails.
> We already have to resort to strace in way too many places... :(

This is an argument for having some sort of global --debug option.
Silent by default, but debug messages are a mere runtime flag away.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v

Re: [Qemu-devel] [PATCH] Remove debugging messages.

[Richard W.M. Jones]

On Tue, Jul 19, 2011 at 03:04:22PM -0500, Anthony Liguori wrote:
> On 07/19/2011 02:23 PM, Richard W.M. Jones wrote:
> >From: "Richard W.M. Jones"<rjones@redhat.com>
> >
> >When using qemu -machine accel=kvm:tcg, if KVM is not available you
> >get a load of debugging output:
> >
> >   Could not access KVM kernel module: No such file or directory
> >   failed to initialize KVM: No such file or directory
> >   Back to tcg accelerator.
> 
> If you ask for kvm, and it can't be initialized, then shouldn't we
> alert the user about this?

That's one way to look at it.

Another is that libguestfs has specified that we want a "best effort".
Give me KVM, if available - great!  If not, I don't really care, give
me TCG.

The -machine option isn't expressive enough to say all these things of
course.

BTW, it seems in 0.15 that the -machine option is now required?  eg in
this case where it seems totally pointless:

$ qemu -nographic -device \?
KVM not supported for this target
No accelerator found!

This resulted in another bug and fix in libguestfs.

http://git.annexia.org/?p=libguestfs.git;a=commitdiff;h=d82438431c1551610eb7d9945fa76d6387534582

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Re: [Qemu-devel] [PATCH] Remove debugging messages.

[Anthony Liguori]

On 07/19/2011 03:59 PM, Richard W.M. Jones wrote:
> On Tue, Jul 19, 2011 at 03:04:22PM -0500, Anthony Liguori wrote:
>> On 07/19/2011 02:23 PM, Richard W.M. Jones wrote:
>>> From: "Richard W.M. Jones"<rjones@redhat.com>
>>>
>>> When using qemu -machine accel=kvm:tcg, if KVM is not available you
>>> get a load of debugging output:
>>>
>>>    Could not access KVM kernel module: No such file or directory
>>>    failed to initialize KVM: No such file or directory
>>>    Back to tcg accelerator.
>>
>> If you ask for kvm, and it can't be initialized, then shouldn't we
>> alert the user about this?
>
> That's one way to look at it.
>
> Another is that libguestfs has specified that we want a "best effort".
> Give me KVM, if available - great!  If not, I don't really care, give
> me TCG.

Can't libguestfs just ignore the messages?

>
> The -machine option isn't expressive enough to say all these things of
> course.
>
> BTW, it seems in 0.15 that the -machine option is now required?  eg in
> this case where it seems totally pointless:
>
> $ qemu -nographic -device \?
> KVM not supported for this target
> No accelerator found!

Hrm, what's your HEAD because mine doesn't exhibit this behavior.

Regards,

Anthony Liguori

>
> This resulted in another bug and fix in libguestfs.
>
> http://git.annexia.org/?p=libguestfs.git;a=commitdiff;h=d82438431c1551610eb7d9945fa76d6387534582
>
> Rich.
>

Re: [Qemu-devel] [PATCH] Remove debugging messages.

[Richard W.M. Jones]

On Tue, Jul 19, 2011 at 04:56:24PM -0500, Anthony Liguori wrote:
> Can't libguestfs just ignore the messages?

It does, but they get printed to stderr which confuses users and has
caused several bug reports in the past.  We'll probably have to
redirect them somewhere.

> >$ qemu -nographic -device \?
> >KVM not supported for this target
> >No accelerator found!
> 
> Hrm, what's your HEAD because mine doesn't exhibit this behavior.

Right.  This seems to be a problem specific to Fedora's package.  I
also can't reproduce it with upstream.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://et.redhat.com/~rjones/virt-df/

Re: [Qemu-devel] [PATCH] Remove debugging messages.

[Anthony Liguori]

On 07/20/2011 03:50 AM, Richard W.M. Jones wrote:
> On Tue, Jul 19, 2011 at 04:56:24PM -0500, Anthony Liguori wrote:
>> Can't libguestfs just ignore the messages?
>
> It does, but they get printed to stderr which confuses users and has
> caused several bug reports in the past.  We'll probably have to
> redirect them somewhere.

Given the large number of reports we've gotten in the past where poor 
performance was caused by the expectation that a user had KVM enabled 
and really didn't, I think it's more important to overcommunicate to the 
user here.

If in the case of libguestfs, you think this isn't something a user 
should worry about, you can just filter out the message before it hits 
stderr.

Regards,

Anthony Liguori

>
>>> $ qemu -nographic -device \?
>>> KVM not supported for this target
>>> No accelerator found!
>>
>> Hrm, what's your HEAD because mine doesn't exhibit this behavior.
>
> Right.  This seems to be a problem specific to Fedora's package.  I
> also can't reproduce it with upstream.
>
> Rich.
>

Re: [Qemu-devel] [PATCH] Remove debugging messages.

[Richard W.M. Jones]

On Wed, Jul 20, 2011 at 07:51:29AM -0500, Anthony Liguori wrote:
> Given the large number of reports we've gotten in the past where
> poor performance was caused by the expectation that a user had KVM
> enabled and really didn't, I think it's more important to
> overcommunicate to the user here.
> 
> If in the case of libguestfs, you think this isn't something a user
> should worry about, you can just filter out the message before it
> hits stderr.

That's what we're now doing.

But what's the rationale here?  For instance, qemu 0.15 is also
printing:

  qemu: terminating on signal 15 from pid 9617

which I guess is interesting, but unlikely to be useful for users.

Is qemu now noisy by default?  Is any old debug message that a user
might find interesting fair game?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top