Re: [Qemu-devel] [PATCH 2/6] target-i386: cpu: separate feature string parsing from CPU model lookup

[Andreas Färber]

Am 04.12.2012 20:34, schrieb Eduardo Habkost:
> Instead of using parsing the whole cpu_model string inside

Dropped "using".

> cpu_x86_find_by_name(), first split it into the CPU model name and the
> full feature string, then parse the feature string into pieces.
[...]
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 7afe839..70ba323 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1208,25 +1208,10 @@ static void x86_cpuid_set_tsc_freq(Object *obj, 
> Visitor *v, void *opaque,
>      cpu->env.tsc_khz = value / 1000;
>  }
>  
> -static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char 
> *cpu_model)
> +static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
>  {
> -    unsigned int i;
>      x86_def_t *def;
>  
> -    char *s = g_strdup(cpu_model);
> -    char *featurestr, *name = strtok(s, ",");
> -    /* Features to be added*/
[...]
> @@ -1235,16 +1220,37 @@ static int cpu_x86_find_by_name(x86_def_t 
> *x86_cpu_def, const char *cpu_model)
>      if (kvm_enabled() && name && strcmp(name, "host") == 0) {
>          kvm_cpu_fill_host(x86_cpu_def);
>      } else if (!def) {
> -        goto error;
> +        return -1;
>      } else {
>          memcpy(x86_cpu_def, def, sizeof(*def));
>      }
>  
> +    return 0;
> +}
> +
> +/* Parse "+feature,-feature,feature=foo" CPU feature string
> + */
> +static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
> +{
> +    unsigned int i;
> +    char *featurestr; /* Single 'key=value" string being parsed */
> +    /* Features to be added*/

I took the liberty of adding a space above by editing patches 1 & 2.

Also, in Linux, multi-level topics such as "PPC: KVM: " seem to be
common, whereas in QEMU we've been using paths ("tcg/ppc: "); Anthony
has once used "isa: pic: " when he meant affecting both isa and pic
(8f04ee0882aec9fe91fb70f767edf5dacff59835), so since this does not touch
on the QOM cpu.c that I usually label "cpu: ", in a previous case I have
edited your patch as "target-i386/cpu: ", but since this subject
redundantly mentions CPU later on I'm dropping it. Hope this explains
the rationale! Generally I found the GNOME guidelines pretty convincing:
https://live.gnome.org/Git/CommitMessages
(capitalizing after the lowercase topic makes the main message stand out
when reading through shortlogs IMO)

Thanks, applied patches 1-2 to qom-cpu:
https://github.com/afaerber/qemu-cpu/commits/qom-cpu

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg