Re: [Qemu-devel] [PATCH v3 3/3] Change other funcitons referring to feature_word_info[]

[Robert Hoo]

On Fri, 2018-08-10 at 10:17 -0500, Eric Blake wrote:
> On 08/10/2018 09:06 AM, Robert Hoo wrote:
> 
> In the subject: s/funcitons/functions/
> 
> Also, it may be worth using a topic prefix (most of our commit messages 
> resemble:
> 
> topic: Description of patch
> 
> to make it easier to spot patches by topic).
> 
> > Add an util function feature_word_description(), which help construct the 
> > string
> 
> s/an util/a util/
> s/help/helps/
> 
Thanks Eric, will fix these typos in v2.
> > describing the feature word (both CPUID and MSR types).
> > 
> > report_unavailable_features(): add MSR_FEATURE_WORD type support.
> > x86_cpu_get_feature_words(): limit to CPUID_FEATURE_WORD only.
> > x86_cpu_get_supported_feature_word(): add MSR_FEATURE_WORD type support.
> > x86_cpu_adjust_feat_level(): assert the requested feature must be
> > CPUID_FEATURE_WORD type.
> > 
> > Signed-off-by: Robert Hoo <address@hidden>
> > ---
> >   target/i386/cpu.c | 77 
> > +++++++++++++++++++++++++++++++++++++++++--------------
> >   1 file changed, 58 insertions(+), 19 deletions(-)
> > 
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index f7c70d9..21ed599 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -3024,21 +3024,51 @@ static const TypeInfo host_x86_cpu_type_info = {
> >   
> >   #endif
> >   
> > +/*
> > +*caller should have input str no less than 64 byte length.
> > +*/
> > +#define FEATURE_WORD_DESCPTION_LEN 64
> 
> s/DESCPTION/DESCRIPTION/
> 
> > +static int feature_word_description(char str[], FeatureWordInfo *f,
> > +                                            uint32_t bit)
> > +{
> 
> Prone to buffer overflow if the caller doesn't pass in the length. 
> Would it be better to just g_strdup_printf() into a malloc'd result 
> instead of trying to snprintf()'ing into a buffer that you hope the 
> caller sized large enough?
> 
Oh, wasn't aware of such good util functions. Sure I'd like to use them.
Is there some web page introducing them?

Eduardo/Paolo, do you have more comments?
> > +    int ret;
> > +
> > +    assert(f->type == CPUID_FEATURE_WORD ||
> > +        f->type == MSR_FEATURE_WORD);
> > +    switch (f->type) {
> > +    case CPUID_FEATURE_WORD:
> > +        {
> > +            const char *reg = get_register_name_32(f->cpuid.reg);
> > +            assert(reg);
> > +            ret = snprintf(str, FEATURE_WORD_DESCPTION_LEN,
> > +                "CPUID.%02XH:%s%s%s [bit %d]",
> > +                f->cpuid.eax, reg,
> > +                f->feat_names[bit] ? "." : "",
> > +                f->feat_names[bit] ? f->feat_names[bit] : "", bit);
> > +            break;
> > +        }
> > +    case MSR_FEATURE_WORD:
> > +        ret = snprintf(str, FEATURE_WORD_DESCPTION_LEN,
> > +            "MSR(%xH).%s [bit %d]",
> > +            f->msr.index,
> > +            f->feat_names[bit] ? f->feat_names[bit] : "", bit);
> > +        break;
> > +    }
>