[libvirt] [PATCH] Unify *ReportError logic
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. Thanks, Cole
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. 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.
+ +/** + * __virReportErrorHelper + * + * @conn: the connection to the hypervisor if available + * @dom: the domain if available + * @net: the network if available + * @domcode: the virErrorDomain indicating where it's coming from + * @errcode: the virErrorNumber code for the error + * @filename: Source file error is dispatched from + * @linenr: Line number error is dispatched from + * @msg: the message to display/transmit + * @...: extra parameters for the message display + * + * Helper function to do most of the grunt work for individual driver ReportError + */ +void __virReportErrorHelper(virConnectPtr conn, virDomainPtr dom, virNetworkPtr net, + int domcode, int errcode, + const char *filename ATTRIBUTE_UNUSED, + long long linenr ATTRIBUTE_UNUSED, + const char *fmt, ...)
So I think this would be sufficient: void __virReportErrorHelper(virConnectPtr conn, int domcode, int errcode, const char *fmt, ...)
+{ + va_list args; + char errorMessage[1024]; + const char *virerr; + + if (fmt) { + va_start(args, fmt); + vsnprintf(errorMessage, sizeof(errorMessage)-1, fmt, args); + va_end(args); + } else { + errorMessage[0] = '\0'; + } + + virerr = __virErrorMsg(errcode, (errorMessage[0] ? errorMessage : NULL)); + __virRaiseError(conn, dom, net, domcode, errcode, VIR_ERR_ERROR, + virerr, errorMessage, NULL, -1, -1, virerr, errorMessage); + +}
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 :|
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.
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. Thanks, Cole
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 :|
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
On Thu, Oct 02, 2008 at 08:53:12PM +0100, 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.
i'm still a bit sad about this though, I'm still left with the feeling that being unable to refcount, result in a loss of useful informations. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (3)
-
Cole Robinson -
Daniel P. Berrange -
Daniel Veillard