qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Eric Blake <eblake@redhat.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	qemu-devel@nongnu.org
Cc: pbonzini@redhat.com, qemu-block@nongnu.org,
	Kevin Wolf <kwolf@redhat.com>, Max Reitz <mreitz@redhat.com>
Subject: Re: [Qemu-devel] [RFC PATCH v6 13/12] tweak test 83 verbosity
Date: Mon, 30 Oct 2017 14:00:40 +0100	[thread overview]
Message-ID: <d05bb732-2bfa-4df7-ce87-9d2985792c55@redhat.com> (raw)
In-Reply-To: <87d8006a-f345-452c-033d-1769982395bc@virtuozzo.com>

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

On 10/30/2017 01:34 PM, Vladimir Sementsov-Ogievskiy wrote:
> 27.10.2017 13:45, Eric Blake wrote:
>> Commenting these two lines is enough to avoid the change to 083.out
>> in 12/12.  That is evidence that we may want these two lines to be
>> trace points rather than error messages; or maybe we really do like
>> the extra verbosity in the case of an unexpected communication break.
>>
>> This patch does not meet coding guidelines, and I'm not proud enough
>> of it to give S-o-b, but I'm posting it for conversation.
>>

>>       if (ret < 0) {
>> -        error_report_err(local_err);
>> +        //error_report_err(local_err);
>>       }
>>       return ret;
>>   }
> 
> commenting is not ok, you should call error_free instead.

Indeed, and that's why I marked this RFC and used the wrong style
comment, to make it obvious that if we aren't going to print an error
message, there are better ways to go about doing that which don't leak
memory.  The question is whether the output is useful, or whether it
should be only via a trace (as it is, we aren't yet tracing anything in
block/nbd-client.c), or whether it is always just extra noise (in which
case passing the errp parameter around isn't helpful).


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

  reply	other threads:[~2017-10-30 13:01 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-10-27 10:40 [Qemu-devel] [PATCH v6 00/12] nbd minimal structured read Eric Blake
2017-10-27 10:40 ` [Qemu-devel] [PATCH v6 01/12] nbd: Include error names in trace messages Eric Blake
2017-10-27 10:40 ` [Qemu-devel] [PATCH v6 02/12] nbd: Move nbd_errno_to_system_errno() to public header Eric Blake
2017-10-27 10:40 ` [Qemu-devel] [PATCH v6 03/12] nbd: Expose constants and structs for structured read Eric Blake
2017-10-27 10:40 ` [Qemu-devel] [PATCH v6 04/12] nbd/server: Report error for write to read-only export Eric Blake
2017-10-27 10:40 ` [Qemu-devel] [PATCH v6 05/12] nbd/server: Simplify nbd_negotiate_options loop Eric Blake
2017-10-30 16:43   ` Vladimir Sementsov-Ogievskiy
2017-10-27 10:40 ` [Qemu-devel] [PATCH v6 06/12] nbd/server: Refactor zero-length option check Eric Blake
2017-10-30 17:22   ` Vladimir Sementsov-Ogievskiy
2017-10-30 20:11     ` Eric Blake
2017-10-30 20:46       ` Eric Blake
2017-10-27 10:40 ` [Qemu-devel] [PATCH v6 07/12] nbd: Minimal structured read for server Eric Blake
2017-10-30 17:30   ` Vladimir Sementsov-Ogievskiy
2017-10-27 10:40 ` [Qemu-devel] [PATCH v6 08/12] nbd/server: Include human-readable message in structured errors Eric Blake
2017-10-27 10:40 ` [Qemu-devel] [PATCH v6 09/12] nbd/client: refactor nbd_receive_starttls Eric Blake
2017-10-30 17:32   ` Vladimir Sementsov-Ogievskiy
2017-10-27 10:40 ` [Qemu-devel] [PATCH v6 10/12] nbd/client: prepare nbd_receive_reply for structured reply Eric Blake
2017-11-13 19:33   ` Eric Blake
2017-10-27 10:40 ` [Qemu-devel] [PATCH v6 11/12] nbd: Move nbd_read() to common header Eric Blake
2017-10-27 10:40 ` [Qemu-devel] [PATCH v6 12/12] nbd: Minimal structured read for client Eric Blake
2017-10-30 17:55   ` Vladimir Sementsov-Ogievskiy
2017-10-27 10:45 ` [Qemu-devel] [RFC PATCH v6 13/12] tweak test 83 verbosity Eric Blake
2017-10-30 12:34   ` Vladimir Sementsov-Ogievskiy
2017-10-30 13:00     ` Eric Blake [this message]
2017-10-30 18:01   ` Vladimir Sementsov-Ogievskiy
2017-10-27 10:59 ` [Qemu-devel] [PATCH v6 00/12] nbd minimal structured read no-reply
2017-10-27 14:59   ` 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=d05bb732-2bfa-4df7-ce87-9d2985792c55@redhat.com \
    --to=eblake@redhat.com \
    --cc=kwolf@redhat.com \
    --cc=mreitz@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=vsementsov@virtuozzo.com \
    /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).