[PATCH 07/25] util: eliminate error label in virDomainDefFormatInternalSetRootName()

Laine Stump laine at redhat.com
Thu Jun 25 23:38:35 UTC 2020


On 6/25/20 7:08 PM, Ján Tomko wrote:
> On a Wednesday in 2020, Laine Stump wrote:
>> The only reason for the error label in this function is to call
>> virBufferFreeAndReset(). It's actually more common for a failed format
>> function to just leave the virBuffer alone and let the caller free it
>> when there is a failure, and in fact the only caller of this function
>> that *wasn't* already calling virBufferFreeAndReset() on failure was
>> virDomainDefFormat() (via virDomainDefFormatInternal()).
>>
>
> qemuDomainDefFormatXMLInternal does not call it either.


Dang! I thought I had followed every call chain with cscope, but maybe I 
just searched in this one file? Anyway, it's especially embarrassing 
because not only did I miss qemuDomainFormatXMLInternal(), I also missed 
virDomainSnapshotDefFormat (which called 
virDomainSnapshotDefFormatInternal(), which calls 
virDomainDefFormatInternal()) :-(


I think as a followup patch, I should convert every occurrence of 
"virBuffer blah = VIR_BUFFER_INITIALIZER" to "g_auto(virBuffer) blah = 
VIR_BUFFER_INITIALIZER" - in a quick search just now I already found a 
couple more (totally unrelated to virDomainDefFormat) that aren't 
properly cleared out on error.


Thanks for taking the time to actually fact check my claims.


#FakeCommitLogs


>
>> That is easily solved by modifying virDomainDefFormat() to declare its
>> virBuffer buf with g_auto(), so that virBufferFreeAndReset() is
>> automatically called.
>>
>> Signed-off-by: Laine Stump <laine at redhat.com>
>> ---
>> src/conf/domain_conf.c | 88 ++++++++++++++++++++----------------------
>> 1 file changed, 42 insertions(+), 46 deletions(-)
>
> With that fixed:
> Reviewed-by: Ján Tomko <jtomko at redhat.com>
>
> Jano





More information about the libvir-list mailing list