From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59342) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wzn2K-0003ci-76 for qemu-devel@nongnu.org; Wed, 25 Jun 2014 09:14:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wzn2C-0002bU-Nt for qemu-devel@nongnu.org; Wed, 25 Jun 2014 09:14:52 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35378) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wzn2C-0002bI-Bu for qemu-devel@nongnu.org; Wed, 25 Jun 2014 09:14:44 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s5PDEfMX009509 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Wed, 25 Jun 2014 09:14:42 -0400 Date: Wed, 25 Jun 2014 14:14:38 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20140625131437.GD2387@work-vm> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87a991ywhq.fsf@troll.troll> Subject: Re: [Qemu-devel] [PATCH 04/27] vmstate: Port versioned tests to new format List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Juan Quintela Cc: qemu-devel@nongnu.org * 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? > >> + 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. > >> + 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. Dave > > Thanks, Juan. > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK