On Wed, Jul 18, 2012 at 07:54:11AM -0600, Eric Blake wrote:
> > +++ 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.
Yeah sounds like a reasonble idea to fix this.
> > +++ 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 :)
Arugably nearly every use of VIR_ERR_INTERNAL_ERROR ought to
be replaced by something more meaningful. That's not a job I
wish to tackle now :-)
>
> > +++ 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.
I'm fixing this as a separate patch, since this was a pre-existing
problem
>
> > +++ 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.
Something to fix at the same time as the capitalization
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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list