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