[libvirt] [PATCH] Unify *ReportError logic

[Cole Robinson]

Daniel P. Berrange wrote:
> On Thu, Oct 02, 2008 at 04:12:10PM -0400, Cole Robinson wrote:
>> Daniel P. Berrange wrote:
>>>
>>> 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.
> 

I held off on doing this for my latest version of the patch.
Some files call RaiseError directly so changing the arguments
will be some extra work. There are a few places a couldn't
quickly remove the custom error functions either, so I'll
send a later patch that cleans up the sticky situations.

For now, I think using the helper is okay, it will be easy
to swap out for an updated __virRaiseError at a later time.

>> 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

A test program and 'strings' indicates that only the
file basename is stored, so it shouldn't be a problem.

I'm about to post a complete version of this patch,
fyi.

Thanks,
Cole