From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH 1/2] hostmem: fix QEMU crash by 'info memdev' Date: Wed, 13 Jul 2016 13:37:25 +0200 Message-ID: <0c7f2d23-92c0-8478-7262-6cdd2b800d5d@redhat.com> References: <1468383486-108169-1-git-send-email-guangrong.xiao@linux.intel.com> <945b233e-286f-fc22-8837-761e1ac2e522@redhat.com> <87r3ax6ajl.fsf@dusky.pond.sub.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: Xiao Guangrong , ehabkost@redhat.com, kvm@vger.kernel.org, mst@redhat.com, gleb@kernel.org, mtosatti@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, imammedo@redhat.com, rth@twiddle.net To: Markus Armbruster Return-path: In-Reply-To: <87r3ax6ajl.fsf@dusky.pond.sub.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org Sender: "Qemu-devel" List-Id: kvm.vger.kernel.org On 13/07/2016 13:29, Markus Armbruster wrote: >> > I'm curious about one thing. Eric/Markus, it would be nice to open code >> > the visit of the list with >> > >> > visit_start_list(v, name, NULL, 0, &err); >> > if (err) { >> > goto out; >> > } >> > ... >> > visit_type_uint16(v, name, &value, &err); >> > visit_next_list(v, NULL, 0); >> > ... >> > visit_end_list(v, NULL); >> > >> > We know here that on the other side there is an output visitor. >> > However, it doesn't work because visit_next_list asserts that tail == >> > NULL. Would it be easy to support this idiom, and would it make sense >> > to extend it to other kinds of visitor? > visit_next_list() asserts tail != NULL because to protect the > next_list() method. qmp_output_next_list() dereferences tail. > > Note that you don't have to call visit_next_list() in a virtual visit. > For an example, see prop_get_fdt(). Good enough already? Yes, definitely! I'm queueing Guangrong's patch because it fixes a crash and the leak existed before, but without next_list we can indeed visit a "virtual" list and fix the leak. It can be done during the -rc period. Paolo From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54136) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bNITw-0008TF-Bh for qemu-devel@nongnu.org; Wed, 13 Jul 2016 07:37:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bNITs-0004XW-6k for qemu-devel@nongnu.org; Wed, 13 Jul 2016 07:37:35 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48930) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bNITs-0004XR-0u for qemu-devel@nongnu.org; Wed, 13 Jul 2016 07:37:32 -0400 References: <1468383486-108169-1-git-send-email-guangrong.xiao@linux.intel.com> <945b233e-286f-fc22-8837-761e1ac2e522@redhat.com> <87r3ax6ajl.fsf@dusky.pond.sub.org> From: Paolo Bonzini Message-ID: <0c7f2d23-92c0-8478-7262-6cdd2b800d5d@redhat.com> Date: Wed, 13 Jul 2016 13:37:25 +0200 MIME-Version: 1.0 In-Reply-To: <87r3ax6ajl.fsf@dusky.pond.sub.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/2] hostmem: fix QEMU crash by 'info memdev' List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Xiao Guangrong , imammedo@redhat.com, ehabkost@redhat.com, kvm@vger.kernel.org, mst@redhat.com, gleb@kernel.org, mtosatti@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, rth@twiddle.net On 13/07/2016 13:29, Markus Armbruster wrote: >> > I'm curious about one thing. Eric/Markus, it would be nice to open code >> > the visit of the list with >> > >> > visit_start_list(v, name, NULL, 0, &err); >> > if (err) { >> > goto out; >> > } >> > ... >> > visit_type_uint16(v, name, &value, &err); >> > visit_next_list(v, NULL, 0); >> > ... >> > visit_end_list(v, NULL); >> > >> > We know here that on the other side there is an output visitor. >> > However, it doesn't work because visit_next_list asserts that tail == >> > NULL. Would it be easy to support this idiom, and would it make sense >> > to extend it to other kinds of visitor? > visit_next_list() asserts tail != NULL because to protect the > next_list() method. qmp_output_next_list() dereferences tail. > > Note that you don't have to call visit_next_list() in a virtual visit. > For an example, see prop_get_fdt(). Good enough already? Yes, definitely! I'm queueing Guangrong's patch because it fixes a crash and the leak existed before, but without next_list we can indeed visit a "virtual" list and fix the leak. It can be done during the -rc period. Paolo