Re: [libvirt] [PATCH] Improve error reporting in virsh

On Thu, Feb 05, 2009 at 05:34:20PM +0000, Daniel P. Berrange wrote:

> Just have the virXendError macro call virReportErrorHelper directly, > as it already does - there is no need to wrap it in the conditional > check 'if(virGetLastError() == NULL)' I'm trying to connect this comment to your other ones, but can't, so I'll have to attempt to guess what you're saying :) I think you're just NAKing the entire change, with your reasoning being that the code must be fixed to only set an error once. Unfortunately fixing code your way seems close to impossible. To give just one example, xenDaemonFormatSxprNet(): 5335 virXendError(conn, VIR_ERR_INTERNAL_ERROR, 5336 _("unsupported network type %d"), def->type); This part of it is "lowest level", so should always report an error. but it also calls virNetworkGetBridgeName() which *isn't*. This CAN report an error, but we immediately over-write it: 5374 bridge = virNetworkGetBridgeName(network); 5375 virNetworkFree(network); 5376 if (!bridge) { 5377 virXendError(conn, VIR_ERR_NO_SOURCE, "%s", 5378 def->data.network.name); Also, there's at least two call stacks that reach this point: xenDaemonDomainDefineXML() xenDaemonFormatSxpr() xenDaemonFormatSxprNet() xenDaemonAttachDevice() xenDaemonFormatSxprNet() In the second case, we are OK. In the first, we're already over-writing the error from xenDaemonFormatSxprNet(). Or for another example, it took just a couple of seconds looking at the xenDaemonDomainDefineXML() code paths to find one where virDomainDefParseString() would not set an error. I do not think it's really possible to get things right the way you're suggesting. The fact that the code is broken everywhere backs me up pretty strongly here. How do you propose to reasonably test such changes? In contrast, "only set an error if one isn't set already" is simple, immediately understandable, and covers almost all the common cases.