-----Original Message----- From: Martin Kletzander [mailto:mkletzan@redhat.com] Sent: Wednesday, January 15, 2014 2:36 PM To: Chen Hanxiao Cc: libvir-list@redhat.com Subject: Re: [libvirt] [PATCH] lxc: do cleanup when failed to create new string
if (virAsprintf(&logfile, "%s/%s.log", cfg->logDir, vm->def->name) < 0) - return -1; + goto cleanup;
I see nothing in that cleanup that needs to be done in this codepath. The only thing which might be needed is cleaning up the vm->def->resource, but that doesn't make much sense to me. Can you explain the change?
But the code structure looks weird. We have already used 'cleanup' before this section.
I'd rather see either a) the two previous gotos changed into returns (which means using a cleanup only where it needs to be done) or b) using cleanups everywhere, but this doesn't go with what we use elsewhere and the cleanup in this section is *really* huge, so it's unnecessary to go there, I guess.
Anyway, me not ACKing it doesn't mean someone else can't agree with you, I just wanted to know the reason (whether there have been a leak or something).
I didn't spot any issues through tests. But when I read these codes, I felt weird about this section. As we leaked so little memory here and it seldom happened, let's keep what they were. Thanks
Martin