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 -- Libvir-list mailing list Libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list