Re: [PATCH v3 00/32] implement vNVDIMM

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

 





On 10/13/2015 01:57 PM, Michael S. Tsirkin wrote:
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.

Ah, got it. :)
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux