[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]