Re: [PATCH 02/48] Destroy virdomainlist.[ch]

[Osier Yang <jyang@xxxxxxxxxx>]

On 2012年08月14日 07:53, Eric Blake wrote:
> On 08/03/2012 09:48 AM, Osier Yang wrote:
> > As the consensus in:
> > https://www.redhat.com/archives/libvir-list/2012-July/msg01692.html,
> > this patch is to destroy conf/virdomainlist.[ch], foldering the
>
> s/foldering/folding/
>
> > helpers into conf/domain_conf.[ch].
>
> > * src/Makefile.am:
> >    - Various indentions fixes incidentally
>
> s/indentions/indentations/
>
> The fact that you used the word 'incidentally' (or any other synonym of
> 'also') should be a red flag that says "am I doing enough extra stuff to
> warrant splitting it into a separate patch?"  If the cleanups are
> limited to the hunks already touched, and the bulk of the changes are
> tied to the new code rather than cleanup, then a single patch is okay.
> But in this particular case, you ended up touching more lines of
> Makefile.am for indentation than you did for actual code changes,
> including hunks that had no other change, so it should probably be split
> into two patches.

Hum, think I was lazy at that time, ;-)

> However, I'd like to see this go in sooner, rather than later, as I have
> a pending patch based on top of this one that further splits
> domain_conf.c into snapshot_conf.c now that I'm adding even more
> snapshot functionality.  So while splitting would be nice, I'll let this
> one slide if its easier for you to push as-is.
>
> However, I did hit a compile failure:
>
>    CC     libvirt_conf_la-domain_conf.lo
> conf/domain_conf.c: In function 'virDomainList':
> conf/domain_conf.c:15533:17: error: implicit declaration of function
> 'virUnrefDomain' [-Werror=implicit-function-declaration]
> conf/domain_conf.c:15533:17: error: nested extern declaration of
> 'virUnrefDomain' [-Werror=nested-externs]
> conf/domain_conf.c: In function 'virDomainListSnapshots':
> conf/domain_conf.c:15579:17: error: implicit declaration of function
> 'virUnrefDomainSnapshot' [-Werror=implicit-function-declaration]
> conf/domain_conf.c:15579:17: error: nested extern declaration of
> 'virUnrefDomainSnapshot' [-Werror=nested-externs]
>
> which means you need to rebase it on top of Dan's atomic patches before
> you can push.
>
> ACK with this squashed in:
>
> diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c
> index 688da54..71f94e8 100644
> --- i/src/conf/domain_conf.c
> +++ w/src/conf/domain_conf.c
> @@ -15530,7 +15530,7 @@ cleanup:
>           int count = virHashSize(domobjs);
>           for (i = 0; i<  count; i++) {
>               if (data.domains[i])
> -                virUnrefDomain(data.domains[i]);

Not sure if it will cause conflicts to you, but the "if" is removed
for "avoid_if_before_free".

> +                virObjectUnref(data.domains[i]);
>           }
>       }
>
> @@ -15576,7 +15576,7 @@ cleanup:
>       if (ret<  0&&  list) {
>           for (i = 0; i<  count; i++)
>               if (list[i])
> -                virUnrefDomainSnapshot(list[i]);

Likewise.

> +                virObjectUnref(list[i]);
>           VIR_FREE(list);
>       }
>       return ret;

Thanks, I pushed the patch as-is (with the changes).

Regards,
Osier

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list