[libvirt] [PATCH 05/13] Convert (nearly) all files in src/util/ to use virReportError()

[Eric Blake]

On 07/18/2012 05:52 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange at redhat.com>
> 
> This removes nearly all the per-file error reporting macros
> from the code in src/util/. A few custom macros remain for the
> case, where the file needs to report errors with a variety of
> different codes or parametes

s/parametes/parameters/

I'm only doing a "positive review" (did the changes you make look
right?), and skipping the "completeness review" (did you miss an
opportunity for even more changes that should be made at the same
time?).  That means I'm probably not spotting the custom macros that you
didn't convert, but that shouldn't stop you from applying this.

> @@ -661,7 +657,7 @@ virExecWithHook(const char *const*argv,
>      if (binary != argv[0])
>          VIR_FREE(binary);
>  
> -    /* NB we don't virCommandError() on any failures here
> +    /* NB we don't virReportError() on any failures here
>         because the code which jumped hre already raised

As long as you are touching this, s/hre/here/

> +++ b/src/util/hooks.c

> @@ -252,9 +248,9 @@ virHookCall(int driver,
>              break;
>      }
>      if (opstr == NULL) {
> -        virHookReportError(VIR_ERR_INTERNAL_ERROR,
> -                           _("Hook for %s, failed to find operation #%d"),
> -                           drvstr, op);
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Hook for %s, failed to find operation #%d"),

Generic question - now that we are touching 90% of the existing error
messages, should we establish a convention on whether all messages
should start with a lower case letter?  (That would at least make us
consistent with GNU Coding Standards, and while we are not a GNU
project, cross-project consistency does have an advantage.  Personally,
I've noticed our inconsistent use of capitals for more than 2 years now,
but it hasn't been high enough on my "itch radar" to scratch it.)
Obviously, a syntax check would be needed IF we make a decision to
standardize.

> +++ b/src/util/json.c
> @@ -43,9 +43,6 @@
>  
>  /* XXX fixme */
>  #define VIR_FROM_THIS VIR_FROM_NONE
> -#define virJSONError(code, ...)                                         \

I think the 'fixme' comment is no longer relevant, and should be nuked.

> +++ b/src/util/pci.c
> @@ -744,7 +740,7 @@ pciResetDevice(pciDevice *dev,
>      int ret = -1;
>  
>      if (activeDevs && pciDeviceListFind(activeDevs, dev)) {
> -        pciReportError(VIR_ERR_INTERNAL_ERROR,
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("Not resetting active device %s"), dev->name);

Code like this means we will probably see a second round of patches in
the future for scrubbing stupid uses of VIR_ERR_INTERNAL_ERROR into
better messages for user-visible errors :)

> +++ b/src/util/stats_linux.c
> @@ -106,8 +102,8 @@ linuxDomainInterfaceStats(const char *path,
>      }
>      VIR_FORCE_FCLOSE(fp);
>  
> -    virStatsError(VIR_ERR_INTERNAL_ERROR,
> -                  _("/proc/net/dev: Interface not found"));
> +    virReportError(VIR_ERR_INTERNAL_ERROR,
> +                   _("/proc/net/dev: Interface not found"));

Needs a "%s" argument to silence compiler warnings when i18n is disabled.

> +++ b/src/util/util.c
> @@ -333,8 +329,8 @@ virPipeReadUntilEOF(int outfd, int errfd,
>                  if (fds[i].revents & POLLHUP)
>                      continue;
>  
> -                virUtilError(VIR_ERR_INTERNAL_ERROR,
> -                             "%s", _("Unknown poll response."));
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               "%s", _("Unknown poll response."));

Is it worth scrubbing for error messages that end with '.'?  That's
another thing that GNU Coding Standards discourage.

ACK.  Either squash in the nits I pointed out, or we can do that as a
separate patch to keep this one mechanical.  As for consensus on whether
to standardize error message conventions, does anyone else have an opinion?

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120718/07e11b57/attachment-0001.sig>