Re: [libvirt] [PATCH RFC 3/9] nodedev: Convert virNodeDevObj[List] to use virPoolObj[Table]
On 02/28/2017 10:15 AM, Michal Privoznik wrote:
On 02/11/2017 05:29 PM, John Ferlan wrote:
> Use the virPoolObj[Table] object management model in order to manage the
> Node Device objects.
>
> While making the adjustments to use the new model, there are some code
> formatting adjustments that were also made with the goal to follow more
> recent code flow and layout.
>
> For API's that were static, rather than use "virNode*", some names
were
> converted to be "node*" - makes it easier to determine while reading code
> what is local and what is "outside" the perveyance of the module.
>
> For the node_device_hal.c - removed the usage of privateData - it wasn't
> actually used anyway other than alloc()/free(). The NODE_DEV_UDI was never
> referenced.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> po/POTFILES.in | 1 +
> src/Makefile.am | 3 +-
> src/conf/node_device_conf.c | 497 +-----------------------------
> src/conf/node_device_conf.h | 85 +-----
> src/conf/virnodedeviceobj.c | 570 +++++++++++++++++++++++++++++++++++
> src/conf/virnodedeviceobj.h | 86 ++++++
> src/libvirt_private.syms | 25 +-
> src/node_device/node_device_driver.c | 388 ++++++++++--------------
> src/node_device/node_device_driver.h | 2 +-
> src/node_device/node_device_hal.c | 82 ++---
> src/node_device/node_device_udev.c | 76 +++--
> src/test/test_driver.c | 294 +++++++-----------
> 12 files changed, 1032 insertions(+), 1077 deletions(-)
> create mode 100644 src/conf/virnodedeviceobj.c
> create mode 100644 src/conf/virnodedeviceobj.h
>
I find it very hard to follow this patch. I think three separate things
are happening here: rename of some static functions, moving
virNodeDeviceObjList* functions into a separate file and switching to
new virPoolObj* APIs.
The disclaimer certainly was it wasn't going to be easy. I can work on
trying to split things up better though for future patches.
The other patches are same from this POV. So I'm gonna stop my review here.
Fair enough - thanks for diving into the Pool... Some feedback is
better than none. I'd like to come up with something that is better than
what we have now. I know it won't be simple, but I think we can come to
a consensus over things.
My overall feeling is good. But there are some issues we need to
resolve. Also, I don't know why it is, but I'd expect the actual code to
shrink in size after this patch set (we are merging some code after
all), but that is not the case. I don't know what part to hold accountable.
Comments. and lots of them. Every new module gets the license stuff, a
bunch of includes, etc. Those comment lines add up quickly. Then of
course there's just my verbosity because while some would like to
believe otherwise, not all code is self commenting...
John
Michal