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

[Thomas Huth <thuth@redhat.com>]

On 07/12/2020 11.26, BALATON Zoltan via wrote:
> 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.

+1

Too many braces always rather confuse me than being helpful.

>>>> +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 :-) )

Easy solution: Simply use the glib variant g_str_equal() instead - that's
way much easier to understand while reading the code!

 Thomas