Re: [libvirt] [PATCH 1/5] report OOMError in virDomainDiskInsert()

On 03/15/2011 07:13 PM, Hu Tao wrote: Right now, it only returns -1 for OOM (it's only a 4-line method, so it's easy to audit that statement for truth), and there's only a single caller. A valid justification would be that later in this patch series you plan on adding code to virDomainDiskInsert that returns -1 for other failures, in which case moving the error reporting into virDomainDiskInsert is absolutely the right thing to do. Or even just arguing the refactoring point of view: a patch that adds 4 lines and subtracts 4 lines is hard to see how it avoids duplicated code, whereas a refactoring that adds 4 lines and subtracts 8 is easier to spot as a useful consolidation. But I'm missing all of that justification in the commit comments - is there a plan for a later patch to be changing semantics? Is it for consistency with other functions in the same file that have similar semantics of reporting OOM on failure rather than delegating to the caller? Is it a refactoring designed to reduce duplication? In other words, code motion just for the sake of it is hard to understand (it doesn't necessarily make it wrong, but certainly delays the review), but code motion with a stated reason is much easier to ack. Even something like "future patches will add more callers to virDomainDiskInsert, so doing the code motion now will reduce code duplication of OOM reporting later" would help. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org