From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:47957) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TQbPx-0003Lv-03 for qemu-devel@nongnu.org; Tue, 23 Oct 2012 06:09:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TQbPv-0007y6-Rc for qemu-devel@nongnu.org; Tue, 23 Oct 2012 06:09:00 -0400 Received: from mx1.redhat.com ([209.132.183.28]:26228) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TQbPv-0007xw-Id for qemu-devel@nongnu.org; Tue, 23 Oct 2012 06:08:59 -0400 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q9NA8wM7016386 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 23 Oct 2012 06:08:58 -0400 Message-ID: <50866CB8.7060304@redhat.com> Date: Tue, 23 Oct 2012 12:08:56 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <1350502556-4885-1-git-send-email-lcapitulino@redhat.com> <1350502556-4885-6-git-send-email-lcapitulino@redhat.com> <507FF1FC.1060502@redhat.com> <20121018104941.486f1404@doriath.home> <50800BEE.9010006@redhat.com> <877gqh1pdq.fsf@blackfin.pond.sub.org> In-Reply-To: <877gqh1pdq.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 5/8] qemu-img: img_create(): use Error object List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, Luiz Capitulino Am 23.10.2012 11:58, schrieb Markus Armbruster: > Kevin Wolf writes: > >> Am 18.10.2012 15:49, schrieb Luiz Capitulino: >>> On Thu, 18 Oct 2012 14:11:40 +0200 >>> Kevin Wolf wrote: >>> >>>> Am 17.10.2012 21:35, schrieb Luiz Capitulino: >>>>> Signed-off-by: Luiz Capitulino >>>>> --- >>>>> qemu-img.c | 10 +++++++--- >>>>> 1 file changed, 7 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/qemu-img.c b/qemu-img.c >>>>> index 12fb6c2..dfde588 100644 >>>>> --- a/qemu-img.c >>>>> +++ b/qemu-img.c >>>>> @@ -302,6 +302,7 @@ static int img_create(int argc, char **argv) >>>>> const char *base_filename = NULL; >>>>> char *options = NULL; >>>>> QEMUOptionParameter *params = NULL; >>>>> + Error *local_err = NULL; >>>>> >>>>> for(;;) { >>>>> c = getopt(argc, argv, "F:b:f:he6o:"); >>>>> @@ -362,9 +363,12 @@ static int img_create(int argc, char **argv) >>>>> goto out; >>>>> } >>>>> >>>>> - ret = bdrv_img_create(filename, fmt, base_filename, base_fmt, >>>>> - options, img_size, BDRV_O_FLAGS, ¶ms, NULL); >>>>> - if (ret < 0) { >>>>> + bdrv_img_create(filename, fmt, base_filename, base_fmt, >>>>> + options, img_size, BDRV_O_FLAGS, ¶ms, &local_err); >>>>> + if (error_is_set(&local_err)) { >>>>> + fprintf(stderr, "qemu-img: %s\n", error_get_pretty(local_err)); >>>> >>>> This should use error_report() instead of adding the "qemu-img:" manually. >>> >>> If you want to do it for consistency with the current code then ok, >>> I'll do it for v2. But I disagree qemu-img should keep using error_report(), >>> printing to hmp for example is something beyond the interests of a tool like >>> qemu-img. >> >> qemu-img doesn't have an HMP monitor, so it doesn't hurt either. If you >> want to replace it, replace it with a copy of qemu-error.c that only >> removes the monitor_vprintf() case. That is, in particular, leave all of >> the loc_*() functionality there, because this is something that is meant >> for use in command line parsers. > > Strongly agreed on use of error_report(). Buys us uniform error > messages that can point to the location causing the error. The fact > that error_report() does the right thing in monitor context is detail. > > I don't see why purging a few monitor references from tools is worth > copying the file. Stubbing out cur_mon and monitor_vprintf() in tools > is just fine, in my opinion. Purging monitor references from the code isn't worth it. Becoming independent from functions also used by the monitor, which keep being subject of heated discussions, might be worth it, because I don't feel like joining these discussions more often than absolutely necessary. It was just my offer in case Luiz insists on getting rid of error_report() in the tools, not something I'd advocate myself. Kevin