[libvirt] [PATCH] Unify *ReportError logic

[Daniel P. Berrange]

On Thu, Oct 02, 2008 at 04:12:10PM -0400, Cole Robinson wrote:
> Daniel P. Berrange wrote:
> > On Thu, Oct 02, 2008 at 03:41:21PM -0400, Cole Robinson wrote:
> >> Currently, most src/* files have their own ReportError
> >> function. Some support printf style arguments, others
> >> only allow reporting a single string message. The code
> >> for all of them does virtually the same thing, possibly
> >> passing a different constant off to another function.
> >>
> >> The attached patch adds a function to virterror.c which
> >> encapsulates the common ReportError logic. I used this
> >> to replace qemudReportError with a macro, which also 
> >> allows passing off filename and line number info if
> >> we wanted to do something with it later.
> >>
> >> I did just the one function conversion to see what
> >> people think: if I'm missing something, or ideas for
> >> anything else to add. Seems to work as expected in
> >> my testing.
> > 
> > 
> > Basically a good idea - we've discussed doing exactly this in the past.
> > You can do further though, and kill off the 'dom' and 'net' parameters
> > here too. Those are deprecated and should always be left NULL these
> > days. 
> 
> Sounds good.

Actually, lets go one step further than this. Just change the signature
of virRaiseError itself, rather than creating a virRaiseErrorHelper.
No caller in libvirt uses all these parameter it has, so might as well
just remove them and have it set those fields to NULL/0 as directly.

> > Likewise passing the filename/linenr is not much use - if a
> > particular error message wants to include file/line number then they
> > can be includes as args to the printf fmt string.
> 
> Hmm, I kind of like the idea of having this info by default,
> rather than offloading it to the error message. We could
> for example append filename:lineno to all error messages if
> debugging is enabled, or stick the data as extra string or
> int info to RaiseError.
> 
> I know there have been times when, using virt-manager/
> virtinst, libvirt prints some generic lookup error msg,
> yet nothing seems to fail. It would be nice to get a
> definitive line number of the culprit just by using
> LIBVIRT_DEBUG.

That's an interesting idea - I can't remember offhand though whether the
__FILE__ expands to the full file path, or just the final filename without
directory separator. I'd only want this compiled in by default if it is
the latter - we don't want another 500 K of long strings from full directory
paths in every  build.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|