Re: [PATCH 07/25] util: eliminate error label in virDomainDefFormatInternalSetRootName()
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(a)redhat.com>
> ---
> src/conf/domain_conf.c | 88 ++++++++++++++++++++----------------------
> 1 file changed, 42 insertions(+), 46 deletions(-)
With that fixed:
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano