[{"id":1764252,"web_url":"http://patchwork.ozlabs.org/comment/1764252/","msgid":"<265f10bb-5718-aaac-d0fe-595a78acdf46@redhat.com>","list_archive_url":null,"date":"2017-09-06T16:15:23","subject":"Re: [Qemu-devel] [PATCH v7 07/22] migration: Make\n\tmigrate_fd_error() the owner of the Error","submitter":{"id":6591,"url":"http://patchwork.ozlabs.org/api/people/6591/","name":"Eric Blake","email":"eblake@redhat.com"},"content":"On 09/06/2017 06:51 AM, Juan Quintela wrote:\n> So far, we had to free the error after each caller, so just do it\n> here. Once there, tls.c was leaking the error.\n\nYou mention tls.c,\n\n> \n> Signed-off-by: Juan Quintela \n> ---\n> migration/channel.c | 1 -\n> migration/migration.c | 10 ++++------\n> migration/migration.h | 4 ++--\n> migration/socket.c | 1 -\n> 4 files changed, 6 insertions(+), 10 deletions(-)\n\nbut don't touch it. Am I missing something?\n\n> \n> -void migrate_fd_error(MigrationState *s, const Error *error)\n> +void migrate_fd_error(MigrationState *s, Error *error)\n> {\n\nNo comments at definition,\n\n> +++ b/migration/migration.h\n> @@ -163,8 +163,8 @@ bool migration_has_all_channels(void);\n> \n> uint64_t migrate_max_downtime(void);\n> \n> -void migrate_set_error(MigrationState *s, const Error *error);\n> -void migrate_fd_error(MigrationState *s, const Error *error);\n> +void migrate_set_error(MigrationState *s, Error *error);\n> +void migrate_fd_error(MigrationState *s, Error *error);\n\nor at declaration. That would be worth adding at some point, but this\npatch isn't making it worse.\n\nThe code looks okay in isolation, so if it is only the commit message\nthat needs fixing,\nReviewed-by: Eric Blake ","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx01.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx01.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=eblake@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xnTq50Y9Fz9s9Y\n\tfor ;\n\tThu, 7 Sep 2017 02:40:49 +1000 (AEST)","from localhost ([::1]:37113 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dpdNf-0002aP-8K\n\tfor incoming@patchwork.ozlabs.org; Wed, 06 Sep 2017 12:40:47 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:45704)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dpczZ-0008N6-N9\n\tfor qemu-devel@nongnu.org; Wed, 06 Sep 2017 12:15:57 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dpczT-0003y6-W1\n\tfor qemu-devel@nongnu.org; Wed, 06 Sep 2017 12:15:53 -0400","from mx1.redhat.com ([209.132.183.28]:50056)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from ) id 1dpczT-0003xc-Mo\n\tfor qemu-devel@nongnu.org; Wed, 06 Sep 2017 12:15:47 -0400","from smtp.corp.redhat.com\n\t(int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id A901D81E18\n\tfor ; Wed, 6 Sep 2017 16:15:46 +0000 (UTC)","from [10.10.120.228] (ovpn-120-228.rdu2.redhat.com [10.10.120.228])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 35B734F6DB;\n\tWed, 6 Sep 2017 16:15:26 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com A901D81E18","To":"Juan Quintela , qemu-devel@nongnu.org","References":"<20170906115143.27451-1-quintela@redhat.com>\n\t<20170906115143.27451-8-quintela@redhat.com>","From":"Eric Blake ","Openpgp":"url=http://people.redhat.com/eblake/eblake.gpg","Organization":"Red Hat, Inc.","Message-ID":"<265f10bb-5718-aaac-d0fe-595a78acdf46@redhat.com>","Date":"Wed, 6 Sep 2017 11:15:23 -0500","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<20170906115143.27451-8-quintela@redhat.com>","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"P6pWfS71U3gIs8g9ITrqOOgPm7FHln6M7\"","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.14","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.25]);\n\tWed, 06 Sep 2017 16:15:46 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","X-Content-Filtered-By":"Mailman/MimeDel 2.1.21","Subject":"Re: [Qemu-devel] [PATCH v7 07/22] migration: Make\n\tmigrate_fd_error() the owner of the Error","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"lvivier@redhat.com, dgilbert@redhat.com, peterx@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1764677,"web_url":"http://patchwork.ozlabs.org/comment/1764677/","msgid":"<87tw0ebu7g.fsf@secure.laptop>","list_archive_url":null,"date":"2017-09-07T11:53:55","subject":"Re: [Qemu-devel] [PATCH v7 07/22] migration: Make\n\tmigrate_fd_error() the owner of the Error","submitter":{"id":2643,"url":"http://patchwork.ozlabs.org/api/people/2643/","name":"Juan Quintela","email":"quintela@redhat.com"},"content":"Eric Blake wrote:\nD> On 09/06/2017 06:51 AM, Juan Quintela wrote:\n>> So far, we had to free the error after each caller, so just do it\n>> here. Once there, tls.c was leaking the error.\n>\n> You mention tls.c,\n>\n>> \n>> Signed-off-by: Juan Quintela \n>> ---\n>> migration/channel.c | 1 -\n>> migration/migration.c | 10 ++++------\n>> migration/migration.h | 4 ++--\n>> migration/socket.c | 1 -\n>> 4 files changed, 6 insertions(+), 10 deletions(-)\n>\n> but don't touch it. Am I missing something?\n>\n\nIt was missing error_free(); So it leaked the Error * variable.\nI will improve the message for next version.\n\n\n\n>> \n>> -void migrate_fd_error(MigrationState *s, const Error *error)\n>> +void migrate_fd_error(MigrationState *s, Error *error)\n>> {\n>\n> No comments at definition,\n\nWe free it inside now, so it can't be const.\n\n>> +++ b/migration/migration.h\n>> @@ -163,8 +163,8 @@ bool migration_has_all_channels(void);\n>> \n>> uint64_t migrate_max_downtime(void);\n>> \n>> -void migrate_set_error(MigrationState *s, const Error *error);\n>> -void migrate_fd_error(MigrationState *s, const Error *error);\n>> +void migrate_set_error(MigrationState *s, Error *error);\n>> +void migrate_fd_error(MigrationState *s, Error *error);\n>\n> or at declaration. That would be worth adding at some point, but this\n> patch isn't making it worse.\n\nwill add them, thanks.\n\n> The code looks okay in isolation, so if it is only the commit message\n> that needs fixing,\n> Reviewed-by: Eric Blake \n\nThanks.","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx03.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx03.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=quintela@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xnzQQ10slz9sRV\n\tfor ;\n\tThu, 7 Sep 2017 21:54:38 +1000 (AEST)","from localhost ([::1]:39924 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dpvOF-0000Lz-OO\n\tfor incoming@patchwork.ozlabs.org; Thu, 07 Sep 2017 07:54:35 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:52155)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dpvNl-0000HI-GY\n\tfor qemu-devel@nongnu.org; Thu, 07 Sep 2017 07:54:10 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dpvNg-00018Y-Q7\n\tfor qemu-devel@nongnu.org; Thu, 07 Sep 2017 07:54:05 -0400","from mx1.redhat.com ([209.132.183.28]:42582)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from ) id 1dpvNg-000188-Jy\n\tfor qemu-devel@nongnu.org; Thu, 07 Sep 2017 07:54:00 -0400","from smtp.corp.redhat.com\n\t(int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id C8A037E42C\n\tfor ; Thu, 7 Sep 2017 11:53:58 +0000 (UTC)","from localhost (ovpn-116-231.ams2.redhat.com [10.36.116.231])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id DF030BE804;\n\tThu, 7 Sep 2017 11:53:55 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com C8A037E42C","From":"Juan Quintela ","To":"Eric Blake ","In-Reply-To":"<265f10bb-5718-aaac-d0fe-595a78acdf46@redhat.com> (Eric Blake's\n\tmessage of \"Wed, 6 Sep 2017 11:15:23 -0500\")","References":"<20170906115143.27451-1-quintela@redhat.com>\n\t<20170906115143.27451-8-quintela@redhat.com>\n\t<265f10bb-5718-aaac-d0fe-595a78acdf46@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux)","Date":"Thu, 07 Sep 2017 13:53:55 +0200","Message-ID":"<87tw0ebu7g.fsf@secure.laptop>","MIME-Version":"1.0","Content-Type":"text/plain","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.12","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.27]);\n\tThu, 07 Sep 2017 11:53:58 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [PATCH v7 07/22] migration: Make\n\tmigrate_fd_error() the owner of the Error","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Reply-To":"quintela@redhat.com","Cc":"lvivier@redhat.com, qemu-devel@nongnu.org, peterx@redhat.com,\n\tdgilbert@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1766461,"web_url":"http://patchwork.ozlabs.org/comment/1766461/","msgid":"<20170911185818.GG2857@work-vm>","list_archive_url":null,"date":"2017-09-11T18:58:18","subject":"Re: [Qemu-devel] [PATCH v7 07/22] migration: Make\n\tmigrate_fd_error() the owner of the Error","submitter":{"id":48102,"url":"http://patchwork.ozlabs.org/api/people/48102/","name":"Dr. David Alan Gilbert","email":"dgilbert@redhat.com"},"content":"* Eric Blake (eblake@redhat.com) wrote:\n> On 09/06/2017 06:51 AM, Juan Quintela wrote:\n> > So far, we had to free the error after each caller, so just do it\n> > here. Once there, tls.c was leaking the error.\n> \n> You mention tls.c,\n> \n> > \n> > Signed-off-by: Juan Quintela \n> > ---\n> > migration/channel.c | 1 -\n> > migration/migration.c | 10 ++++------\n> > migration/migration.h | 4 ++--\n> > migration/socket.c | 1 -\n> > 4 files changed, 6 insertions(+), 10 deletions(-)\n> \n> but don't touch it. Am I missing something?\n\n\nhmm well I see in migration/tls.c:\n\n if (qio_task_propagate_error(task, &err)) {\n trace_migration_tls_outgoing_handshake_error(error_get_pretty(err));\n migrate_fd_error(s, err);\n error_free(err);\n } else {\n trace_migration_tls_outgoing_handshake_complete();\n migration_channel_connect(s, ioc, NULL);\n }\n\nso I think that error_free has to go?\n\nDave\n\n> > \n> > -void migrate_fd_error(MigrationState *s, const Error *error)\n> > +void migrate_fd_error(MigrationState *s, Error *error)\n> > {\n> \n> No comments at definition,\n> \n> > +++ b/migration/migration.h\n> > @@ -163,8 +163,8 @@ bool migration_has_all_channels(void);\n> > \n> > uint64_t migrate_max_downtime(void);\n> > \n> > -void migrate_set_error(MigrationState *s, const Error *error);\n> > -void migrate_fd_error(MigrationState *s, const Error *error);\n> > +void migrate_set_error(MigrationState *s, Error *error);\n> > +void migrate_fd_error(MigrationState *s, Error *error);\n> \n> or at declaration. That would be worth adding at some point, but this\n> patch isn't making it worse.\n> \n> The code looks okay in isolation, so if it is only the commit message\n> that needs fixing,\n> Reviewed-by: Eric Blake \n> \n> -- \n> Eric Blake, Principal Software Engineer\n> Red Hat, Inc. +1-919-301-3266\n> Virtualization: qemu.org | libvirt.org\n> \n\n\n\n--\nDr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx05.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx05.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=dgilbert@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xrcf24Ghxz9s7M\n\tfor ;\n\tTue, 12 Sep 2017 04:58:50 +1000 (AEST)","from localhost ([::1]:60020 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1drTuy-0004rK-PJ\n\tfor incoming@patchwork.ozlabs.org; Mon, 11 Sep 2017 14:58:48 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:49180)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1drTuf-0004qz-11\n\tfor qemu-devel@nongnu.org; Mon, 11 Sep 2017 14:58:29 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1drTub-0006Ye-Qf\n\tfor qemu-devel@nongnu.org; Mon, 11 Sep 2017 14:58:29 -0400","from mx1.redhat.com ([209.132.183.28]:39354)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from ) id 1drTub-0006YS-K2\n\tfor qemu-devel@nongnu.org; Mon, 11 Sep 2017 14:58:25 -0400","from smtp.corp.redhat.com\n\t(int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 803D2347A50\n\tfor ; Mon, 11 Sep 2017 18:58:24 +0000 (UTC)","from work-vm (ovpn-117-187.ams2.redhat.com [10.36.117.187])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 701F45D6A2;\n\tMon, 11 Sep 2017 18:58:21 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 803D2347A50","Date":"Mon, 11 Sep 2017 19:58:18 +0100","From":"\"Dr. David Alan Gilbert\" ","To":"Eric Blake ","Message-ID":"<20170911185818.GG2857@work-vm>","References":"<20170906115143.27451-1-quintela@redhat.com>\n\t<20170906115143.27451-8-quintela@redhat.com>\n\t<265f10bb-5718-aaac-d0fe-595a78acdf46@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<265f10bb-5718-aaac-d0fe-595a78acdf46@redhat.com>","User-Agent":"Mutt/1.8.3 (2017-05-23)","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.15","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.29]);\n\tMon, 11 Sep 2017 18:58:24 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [PATCH v7 07/22] migration: Make\n\tmigrate_fd_error() the owner of the Error","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"lvivier@redhat.com, qemu-devel@nongnu.org, peterx@redhat.com,\n\tJuan Quintela ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]