Re: [libvirt] [PATCHv5 2/7] storage: Implement file storage APIs in the default storage driver

On 02/11/2014 09:26 AM, Peter Krempa wrote: I tend to agree with Dan's comment on patch 1 (that is, exposing these directly in storage_driver.c instead of making it go through the driver.h interface), which may have some fallout on this patch. That said, I'll review this one. At first I wondered why an empty array, then I realized - this is the framework, then later patches add in new backends. Okay. No need to copy existing bad examples; please document which of these callbacks (if any) must be non-NULL, or explicitly state that all callbacks are optional. (When I first added the gluster backend, I had a hard time figuring out which callbacks storage_driver expected to be able to use). It would also be nice to document the contract placed on implementations of each callback (such as when a callback should return -1 vs -2, or whether errno must be set on error), at the point where you declare typedefs for each callback signature. This patch looks okay in isolation for driving the new callbacks, once later patches implement them. ACK with comments added. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org