qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Eric Blake <eblake@redhat.com>
To: Markus Armbruster <armbru@redhat.com>, qemu-devel@nongnu.org
Cc: pbonzini@redhat.com, alex.bennee@linaro.org
Subject: Re: [Qemu-devel] [PATCH 2/3] log: Fix qemu_set_dfilter_ranges() error reporting
Date: Wed, 15 Jun 2016 13:06:05 -0600	[thread overview]
Message-ID: <5761A71D.8070003@redhat.com> (raw)
In-Reply-To: <1466011636-6112-3-git-send-email-armbru@redhat.com>

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

On 06/15/2016 11:27 AM, Markus Armbruster wrote:
> g_error() is not an acceptable way to report errors to the user:
> 
>     $ qemu-system-x86_64 -dfilter 1000+0
> 
>     ** (process:17187): ERROR **: Failed to parse range in: 1000+0
>     Trace/breakpoint trap (core dumped)
> 
> g_assert() isn't, either:
> 
>     $ qemu-system-x86_64 -dfilter 1000x+64
>     **
>     ERROR:/work/armbru/qemu/util/log.c:180:qemu_set_dfilter_ranges: assertion failed: (e == range_op)
>     Aborted (core dumped)

I see you're trying to improve my range.h patches, and got dragged into
this stuff.

> 
> Convert qemu_set_dfilter_ranges() to Error.  Rework its deeply nested
> control flow.  Touch up the error messages.  Call it with
> &error_fatal.
> 
> This also permits testing without a subprocess, so do that.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/qemu/log.h   |   2 +-
>  tests/test-logging.c |  49 ++++++++--------------
>  util/log.c           | 113 ++++++++++++++++++++++++++-------------------------
>  vl.c                 |   2 +-
>  4 files changed, 75 insertions(+), 91 deletions(-)
> 

> +    qemu_set_dfilter_ranges("0x1000+onehundred", &err);
> +    error_free_or_abort(&err);
> +
> +    qemu_set_dfilter_ranges("0x1000+0", &err);
> +    error_free_or_abort(&err);
>  }
>  

Maybe also worth testing "0x" and "0x1000+0x" for being invalid?

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

  reply	other threads:[~2016-06-15 19:06 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-06-15 17:27 [Qemu-devel] [PATCH 0/3] log: Fix error handling and a memory leak Markus Armbruster
2016-06-15 17:27 ` [Qemu-devel] [PATCH 1/3] log: Plug memory leak on multiple -dfilter Markus Armbruster
2016-06-15 18:42   ` Eric Blake
2016-06-15 17:27 ` [Qemu-devel] [PATCH 2/3] log: Fix qemu_set_dfilter_ranges() error reporting Markus Armbruster
2016-06-15 19:06   ` Eric Blake [this message]
2016-06-16  7:40     ` Markus Armbruster
2016-06-15 17:27 ` [Qemu-devel] [PATCH 3/3] log: Fix qemu_set_log_filename() error handling Markus Armbruster
2016-06-15 19:11   ` Eric Blake

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5761A71D.8070003@redhat.com \
    --to=eblake@redhat.com \
    --cc=alex.bennee@linaro.org \
    --cc=armbru@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).