From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42375) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wznc5-0007Ql-Lr for qemu-devel@nongnu.org; Wed, 25 Jun 2014 09:51:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wznby-00013x-5t for qemu-devel@nongnu.org; Wed, 25 Jun 2014 09:51:49 -0400 Received: from mx1.redhat.com ([209.132.183.28]:63705) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wznbx-00013l-UP for qemu-devel@nongnu.org; Wed, 25 Jun 2014 09:51:42 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s5PDpeWk018831 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Wed, 25 Jun 2014 09:51:40 -0400 From: Juan Quintela In-Reply-To: <20140625131437.GD2387@work-vm> (David Alan Gilbert's message of "Wed, 25 Jun 2014 14:14:38 +0100") References: <1402912703-28195-1-git-send-email-quintela@redhat.com> <1402912703-28195-5-git-send-email-quintela@redhat.com> <20140617115639.GH2503@work-vm> <87a991ywhq.fsf@troll.troll> <20140625131437.GD2387@work-vm> Date: Wed, 25 Jun 2014 15:51:38 +0200 Message-ID: <8738etrtat.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: >> "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:-) > > But those last 7 lines do copy every field don't they? They do. >> >> + 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). > > OK. > >> >> + 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? > > Not really; but it's very reliant on the current format - although perhaps > demonstrates just how delicate that is. Yeap, anyways, that is how the test was written, nothing to do with the refactoring. Improvements on top welcome O:-) >> >> + 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? > > I meant test_simple_no_skip vs test_simple_skip; they seem to duplicate a lot. They were there on the original test, i.e. doesn't mater for the refactoring. Basically we are doing is testing the "test" function O:-) Later, Juan.