Re: [libvirt] [PATCH] Split up platfrom specifics from bridge driver

On 07/04/2013 10:25 AM, Roman Bogorodskiy wrote: I've let review sit for so long that it no longer applies, and needs a rebasing; but I'll at least skim the patch looking for potential issues to be aware of during the rebase... If you generate the email with 'git send-email/format-patch -O/path/to/file', then you can use a file that prioritizes .h files to come first in the diff listing; this can sometimes help in interface reviews. You moved this struct out of a .c file into driver_platform.h; as a result, this poor choice of naming will potentially cause problems to more files that include the .h. Can you please do a separate cleanup pre-patch that s/network_driver/virNetworkDriver/, and uses a typedef so that you don't have to repeat 'struct' everywhere? Also, it might be worth a patch to make this struct inherit from virObjectLockable, instead of open-coding the locking ourselves. But that's separate from your goal of splitting the files, so it's not a prerequisite. For ease of review, it might be worth splitting the function renames into a separate patch from the code motion. One .c including another is not typical, but we've done it before for virthread.c; seems like a reasonable way to split things, as long as all branches of the file declare the same functions. The other alternative for splitting things is the way we've done it in src/security - the public interface is in virSecurityManager, and basically forwards all calls into a private interface, where each backend registers a struct of callback pointers. That method is a bit more robust - you can add a callback to one driver without having to remember to add it to all drivers simultaneously. With your approach, if we add a new function in bridge_driver_linux.c, we MUST remember to add the same function in bridge_driver_noop.c, or it will break compilation on non-Linux, but developers that don't use Linux will be unaware of the breakage. With the callback approach, not adding a callback to bridge_driver_noop.c is not fatal; the manager wrapper would simply know to do nothing for a NULL callback. So, although I like the split, I can't help but wonder if your rebase should take the road of adjusting things to use a callback struct, rather than requiring matching implementations across multiple files. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org