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