From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:45486)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <pbonzini@redhat.com>) id 1TEFDN-0001VG-4s
	for qemu-devel@nongnu.org; Wed, 19 Sep 2012 04:01:03 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <pbonzini@redhat.com>) id 1TEFDH-0001G7-AN
	for qemu-devel@nongnu.org; Wed, 19 Sep 2012 04:00:57 -0400
Received: from mx1.redhat.com ([209.132.183.28]:15498)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <pbonzini@redhat.com>) id 1TEFDG-0001G2-VE
	for qemu-devel@nongnu.org; Wed, 19 Sep 2012 04:00:51 -0400
Message-ID: <50597BAE.4030208@redhat.com>
Date: Wed, 19 Sep 2012 10:00:46 +0200
From: Paolo Bonzini <pbonzini@redhat.com>
MIME-Version: 1.0
References: <1346079626-16386-1-git-send-email-pbonzini@redhat.com>
	<1346079626-16386-12-git-send-email-pbonzini@redhat.com>
	<20120918172211.38ee17d2@doriath.home>
In-Reply-To: <20120918172211.38ee17d2@doriath.home>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [RFC PATCH 11/13] hmp: add NBD server commands
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Luiz Capitulino <lcapitulino@redhat.com>
Cc: kwolf@redhat.com, stefanha@gmail.com, qemu-devel@nongnu.org

Il 18/09/2012 22:22, Luiz Capitulino ha scritto:
>> > +    if (addr.host == NULL || addr.port == NULL) {
>> > +        error_set(&errp, QERR_SOCKET_CREATE_FAILED);
> You can and should use monitor_printf() directly.

Hmm, why? Then I would have two error paths, one with monitor_printf and
one with hmp_handle_error.  But I will change this to add an Error* to
inet_parse.

>> > +        goto exit;
>> > +    }
>> > +
>> > +    qmp_nbd_server_start(&addr, &errp);
>> > +    if (errp != NULL) {
> Please, use error_is_set() instead.

I'll just rename errp to local_err, which is the usual convention.

>> > +        goto exit;
>> > +    }
>> > +
>> > +    /* Then try adding all block devices.  If one fails, close all and
>> > +     * exit.
>> > +     */
>> > +    bs = NULL;
>> > +    while ((bs = bdrv_next(bs))) {
> To keep hmp.c an honest qmp client, you should use qmp_query_block() instead
> of looping through BSs directly. You'll find other examples if you find for
> qmp_query_block() in this file.

Right, thanks!

Paolo