From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51306) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VlLwS-0005IA-JT for qemu-devel@nongnu.org; Tue, 26 Nov 2013 11:56:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VlLwM-0005Ly-I1 for qemu-devel@nongnu.org; Tue, 26 Nov 2013 11:56:52 -0500 Received: from mx1.redhat.com ([209.132.183.28]:64412) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VlLwM-0005Lo-9P for qemu-devel@nongnu.org; Tue, 26 Nov 2013 11:56:46 -0500 Date: Tue, 26 Nov 2013 17:55:32 +0100 From: Igor Mammedov Message-ID: <20131126175532.4df00dd6@thinkpad> In-Reply-To: <87pppn6wwu.fsf@blackfin.pond.sub.org> References: <1385001528-12003-1-git-send-email-imammedo@redhat.com> <1385001528-12003-5-git-send-email-imammedo@redhat.com> <87vbzmxdus.fsf@blackfin.pond.sub.org> <20131126141734.75dd61b9@thinkpad> <87pppn6wwu.fsf@blackfin.pond.sub.org> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 04/27] vl: convert -m to qemu_opts_parse() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, peter.maydell@linaro.org, aliguori@amazon.com, stefanb@linux.vnet.ibm.com, hutao@cn.fujitsu.com, mst@redhat.com, mjt@tls.msk.ru, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com, vasilis.liaskovitis@profitbricks.com, quintela@redhat.com, kraxel@redhat.com, stefanha@redhat.com, pbonzini@redhat.com, marcel.a@redhat.com, lcapitulino@redhat.com, chegu_vinod@hp.com, afaerber@suse.de On Tue, 26 Nov 2013 15:49:05 +0100 Markus Armbruster wrote: > Igor Mammedov writes: > > > On Thu, 21 Nov 2013 11:12:43 +0100 > > Markus Armbruster wrote: > > > >> Igor Mammedov writes: > >> [...] > Two separate issues here: > > 1. The "no qemu_mem_opts have been specified" case > > This is equivalent to "empty options". Therefore, the case can be > eliminated by pre-creating empty options. No objection. > > The three existing merge_lists users don't do that. Perhaps they > should. > > 2. How to provide default values > > Supplying defaults is left to the caller of qemu_opt_get_FOO() by > design. > > Pre-creating option parameters deviates from that pattern. You > justify it by saying it "eliminates need to pepper code with > DEFAULT_RAM_SIZE * 1024 * 1024". How many occurrences? beside of vl.c for: mem & maxmem 1 in hw/i386/pc.c slots - 6 in several files see below for continuation: > > Drawback: you lose the ability to see whether the user gave a value. > See below. > [...] > >> Ugly. > >> > >> Why is the variable called 'end'? > > would be 'suffix' better? > > end points to the whole value string, not the end of anything, and > neither to a suffix of anything. Any suggestions? [...] > >> If you refrain from putting defaults into opts, you can distinguish the > >> cases "user didn't specify maxmem, so assume mem", and "user specified > >> maxmem, so check it's >= mem". > > So foar, there is no point in distinguishing above cases, > > since maxmem <= mem is invalid value and hotplug should be disabled. > > So setting default maxmem to mem or anything less effectively disables hotplug. > > Yes, setting maxmem < mem is invalid and should be rejected, but not > setting maxmem at all should be accepted even when you set mem. > > Your patch like this pseudo-code: > > mem = DEFAULT_RAM_SIZE * 1024 * 1024 > maxmem = mem > > if user specifies mem: > mem = user's mem > if user specifes max-mem: > mem = user's max-mem > > if max-mem < mem > what now? > should error our if max-mem and mem were specified by the user > shouldn't if user didn't specify max-mem! > but can't say whether he did > > I'd do it this way: > > mem = unset > maxmem = unset > > if user specifies mem: > mem = user's mem > if user specifes max-mem: > mem = user's max-mem > > if mem != unset && max-mem != unset && max-mem < mem > error > > I'd use QemuOpts for the user's command line, and no more. For anything > beyond that, I'd use ordinary variables, such as ram_size. Ok, I'll revert to the old code where options users check for option availability, it's not that much code. As for using QemuOpts as global store for global variables: * using local variables would require changing of machine init or/and QEMUMachine and changing functions signature pass them down the stack to consumers. * adding "slots" readonly property to i440fx & q35 for consumption in ACPI hotplug code and building ACPI tables. It would be essentially another global lookup for i440fx & q35 object and pulling "slots" property, which is much longer way/complex way to get global value. That's a lot of boilerplate code for the same outcome. * about setting default for "mem" value: if default "mem" is not set and no -m is provided on CLI, we get case where ram_size = foo & "mem" unset And if I recall correctly there was an effort to provide interface for currently used QemuOpts to external users. So "mem" would get inconsistent with what QEMU uses. To sum up above said: * I'd like to continue using QemuOpts as global constant value store, it saves from adding a lot of boilerplate-code that would do the same. Doing "git grep qemu_get_machine_opts" gets us several precedents that already use it that way. * I believe that setting default in QemuOpts for "mem" is a good thing that leads to consistent meaning of "mem" with what QEMU actually uses. -- Regards, Igor