Re: [Qemu-devel] [PATCH v3 00/32] implement vNVDIMM

[Michael S. Tsirkin]

On Tue, Oct 13, 2015 at 01:29:48PM +0800, Xiao Guangrong wrote:
> 
> 
> On 10/12/2015 07:55 PM, Michael S. Tsirkin wrote:
> >On Sun, Oct 11, 2015 at 11:52:32AM +0800, Xiao Guangrong wrote:
> >>Changelog in v3:
> >>There is huge change in this version, thank Igor, Stefan, Paolo, Eduardo,
> >>Michael for their valuable comments, the patchset finally gets better shape.
> >
> >Thanks!
> >This needs some changes in coding style, and more comments, to
> >make it easier to maintain going forward.
> 
> Thanks for your review, Michael. I have learned lots of thing from
> your comments.
> 
> >
> >High level comments - I didn't point out all instances,
> >please go over code and locate them yourself.
> >I focused on acpi code in this review.
> 
> Okay, will do.
> 
> >
> >     - fix coding style violations, prefix eveything with nvdimm_ etc
> 
> Actually i did not pay attention on naming the stuff which is only internally
> used. Thank you for pointing it out and will fix it in next version.
> 
> >     - in apci code, avoid manual memory management/complex pointer math
> 
> I am not very good at ACPI ASL/AML, could you please more detail?

It's about C.

For example:
        Foo *foo = acpi_data_push(table, sizeof *foo);
        Bar *foo = acpi_data_push(table, sizeof *bar);
is pretty obviously safe, and it doesn't require you to do any
calculations.
        char *buf = acpi_data_push(table, sizeof *foo + sizeof *bar);
is worse, now you need:
        Bar *bar = (Bar *)(buf + sizeof *foo);
which will corrupt memory if you get the size wrong in push.

> >     - comments are needed to document apis & explain what's going on
> >     - constants need comments too, refer to text that
> >       can be looked up in acpi spec verbatim
> 
> Indeed, will document carefully.