From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:42787)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <berrange@redhat.com>) id 1SuQfl-00040Q-W2
	for qemu-devel@nongnu.org; Thu, 26 Jul 2012 12:12:27 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <berrange@redhat.com>) id 1SuQfi-00029y-IJ
	for qemu-devel@nongnu.org; Thu, 26 Jul 2012 12:12:21 -0400
Received: from mx1.redhat.com ([209.132.183.28]:37976)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <berrange@redhat.com>) id 1SuQfi-000290-9I
	for qemu-devel@nongnu.org; Thu, 26 Jul 2012 12:12:18 -0400
Date: Thu, 26 Jul 2012 17:12:12 +0100
From: "Daniel P. Berrange" <berrange@redhat.com>
Message-ID: <20120726161212.GL12180@redhat.com>
References: <1343249431-9245-1-git-send-email-lcapitulino@redhat.com>
	<87boj3dyxg.fsf@codemonkey.ws>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <87boj3dyxg.fsf@codemonkey.ws>
Subject: Re: [Qemu-devel] [RFC 00/14]: add printf-like human msg to
 error_set()
Reply-To: "Daniel P. Berrange" <berrange@redhat.com>
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: Anthony Liguori <aliguori@us.ibm.com>
Cc: peter.maydell@linaro.org, qemu-devel@nongnu.org, armbru@redhat.com, pbonzini@redhat.com, Luiz Capitulino <lcapitulino@redhat.com>, afaerber@suse.de

On Wed, Jul 25, 2012 at 09:43:55PM -0500, Anthony Liguori wrote:
> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > Basically, this series changes a call like:
> >
> >  error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> >
> > to:
> >
> >  error_set(errp, QERR_DEVICE_NOT_FOUND,
> >            "Device 'device=%s' not found", device);
> >
> > In the first call, QERR_DEVICE_NOT_FOUND is a string containing a json dict:
> >
> >     "{ 'class': 'DeviceNotFound', 'data': { 'device': %s } }"
> 
> This is the wrong direction.  Looking through the patch, this makes the
> code much more redundant overall.  You have dozens of calls that are
> duplicating the same error message.  This is not progress.

What we do in libvirt, is to define helper functions for
reporting the specific error codes. So, as well as having
the generic

  error_set(errp, QERR_CODE, "format string", args)

you would have

  error_set_device_not_found(errp, args)

which is just a #define

  #define error_set_device_not_found(errp, args) \
     error_set(errp, QERR_DEVICE_NOT_FOUND, "Device %s not found", args)

for the most part this should avoid the duplication you're concerned
about, while still letting use provided more detailed messsages.


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|