Hi Eric,
On Fri, Mar 21, 2014 at 08:37:40AM -0600, Eric Blake wrote:
> On 03/17/2014 05:10 PM, Leandro Dorileo wrote:
> > Cover basic aspects and API usage for QemuOpt. The current implementation
> > covers the API's planned to be changed by Chunyan Liu in his
> > QEMUOptionParameter
> > replacement/cleanup job.
> >
> > Other APIs should be covered in future improvements.
> >
> > Signed-off-by: Leandro Dorileo <l...@dorileo.org>
>
> Right here is where you should stick a --- marker.
>
> >
> > Changes:
> > v2:
> > + fixed comments;
> > + make use of g_assert_cmpstr();
> > + use error_abort instead of a local_err for qemu_opts_absorb_qdict();
> > + asserts on QemuOptsList (empty and list name);
> > + added test_qemu_opt_unset();
> > + asserts on qemu_opt_*_set() return;
> > + added test_qemu_opts_reset();
> > + added test_qemu_opts_set();
> > ---
>
> It's okay to have a duplicate one; but the main point is that the v2
> changelog is useful to reviewers but not to the git log; and anything
> after the --- marker gets omitted by 'git am' when a maintainer accepts
> your patch into their pull request.
I would say that I even know about the --- marker, but have misplaced it... :(
>
> > tests/Makefile | 3 +
> > tests/test-qemu-opts.c | 455
> > +++++++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 458 insertions(+)
> > create mode 100644 tests/test-qemu-opts.c
> >
> > +static void test_qemu_find_opts(void)
> > +{
> > + QemuOptsList *list;
> > +
> > + register_opts();
> > +
> > + /* we have an "opts_list_01" option, should return it */
> > + list = qemu_find_opts("opts_list_01");
> > + g_assert(list != NULL);
> > + g_assert_cmpstr(list->name, ==, "opts_list_01");
> > +}
> > +
> > +static void test_qemu_opts_create(void)
> > +{
> > + QemuOptsList *list;
> > + QemuOpts *opts;
> > +
> > + register_opts();
> > +
> > + list = qemu_find_opts("opts_list_01");
> > + g_assert(list != NULL);
> > + g_assert(QTAILQ_EMPTY(&list->head));
> > + g_assert_cmpstr(list->name, ==, "opts_list_01");
>
> This test starts as a duplicate of test_qemu_find_opts; I guess that's
> okay (having both tests means a bit more insight into what broke if one
> fails but the other passes).
>
> > +
> > +static void test_qemu_opt_unset(void)
> > +{
> > + QemuOpts *opts;
> > + const char *value;
> > + int ret;
> > +
> > + /* dinamically initialized (parsed) opts */
>
> s/dinamically/dynamically/
>
> That's trivially fixable, so feel free to add:
Ok...
>
> Reviewed-by: Eric Blake <ebl...@redhat.com>
Thanks for reviewing...
--
Leandro Dorileo
