From patchwork Tue Oct 25 19:12:23 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Dr. David Alan Gilbert" X-Patchwork-Id: 9395355 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id E2BD260234 for ; Tue, 25 Oct 2016 19:35:07 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 05C8829730 for ; Tue, 25 Oct 2016 19:35:08 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id EE9572973E; Tue, 25 Oct 2016 19:35:07 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, T_HK_NAME_DR autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 62E9B29730 for ; Tue, 25 Oct 2016 19:35:07 +0000 (UTC) Received: from localhost ([::1]:57767 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bz7V4-0006j8-2w for patchwork-qemu-devel@patchwork.kernel.org; Tue, 25 Oct 2016 15:35:06 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48767) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bz79D-0004dR-I3 for qemu-devel@nongnu.org; Tue, 25 Oct 2016 15:12:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bz79A-00021W-Dn for qemu-devel@nongnu.org; Tue, 25 Oct 2016 15:12:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42478) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bz79A-00021N-6I for qemu-devel@nongnu.org; Tue, 25 Oct 2016 15:12:28 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 79F3B8EB5A; Tue, 25 Oct 2016 19:12:27 +0000 (UTC) Received: from work-vm ([10.33.36.2]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u9PJCOi7015542 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Tue, 25 Oct 2016 15:12:26 -0400 Date: Tue, 25 Oct 2016 20:12:23 +0100 From: "Dr. David Alan Gilbert" To: Halil Pasic Message-ID: <20161025191223.GB5667@work-vm> References: <20161021143741.8597-1-pasic@linux.vnet.ibm.com> <20161021143741.8597-4-pasic@linux.vnet.ibm.com> <20161025101346.GB2034@work-vm> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.7.1 (2016-10-04) X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Tue, 25 Oct 2016 19:12:27 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 209.132.183.28 Subject: Re: [Qemu-devel] [RFC PATCH 3/4] migration/vmstate: fix array of pointers to struct X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Amit Shah , Guenther Hutzl , qemu-devel@nongnu.org, Juan Quintela Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > > > On 10/25/2016 12:13 PM, Dr. David Alan Gilbert wrote: > > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > >> > Make VMS_ARRAY_OF_POINTER cope with null pointers. Previously the reward > >> > for trying to migrate an array with some null pointers in it was an > >> > illegal memory access, that is a swift and painless death of the > >> > process. Let's make vmstate cope with this scenario at least for > >> > pointers to structs. The general approach is when we encounter a null > >> > pointer (element) instead of following the pointer to save/load the data > >> > behind it we save/load a placeholder. This way we can detect if we > >> > expected a null pointer at the load side but not null data was saved > >> > instead. Sadly all other error scenarios are not detected by this scheme > >> > (and would require the usage of the JSON meta data). > >> > > >> > Limitations: Does not work for pointers to primitives. > > Hmm is this needed - I mean could you do this just by giving the vmsd > > that defines the children of the array a '.needed' that tests if their > > pointer is NULL? > > > > > > I do not think so: .needed is basically for subsections (also used > in migration/savevm.c via the exported vmstate_save_needed function), > and .field_exists is also no use for this (AFAIU). Have also tried > just to be sure, it did not work for me. Hmm yes you're right; I thought .needed was more general; and field_exists does seem to be too late. > If I did not convince you, a bit of a code proving me wrong would be > highly appreciated. Well, here's some untested code (on top of your code with the test); it seems simple (if it works!) Dave > Thanks for the comment! > > Halil > --- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK diff --git a/migration/vmstate.c b/migration/vmstate.c index 0bc9f35..6d230ef 100644 --- a/migration/vmstate.c +++ b/migration/vmstate.c @@ -328,7 +328,9 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd, addr = *(void **)addr; } if (field->flags & VMS_STRUCT) { - vmstate_save_state(f, field->vmsd, addr, vmdesc_loop); + if (vmstate_save_needed(field->vmsd, addr)) { + vmstate_save_state(f, field->vmsd, addr, vmdesc_loop); + } } else { field->info->put(f, addr, size); } diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c index f8e7037..97919bb 100644 --- a/tests/test-vmstate.c +++ b/tests/test-vmstate.c @@ -497,6 +497,23 @@ const VMStateDescription vmsd_tst = { } }; +static bool tst_null_check(void *opaque) +{ + fprintf(stderr, "%s: %p\n", __func__, opaque); + return opaque != NULL; +} + +const VMStateDescription vmsd_tst_null = { + .name = "test/tstnull", + .version_id = 1, + .minimum_version_id = 1, + .needed = tst_null_check, + .fields = (VMStateField[]) { + VMSTATE_INT32(i, TestStructTriv), + VMSTATE_END_OF_LIST() + } +}; + #define AR_SIZE 4 typedef struct { @@ -513,6 +530,16 @@ const VMStateDescription vmsd_arps = { VMSTATE_END_OF_LIST() } }; +const VMStateDescription vmsd_arps_null = { + .name = "test/arpsnull", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_ARRAY_OF_POINTER_TO_STRUCT(ar, TestArrayOfPtrToStuct, + AR_SIZE, 0, vmsd_tst_null, TestStructTriv), + VMSTATE_END_OF_LIST() + } +}; static void test_arr_ptr_str_no0_save(void) { TestStructTriv ar[AR_SIZE] = {{.i = 0}, {.i = 1}, {.i = 2}, {.i = 3} }; @@ -557,7 +584,7 @@ static void test_arr_ptr_str_0_save(void) TestStructTriv ar[AR_SIZE] = {{.i = 0}, {.i = 1}, {.i = 2}, {.i = 3} }; TestArrayOfPtrToStuct sample = {.ar = {&ar[0], NULL, &ar[2], &ar[3]} }; - save_vmstate(&vmsd_arps, &sample); /* fails with SEGFAULT with master */ + save_vmstate(&vmsd_arps_null, &sample); /* fails with SEGFAULT with master */ } static void test_arr_ptr_str_0_load(void) @@ -568,14 +595,13 @@ static void test_arr_ptr_str_0_load(void) int idx; uint8_t wire_sample[] = { 0x00, 0x00, 0x00, 0x00, - 0x00, /* marker for the null pointer */ 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x03, QEMU_VM_EOF }; save_buffer(wire_sample, sizeof(wire_sample)); - SUCCESS(load_vmstate_one(&vmsd_arps, &obj, 1, + SUCCESS(load_vmstate_one(&vmsd_arps_null, &obj, 1, wire_sample, sizeof(wire_sample))); for (idx = 0; idx < AR_SIZE; ++idx) { /* compare the target array ar with the ground truth array ar_gt */