From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from [140.186.70.92] (port=49608 helo=eggs.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1PwdTR-0006Qi-5R
	for qemu-devel@nongnu.org; Mon, 07 Mar 2011 11:39:58 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <Jes.Sorensen@redhat.com>) id 1PwdTP-0001r5-NN
	for qemu-devel@nongnu.org; Mon, 07 Mar 2011 11:39:57 -0500
Received: from mx1.redhat.com ([209.132.183.28]:21834)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <Jes.Sorensen@redhat.com>) id 1PwdTP-0001qf-Ex
	for qemu-devel@nongnu.org; Mon, 07 Mar 2011 11:39:55 -0500
Message-ID: <4D750A4B.1070304@redhat.com>
Date: Mon, 07 Mar 2011 17:39:39 +0100
From: Jes Sorensen <Jes.Sorensen@redhat.com>
MIME-Version: 1.0
Subject: Re: [Qemu-devel] [PATCH v3] Improve error handling in
	do_snapshot_blkdev()
References: <1299511653-11357-1-git-send-email-Jes.Sorensen@redhat.com>
	<4D75092F.1020107@codemonkey.ws>
In-Reply-To: <4D75092F.1020107@codemonkey.ws>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/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: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Anthony Liguori <anthony@codemonkey.ws>
Cc: kwolf@redhat.com, qemu-devel@nongnu.org, stefanha@linux.vnet.ibm.com

On 03/07/11 17:34, Anthony Liguori wrote:
> On 03/07/2011 09:27 AM, Jes.Sorensen@redhat.com wrote:
>>       if (ret != 0) {
>> -        abort();
>> +        qerror_report(QERR_OPEN_FILE_FAILED, filename);
>> +        error_printf("do_snapshot_blkdev(): Unable to open newly
>> created "
>> +                     "snapshot file: \n");
>> +        error_printf("%s. Attempting to revert to original image: %s\n",
>> +                     filename, old_filename);
> 
> You can't combine qerror_report with continued action.  qerror_report()
> should be a terminal action.  You also shouldn't combine error_printf()
> with qerror_report().
> 
> You should restore the original image file before doing qerror_report()
> and just drop the error_printf()s as it's all redundant information.

I would hardly consider it redundant information that it failed and we
try to go back to the original image. That is an error in itself, even
though rolling back is better than abort()ing.

If qerror_report() is a fatal situation, that is problematic. Then we
need qerror_warn() or something as well, which can return non fatal
information.

The printfs are very valuable for the human monitor, but it isn't really
clear to me what is the ideal return value.

Cheers,
Jes