[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [XEN PATCH] x86/guest_walk: address violations of MISRA C:2012 Rule 8.3



On 28/11/23 14:43, Jan Beulich wrote:
On 28.11.2023 14:17, Andrew Cooper wrote:
On 28/11/2023 1:00 pm, Jan Beulich wrote:
On 28.11.2023 10:46, Federico Serafini wrote:
Uniform declaration and definition of guest_walk_tables() using
parameter name "pfec_walk":
this name highlights the connection with PFEC_* constants and it is
consistent with the use of the parameter within function body.
No functional change.

Signed-off-by: Federico Serafini <federico.serafini@xxxxxxxxxxx>
I'm curious what other x86 maintainers think. I for one don't like this,
but not enough to object if others are happy. That said, there was earlier
discussion (and perhaps even a patch), yet without a reference I don't
think I can locate this among all the Misra bits and pieces.

I looked at this and wanted a bit of time to think.

Sadly, this code is half way through some cleanup, which started before
speculation and will continue in my copious free time.

It's wrong to be passing PFEC_* constants, and that's why I renamed pfec
-> walk the last time I was fixing security bugs here  (indeed, passing
the wrong constant here *was* the security issue).  I missed the
prototype while fixing the implementation.

At some point, PFEC_* will no longer be passed in.

Therefore I'd far rather this was a one-line change for the declaration
changing pfec -> walk.

So that was what Federico originally had. Did you see my response at
https://lists.xenproject.org/archives/html/xen-devel/2023-07/msg00122.html ?
While I certainly agree with your plans (as far as I understand them),
doing as you suggest would make it harder to spot what values are correct
to pass to the function today. With a suitable new set of constants and
perhaps even a proper typedef, such confusion would likely not arise.
Thanks to both for the information.

I take this opportunity to inform that we are really close to the end
with Rule 8.3 for x86, this is the situation:
- do_multicall(), Stefano sent a patch;
- guest_walk_tables(), Andrew will take care of it;
- xenmem_add_to_physmap_one(), this is the last one.

For the latter, I see you (x86) share the declaration with ARM,
where "gfn" is used for the last parameter instead of "gpfn".
Do you agree in changing the name in the definition from "gpfn"
to "gfn"?
If you agree, do you have any suggestions on how to rename
the local variable "gfn"?
Following your suggestions, I can do the renaming in two
steps to prevent bad things to happening.

--
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.