From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34038) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bi0W0-0005GL-EP for qemu-devel@nongnu.org; Thu, 08 Sep 2016 10:41:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bi0Vz-0006CM-Gw for qemu-devel@nongnu.org; Thu, 08 Sep 2016 10:41:20 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34446) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bi0Vz-0006C6-BT for qemu-devel@nongnu.org; Thu, 08 Sep 2016 10:41:19 -0400 From: Juan Quintela In-Reply-To: <01e7066e-b807-95a6-6a5e-3435651a949e@redhat.com> (Eric Blake's message of "Wed, 7 Sep 2016 17:03:37 -0500") References: <1473169187-22750-1-git-send-email-ashijeetacharya@gmail.com> <01e7066e-b807-95a6-6a5e-3435651a949e@redhat.com> Reply-To: quintela@redhat.com Date: Thu, 08 Sep 2016 16:41:11 +0200 Message-ID: <87twdqzc6w.fsf@emacs.mitica> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v3] Move max-bandwidth and downtime-limit into migrate_set_parameter for both hmp and qmp List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Ashijeet Acharya , lcapitulino@redhat.com, amit.shah@redhat.com, armbru@redhat.com, dgilbert@redhat.com, pbonzini@redhat.com, berrange@redhat.com, qemu-devel@nongnu.org Eric Blake wrote: >> + if (has_max_bandwidth) { >> + s->parameters.max_bandwidth = max_bandwidth; >> + if (s->to_dst_file) { >> + qemu_file_set_rate_limit(s->to_dst_file, >> + s->parameters.max_bandwidth / XFER_LIMIT_RATIO); >> + } >> + } >> + if (has_downtime_limit) { >> + downtime_limit *= 1000000; /* convert to nanoseconds */ > > Are you sure this won't overflow? Ashijeet, Eric mere means that if downtime_limit is bigger that INT64_MAX/1000000, then you get an overflow with the multiplication. Notice that if it overflows, the value is already quite big O:-) 2^63/1000/1000/1000/3600/24/365 292.47 Allowing "only" 292 years of downtime should be enough for the time being (famous last words) O:-) > >> +void qmp_migrate_set_speed(int64_t valuebw, Error **errp) >> +{ >> + bool has_compress_level = false; >> + bool has_compress_threads = false; >> + bool has_decompress_threads = false; >> + bool has_cpu_throttle_initial = false; >> + bool has_cpu_throttle_increment = false; >> + bool has_tls_creds = false; >> + bool has_tls_hostname = false; >> + bool has_max_bandwidth = true; >> + bool has_downtime_limit = false; >> + const char *valuestr = NULL; >> + long valueint = 0; >> + Error *err = NULL; >> + >> + qmp_migrate_set_parameters(has_compress_level, valueint, >> + has_compress_threads, valueint, > > Ugg. This looks gross. No need to name a bunch of variables set to > false, when you can instead use QAPI's boxed conventions to pass a > pointer to a struct, and rely on 0-initialization of the struct to leave > all the parameters that you don't care about unmentioned. We should change qmp_migrate_set_parameters to your new api. I fully agree that this is gross, but it is the way it was written, nothing to do with this patch, really. Ashijeet, if you want to do this change in a different patch before this change, I am all for it, but that is independent of your change. With all the other suggestions of Eric, I agree, or I don't care. (If time is an int in milliseconds or a float, I don't really care one way or another. Whatever libvirt preffers). Thanks, Juan.