Re: [Qemu-devel] [Patch v2 00/29] s390x CPU models: exposing features

[Cornelia Huck]

On Mon, 8 Aug 2016 09:45:04 -0700 (PDT)
address@hidden wrote:

<Trimmed the no-reply@ -- I wonder whether reply-to should be setup to
something sensible, e.g. qemu-devel>

> Checking PATCH 4/29: s390x/cpumodel: introduce CPU features...
> WARNING: line over 80 characters
> #65: FILE: target-s390x/cpu_features.c:27:
> +    FEAT_INIT("zarch", S390_FEAT_TYPE_STFL, 1, "z/Architecture architectural 
> mode"),

We could conceivably break the line after one of the arguments, but
there are some very long strings below. I'm not a fan of very long
lines myself, but I wonder whether we should relax that limit? The
Linux kernel has settled upon a relaxed limit and forbids splitting
strings for greppability.


> Checking PATCH 5/29: s390x/cpumodel: generate CPU feature lists for CPU 
> models...
> ERROR: Macros with complex values should be enclosed in parenthesis
> #113: FILE: target-s390x/gen-features.c:20:
> +#define S390_FEAT_GROUP_PLO \
> +    S390_FEAT_PLO_CL, \
> +    S390_FEAT_PLO_CLG, \
> +    S390_FEAT_PLO_CLGR, \
> +    S390_FEAT_PLO_CLX, \
> +    S390_FEAT_PLO_CS, \
> +    S390_FEAT_PLO_CSG, \
> +    S390_FEAT_PLO_CSGR, \
> +    S390_FEAT_PLO_CSX, \
> +    S390_FEAT_PLO_DCS, \
> +    S390_FEAT_PLO_DCSG, \
> +    S390_FEAT_PLO_DCSGR, \
> +    S390_FEAT_PLO_DCSX, \
> +    S390_FEAT_PLO_CSST, \
> +    S390_FEAT_PLO_CSSTG, \
> +    S390_FEAT_PLO_CSSTGR, \
> +    S390_FEAT_PLO_CSSTX, \
> +    S390_FEAT_PLO_CSDST, \
> +    S390_FEAT_PLO_CSDSTG, \
> +    S390_FEAT_PLO_CSDSTGR, \
> +    S390_FEAT_PLO_CSDSTX, \
> +    S390_FEAT_PLO_CSTST, \
> +    S390_FEAT_PLO_CSTSTG, \
> +    S390_FEAT_PLO_CSTSTGR, \
> +    S390_FEAT_PLO_CSTSTX

I think the check is wrong to complain here.

> Checking PATCH 6/29: s390x/cpumodel: generate CPU feature group lists...
> Checking PATCH 7/29: s390x/cpumodel: introduce CPU feature group 
> definitions...
> WARNING: line over 80 characters
> #71: FILE: target-s390x/cpu_features.c:363:
> +    FEAT_GROUP_INIT("gen13ptff", GEN13_PTFF, "PTFF enhancements introduced 
> with z13"),

Same comments as above.


> Checking PATCH 8/29: s390x/cpumodel: register defined CPU models as 
> subclasses...
> WARNING: line over 80 characters
> #64: FILE: target-s390x/cpu_models.c:30:
> +        .base_feat = { S390_FEAT_LIST_GEN ## _gen ## _GA ## _ec_ga ## _BASE 
> },  \

And here. I honestly don't see a way to get this below 80 chars.


> Checking PATCH 19/29: linux-headers: update against kvm/next...
> ERROR: code indent should never use tabs
> #21: FILE: include/standard-headers/linux/input-event-codes.h:615:
> +#define KEY_RIGHT_UP^I^I^I0x266$

We should not check a linux headers update against qemu coding style.
Either ignore the respective files, or check whether this patch is a
linux headers update and nothing else. (I lack the perl skills for
that :)


> Checking PATCH 24/29: qmp: add QMP interface "query-cpu-model-expansion"...
> WARNING: line over 80 characters
> #29: FILE: include/sysemu/arch_init.h:38:
> +CpuModelExpansionInfo *arch_query_cpu_model_expansion(CpuModelExpansionType 
> type,

This is a case of VeryLongVariableNames. Not sure whether we should do
anything about that.

> Checking PATCH 27/29: s390x/cpumodel: implement QMP interface 
> "query-cpu-model-expansion"...
> WARNING: line over 80 characters
> #152: FILE: target-s390x/cpu_models.c:408:
> +        s390_feat_bitmap_to_ascii(model->features, qdict, 
> qdict_add_enabled_feat);
> 
> WARNING: line over 80 characters
> #165: FILE: target-s390x/cpu_models.c:421:
> +CpuModelExpansionInfo *arch_query_cpu_model_expansion(CpuModelExpansionType 
> type,
> 
> ERROR: space prohibited before that close parenthesis ')'
> #181: FILE: target-s390x/cpu_models.c:437:
> +    } else if (type != CPU_MODEL_EXPANSION_TYPE_FULL ) {

That's actually a warning I concur with :)

I think we should at least come up with a check for a linux header
update to avoid spamming the mailing list. And it would be a good idea
to have some consensus about the 80 char limit as well.