[libvirt] [PATCH 18/20] qemuagenttest: Introduce testing of shutdown commands

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130730/80a639eb/attachment-0001.sig>