Re: [Qemu-devel] [PATCH 1/1] target-i386: Remove icc_bridge parameter from cpu_x86_create()

[Eduardo Habkost]

On Tue, Mar 10, 2015 at 02:33:17PM +0100, Andreas Färber wrote:
> Am 10.03.2015 um 14:30 schrieb Eduardo Habkost:
> > On Tue, Mar 10, 2015 at 02:22:01PM +0100, Andreas Färber wrote:
> >> Am 05.03.2015 um 18:26 schrieb Eduardo Habkost:
> >>> Instead of passing icc_bridge from the PC initialization code to
> >>> cpu_x86_create(), make the PC initialization code attach the CPU to
> >>> icc_bridge.
> >>>
> >>> The only difference here is that icc_bridge attachment will now be done
> >>> after x86_cpu_parse_featurestr() is called. But this shouldn't make any
> >>> difference, as property setters shouldn't depend on icc_bridge.
> >>>
> >>> Signed-off-by: Eduardo Habkost <address@hidden>
> >>> ---
> >>>  hw/i386/pc.c      |  6 +++++-
> >>>  target-i386/cpu.c | 14 ++------------
> >>>  target-i386/cpu.h |  3 +--
> >>>  3 files changed, 8 insertions(+), 15 deletions(-)
> >>>
> >>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >>> index ed54d93..66b9fa6 100644
> >>> --- a/hw/i386/pc.c
> >>> +++ b/hw/i386/pc.c
> >>> @@ -995,12 +995,16 @@ static X86CPU *pc_new_cpu(const char *cpu_model, 
> >>> int64_t apic_id,
> >>>      X86CPU *cpu;
> >>>      Error *local_err = NULL;
> >>>  
> >>> -    cpu = cpu_x86_create(cpu_model, icc_bridge, &local_err);
> >>> +    cpu = cpu_x86_create(cpu_model, &local_err);
> >>>      if (local_err != NULL) {
> >>>          error_propagate(errp, local_err);
> >>>          return NULL;
> >>>      }
> >>>  
> >>> +    assert(icc_bridge);
> >>
> >> On second thoughts, why are you asserting here rather than setting errp?
> >> Just add an out: below and goto out, like I did.
> >>
> >> On startup it doesn't matter much, but for hot-add asserting would not
> >> be so nice.
> > 
> > Because not having icc_bus passed as argument would be a coding error.
> > 
> > Also, I have no idea what kind of things would break if we destroy a CPU
> > after cpu_exec_init() was already called in instance_init.
> 
> Then do it before cpu_x86_create()! :)
> 
> Also, every memory allocation failure can result in an assertion (which
> is why I'm trying to cut down on their number).

I still think assert() is good enough (and simpler) if it's a coding
error that should never happen in the first place, but I will send a new
version that moves the existing error_setg() call from cpu.c to pc.c.

-- 
Eduardo