From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56351) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wzmov-00060i-KQ for qemu-devel@nongnu.org; Wed, 25 Jun 2014 09:01:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wzmon-0006Hg-KO for qemu-devel@nongnu.org; Wed, 25 Jun 2014 09:01:01 -0400 Received: from mx1.redhat.com ([209.132.183.28]:11948) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wzmon-0006HY-Cd for qemu-devel@nongnu.org; Wed, 25 Jun 2014 09:00:53 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s5PD0pv4031544 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Wed, 25 Jun 2014 09:00:52 -0400 From: Juan Quintela In-Reply-To: <20140617115639.GH2503@work-vm> (David Alan Gilbert's message of "Tue, 17 Jun 2014 12:56:40 +0100") References: <1402912703-28195-1-git-send-email-quintela@redhat.com> <1402912703-28195-5-git-send-email-quintela@redhat.com> <20140617115639.GH2503@work-vm> Date: Wed, 25 Jun 2014 15:00:49 +0200 Message-ID: <87a991ywhq.fsf@troll.troll> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 04/27] vmstate: Port versioned tests to new format Reply-To: quintela@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert" Cc: qemu-devel@nongnu.org "Dr. David Alan Gilbert" wrote: > * Juan Quintela (quintela@redhat.com) wrote: >> Signed-off-by: Juan Quintela >> +static void obj_versioned_copy(void *arg1, void *arg2) >> { >> - QEMUFile *fsave = open_test_file(true); >> - uint8_t buf[] = { >> - 0, 0, 0, 10, /* a */ >> - 0, 0, 0, 30, /* c */ >> - 0, 0, 0, 0, 0, 0, 0, 40, /* d */ >> - QEMU_VM_EOF, /* just to ensure we won't get EOF reported prematurely */ >> - }; >> - qemu_put_buffer(fsave, buf, sizeof(buf)); >> - qemu_fclose(fsave); >> - >> - QEMUFile *loading = open_test_file(false); >> - TestStruct obj = { .b = 200, .e = 500, .f = 600 }; >> - vmstate_load_state(loading, &vmstate_versioned, &obj, 1); >> - g_assert(!qemu_file_get_error(loading)); >> - g_assert_cmpint(obj.a, ==, 10); >> - g_assert_cmpint(obj.b, ==, 200); >> - g_assert_cmpint(obj.c, ==, 30); >> - g_assert_cmpint(obj.d, ==, 40); >> - g_assert_cmpint(obj.e, ==, 500); >> - g_assert_cmpint(obj.f, ==, 600); >> - qemu_fclose(loading); >> + TestVersioned *target = arg1; >> + TestVersioned *source = arg2; >> + >> + target->a = source->a; >> + target->b = source->b; >> + target->c = source->c; >> + target->d = source->d; >> + target->e = source->e; >> + target->f = source->f; >> + target->skip_c_e = source->skip_c_e; > > Why's that not simply *target = *source? To be able to only copy some of the fields, depending on what we want to test O:-) >> + TestVersioned obj, obj_clone; >> + >> + memset(&obj, 0, sizeof(obj)); >> + save_vmstate(&vmstate_simple_versioned, &obj_versioned); >> + >> + compare_vmstate(wire_simple_v2, sizeof(wire_simple_v2)); >> + >> + SUCCESS(load_vmstate(&vmstate_simple_versioned, &obj, &obj_clone, >> + obj_versioned_copy, 2, wire_simple_v2, >> + sizeof(wire_simple_v2))); >> + >> +#define FIELD_EQUAL(name) g_assert_cmpint(obj.name, ==, obj_versioned.name) >> +#define FIELD_NOT_EQUAL(name) \ >> + g_assert_cmpint(obj.name, !=, obj_versioned.name) > > Given that macro is shared with the next few functions it would be seem > to declare it outside of the function. > It is not always the same (see the whole series otherwise). I ended finding easier to do it this way. I ended defining it the 1st time that I needed it. That is coherent with the whole series, but I can change it (don't really matter). >> + memset(&obj, 0, sizeof(obj)); >> + obj_versioned.skip_c_e = false; >> + save_vmstate(&vmstate_simple_skipping, &obj_versioned); >> + >> + compare_vmstate(wire_simple_no_skip, sizeof(wire_simple_no_skip)); >> + >> + /* We abuse the fact that f has a 0x00 value in the right position */ > > A bit nasty. Any better ideas? >> + compare_vmstate(wire_simple_skip, sizeof(wire_simple_skip)); >> + >> + /* We abuse the fact that f has a 0x00 value in the right position */ >> + SUCCESS(load_vmstate(&vmstate_simple_skipping, &obj, &obj_clone, >> + obj_versioned_copy, 1, wire_simple_skip, >> + sizeof(wire_simple_skip) - 8)); >> + >> + FIELD_EQUAL(skip_c_e); >> + FIELD_EQUAL(a); >> + FIELD_EQUAL(b); >> + FIELD_NOT_EQUAL(c); >> + FIELD_EQUAL(d); >> + FIELD_NOT_EQUAL(e); >> + FIELD_NOT_EQUAL(f); >> + >> + SUCCESS(load_vmstate(&vmstate_simple_skipping, &obj, &obj_clone, >> + obj_versioned_copy, 2, wire_simple_skip, >> + sizeof(wire_simple_skip))); >> + >> + FIELD_EQUAL(skip_c_e); >> + FIELD_EQUAL(a); >> + FIELD_EQUAL(b); >> + FIELD_NOT_EQUAL(c); >> + FIELD_EQUAL(d); >> + FIELD_NOT_EQUAL(e); >> + FIELD_EQUAL(f); >> } > > Couldn't those functions just be merged and take a flag? >I think it is clear this way, but that is the same to me. FIELD_EQUAL(a) FIELD_NOT_EQUAL(a) vs COMPARE_FIELD(a, true) COMPARE_FILED(b, false) For me, I don't need to go to the definition of the macro in the 1st case, and I do on the second one. Or do you mean anything different? Thanks, Juan.