On 07/30/2013 07:05 AM, Peter Krempa wrote: > This patch exports a few utility functions and adds testing of shutdown > commands of the guest agent. > --- > tests/qemuagenttest.c | 119 +++++++++++++++++++++++++++++++++++++++++++ > tests/qemumonitortestutils.c | 21 ++++---- > tests/qemumonitortestutils.h | 28 ++++++++-- > 3 files changed, 153 insertions(+), 15 deletions(-) > > +++ b/tests/qemumonitortestutils.c > @@ -37,12 +37,6 @@ > > #define VIR_FROM_THIS VIR_FROM_NONE > > -typedef struct _qemuMonitorTestItem qemuMonitorTestItem; > -typedef qemuMonitorTestItem *qemuMonitorTestItemPtr; > -typedef int (*qemuMonitorTestResponseCallback)(qemuMonitorTestPtr test, > - qemuMonitorTestItemPtr item, > - const char *message); > - Hmm - this series is what added qemuMonitorTestResponseCallback, so this feels like churn. You should probably amend patch 10/20 to declare qemuMonitorTestResponseCallback in the .h in the first place, and move the typedefs at that time. > @@ -167,7 +161,6 @@ cleanup: > } > > > - > static int Definitely some churn going on. > @@ -414,6 +407,12 @@ error: > return -1; > } > > +void * > +qemuMonitorTestItemGetPrivateData(qemuMonitorTestItemPtr item) Missing the typical two blank lines. Even after you hoist hunks to the proper earlier patches, this still feels like two patches rolled into one - I'd split it into the exporting of useful helper functions, then the new agent test that takes advantage of the helpers. At any rate, the test addition itself seems sane, and my complaints are only about presentation of the commit contents. ACK to the end result, and rebasing seems like something you can safely do without me needing to see a v2, as long as you test that each step compiles and passes 'make check'. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list