From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 352ACC0015E for ; Wed, 26 Jul 2023 08:36:50 +0000 (UTC) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qOa04-0007nW-VP; Wed, 26 Jul 2023 04:36:05 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qOa03-0007my-Lf for qemu-devel@nongnu.org; Wed, 26 Jul 2023 04:36:03 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.129.124]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qOa01-0003Y6-OO for qemu-devel@nongnu.org; Wed, 26 Jul 2023 04:36:03 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1690360561; h=from:from:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=sMXNw/fgKVvdY5aHitoqTnjttgtvO3LJEYlWtEdHkGo=; b=Gp4y7LNRjMec7utzj+1rhiEHJKlolgrRxNlPv/74gFMy/KaZL3n/jKe5JlScW9jegSjee6 ddxqKC92X+EK0CUw2Fp99Ugsr+R0ASqJVE+wU8EVCR0e+LYCH026psA0Gm4temR/9X/3rO UVs0ydA7b1hZ3CVewtT5IALjOQ47g6M= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-93-UuYhttLFMR6oAnZMMslIvg-1; Wed, 26 Jul 2023 04:35:57 -0400 X-MC-Unique: UuYhttLFMR6oAnZMMslIvg-1 Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.rdu2.redhat.com [10.11.54.10]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 198EB858290; Wed, 26 Jul 2023 08:35:57 +0000 (UTC) Received: from redhat.com (unknown [10.42.28.52]) by smtp.corp.redhat.com (Postfix) with ESMTPS id BC973492C13; Wed, 26 Jul 2023 08:35:55 +0000 (UTC) Date: Wed, 26 Jul 2023 09:35:53 +0100 From: Daniel =?utf-8?B?UC4gQmVycmFuZ8Op?= To: Het Gala Cc: "qemu-devel@nongnu.org" , Prerna Saxena , "quintela@redhat.com" , "dgilbert@redhat.com" , "pbonzini@redhat.com" , "armbru@redhat.com" , "eblake@redhat.com" , Manish Mishra , Aravind Retnakaran Subject: Re: [PATCH v9 06/10] migration: New migrate and migrate-incoming argument 'channels' Message-ID: References: <20230721144914.170991-1-het.gala@nutanix.com> <20230721144914.170991-7-het.gala@nutanix.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/2.2.9 (2022-11-12) X-Scanned-By: MIMEDefang 3.1 on 10.11.54.10 Received-SPF: pass client-ip=170.10.129.124; envelope-from=berrange@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H4=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Daniel =?utf-8?B?UC4gQmVycmFuZ8Op?= Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org On Wed, Jul 26, 2023 at 01:24:48AM +0530, Het Gala wrote: > Sorry, last reply on this patch was accidently replied only to Daniel. > Pasting the reply again so it is received by all the active maintianers > here. Apologies for the error 😅 > > On 26/07/23 12:07 am, Daniel P. Berrangé wrote: > > On Tue, Jul 25, 2023 at 07:34:09PM +0100, Daniel P. Berrangé wrote: > > > On Fri, Jul 21, 2023 at 02:49:31PM +0000, Het Gala wrote: > > > > MigrateChannelList allows to connect accross multiple interfaces. > > > > Add MigrateChannelList struct as argument to migration QAPIs. > > > > > > > > We plan to include multiple channels in future, to connnect > > > > multiple interfaces. Hence, we choose 'MigrateChannelList' > > > > as the new argument over 'MigrateChannel' to make migration > > > > QAPIs future proof. > > > > > > > > Suggested-by: Aravind Retnakaran > > > > Signed-off-by: Het Gala > > > > Acked-by: Markus Armbruster > > > > --- > > > > migration/migration-hmp-cmds.c | 6 +- > > > > migration/migration.c | 34 ++++++++-- > > > > qapi/migration.json | 109 ++++++++++++++++++++++++++++++++- > > > > softmmu/vl.c | 2 +- > > > > 4 files changed, 139 insertions(+), 12 deletions(-) > > > > > > > > diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c > > > > index 9885d7c9f7..49b150f33f 100644 > > > > --- a/migration/migration-hmp-cmds.c > > > > +++ b/migration/migration-hmp-cmds.c > > > > @@ -424,7 +424,7 @@ void hmp_migrate_incoming(Monitor *mon, const QDict *qdict) > > > > Error *err = NULL; > > > > const char *uri = qdict_get_str(qdict, "uri"); > > > > - qmp_migrate_incoming(uri, &err); > > > > + qmp_migrate_incoming(uri, false, NULL, &err); > > > > hmp_handle_error(mon, err); > > > > } > > > > @@ -705,8 +705,8 @@ void hmp_migrate(Monitor *mon, const QDict *qdict) > > > > const char *uri = qdict_get_str(qdict, "uri"); > > > > Error *err = NULL; > > > > - qmp_migrate(uri, !!blk, blk, !!inc, inc, > > > > - false, false, true, resume, &err); > > > > + qmp_migrate(uri, false, NULL, !!blk, blk, !!inc, inc, > > > > + false, false, true, resume, &err); > > > > if (hmp_handle_error(mon, err)) { > > > > return; > > > > } > > > > diff --git a/migration/migration.c b/migration/migration.c > > > > index f37b388876..bd3a93fc8c 100644 > > > > --- a/migration/migration.c > > > > +++ b/migration/migration.c > > > > @@ -466,10 +466,22 @@ static bool migrate_uri_parse(const char *uri, > > > > return true; > > > > } > > > > -static void qemu_start_incoming_migration(const char *uri, Error **errp) > > > > +static void qemu_start_incoming_migration(const char *uri, bool has_channels, > > > > + MigrationChannelList *channels, > > > > + Error **errp) > > > > { > > > > g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1); > > > > + /* > > > > + * Having preliminary checks for uri and channel > > > > + */ > > > > + if (uri && has_channels) { > > > > + error_setg(errp, "'uri' and 'channels' arguments are mutually " > > > > + "exclusive; exactly one of the two should be present in " > > > > + "'migrate-incoming' qmp command "); > > > > + return; > > > > + } > > > This checks is both are present. > > > > > > Also needs a check if neither are present as that's invalid. > > Also it should (temporarily) raise an error if "has_channels" is > > set, as while we've added the parameter in QAPI, we've not > > implemented it yet. IOW, raise an error now, and remove the > > error in a later patch. > Ack. So in total there should be 3 checks right. 1) if 'has_channels' is > set, 2) if 'uri' and 'channels' both are present, 3) if 'uri' and 'channels' > both are absent. Basically right now only uri should allowed and should > atleast be present. Correct. > I think overall only 1) would be enough and should be checked before > 'migration_channels_and_uri_compatible()' and if 'has_channels' is set, just > return for now. With this 2) would not be necessary or not come into play in > this patch. 3) will be taken care by > 'migration_channels_and_uri_compatible()' itself IMO. I think all the checks should be in this method, as it is the entrypoint for execution of the QMP command and thus where parameter validation should live. Spreading it across methods will make it very easy to open bugs by refactoring code and not realizing the new codepaths don't do the right checks. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|