Re: [PATCH qemu v10] spapr: Implement Open Firmware client interface

[BALATON Zoltan]

On Mon, 7 Dec 2020, Alexey Kardashevskiy wrote:
> On 05/12/2020 05:32, Greg Kurz wrote:
> > On Tue, 13 Oct 2020 13:19:11 +1100
> > Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> >
> > > +static void readstr(hwaddr pa, char *buf, int size)
> > > +{
> > > +    cpu_physical_memory_read(pa, buf, size);
> > > +    if (buf[size - 1] != '\0') {
> > > +        buf[size - 1] = '\0';
> > > +        if (strlen(buf) == size - 1) {
> > > +            trace_spapr_of_client_error_str_truncated(buf, size);
> > > +        }
> > > +    }
> > > +}
> > > +
> > > +static bool cmpservice(const char *s, size_t len,
> > > +                       unsigned nargs, unsigned nret,
> > > +                       const char *s1, size_t len1,
> > > +                       unsigned nargscheck, unsigned nretcheck)
> > > +{
> > > +    if (strcmp(s, s1)) {
> > > +        return false;
> > > +    }
> > > +    if ((nargscheck && (nargs != nargscheck)) ||
> > > +        (nretcheck && (nret != nretcheck))) {
> >
> > Parenthesitis : != has precedence over &&.
>
>
> I love my braces :)

Then keep them for yourself and not leave them around in code :-) I prefer 
minimum parenthesis too as that's easier to read and you can always look 
up operator precedence if in doubt so I'd vote for dropping them unless 
really needed or the compiler complains about it.

> > > +static uint32_t of_client_setprop(SpaprMachineState *spapr,
> > > +                                  uint32_t nodeph, uint32_t pname,
> > > +                                  uint32_t valaddr, uint32_t vallen)
> > > +{
> > > +    char propname[OF_PROPNAME_LEN_MAX + 1];
> > > +    uint32_t ret = -1;
> > > +    int offset;
> > > +    char trval[64] = "";
> > > +
> > > +    readstr(pname, propname, sizeof(propname));
> > > +    /*
> > > +     * We only allow changing properties which we know how to update on
> > > +     * the QEMU side.
> > > +     */
> > > +    if (vallen == sizeof(uint32_t)) {
> > > +        uint32_t val32 = ldl_be_phys(first_cpu->as, valaddr);
> > > +
> > > +        if ((strcmp(propname, "linux,rtas-base") == 0) ||
> > > +            (strcmp(propname, "linux,rtas-entry") == 0)) {
> >
> > What about :
> >
> >          if (!strcmp(propname, "linux,rtas-base") ||
> >              !strcmp(propname, "linux,rtas-entry")) {
>
>
>
> [fstn1-p1 qemu-killslof]$ git grep 'strcmp.*==.*0' | wc -l
> 426
> [fstn1-p1 qemu-killslof]$ git grep '!strcmp' | wc -l
> 447
>
> My team is losing but not by much :) I prefer "==" (literally - "equal) to 
> "!" with "cmp" which is "does not compare" (makes little sense).

This may be personal preference but I also prefer using !strcmp for match 
(and less parenthesis :-) ) and have explicit != 0 for checking for not 
matching which is the less common. You had a few checks for not match 
above as "if (strcmp())"which now can be mistaken for checking for match. 
Using !strcmp might take a bit getting used to but once you do it's 
shorted and clearer.

Anyway, neither of these are strong points just a few comments. Sorry I 
did not have a chance to try the whole patch yet.

Regards,
BALATON Zoltan