[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 04/27] vl: convert -m to qemu_opts_parse()
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 04/27] vl: convert -m to qemu_opts_parse() |
Date: |
Wed, 27 Nov 2013 15:35:09 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux) |
Igor Mammedov <address@hidden> writes:
> On Tue, 26 Nov 2013 15:49:05 +0100
> Markus Armbruster <address@hidden> wrote:
>
>> Igor Mammedov <address@hidden> writes:
>>
>> > On Thu, 21 Nov 2013 11:12:43 +0100
>> > Markus Armbruster <address@hidden> wrote:
>> >
>> >> Igor Mammedov <address@hidden> 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
Could the common code be factored out the old-fashioned way?
Precedence: qemu_get_machine_opts() encapsulates some QemuOpts-related
details, so its many users don't have to deal with them.
> 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?
What about val?
> [...]
>> >> 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.
Extending QEMUMachineInitArgs should suffice. Once you're inside the
board code, passing stuff around as C parameters is probably an
improvement over passing around QemuOpts.
> * 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.
Can't say without seeing the code.
> * 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.
QemuOpts do not record what QEMU uses. They record what the user asked
for.
> 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.
Keeping the user's configuration just in QemuOpts is fine. What I don't
like is messing with it there. This includes storing defaults.
Here's another reason: -writeconfig should write out exactly the user's
configuration. If you mess with it, it may write out messed up
configuration, depending on *when* you mess with it.
> Doing
> "git grep qemu_get_machine_opts"
> gets us several precedents that already use it that way.
Note that it does *not* store defaults in QemuOpts, it only creates
empty opts. I'm not sure that was a good idea.
> * 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.
I'm not sure I got this argument.
- [Qemu-devel] [PATCH 00/27 RFC v7] ACPI memory hotplug, Igor Mammedov, 2013/11/20
- [Qemu-devel] [PATCH 01/27] acpi: factor out common pm_update_sci() into acpi core, Igor Mammedov, 2013/11/20
- [Qemu-devel] [PATCH 03/27] pc: add 'etc/reserved-memory-end' fw_cfg interface for SeaBIOS, Igor Mammedov, 2013/11/20
- [Qemu-devel] [PATCH 04/27] vl: convert -m to qemu_opts_parse(), Igor Mammedov, 2013/11/20
- Re: [Qemu-devel] [PATCH 04/27] vl: convert -m to qemu_opts_parse(), Li Guang, 2013/11/21
- Re: [Qemu-devel] [PATCH 04/27] vl: convert -m to qemu_opts_parse(), Markus Armbruster, 2013/11/21
- Re: [Qemu-devel] [PATCH 04/27] vl: convert -m to qemu_opts_parse(), Igor Mammedov, 2013/11/26
- Re: [Qemu-devel] [PATCH 04/27] vl: convert -m to qemu_opts_parse(), Markus Armbruster, 2013/11/26
- Re: [Qemu-devel] [PATCH 04/27] vl: convert -m to qemu_opts_parse(), Igor Mammedov, 2013/11/26
- Re: [Qemu-devel] [PATCH 04/27] vl: convert -m to qemu_opts_parse(),
Markus Armbruster <=
- Re: [Qemu-devel] [PATCH 04/27] vl: convert -m to qemu_opts_parse(), Igor Mammedov, 2013/11/27
- Re: [Qemu-devel] [PATCH 04/27] vl: convert -m to qemu_opts_parse(), Markus Armbruster, 2013/11/27
- [Qemu-devel] [PATCH 04/28] vl: convert -m to QemuOpts, Igor Mammedov, 2013/11/26
- [Qemu-devel] [PATCH 05/28] vl.c: extend -m option to support options for memory hotplug, Igor Mammedov, 2013/11/26
Re: [Qemu-devel] [PATCH 04/27] vl: convert -m to qemu_opts_parse(), Paolo Bonzini, 2013/11/25
[Qemu-devel] [PATCH 02/27] rename pci_hotplug_fn to hotplug_fn and make it available for other devices, Igor Mammedov, 2013/11/20