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

[Eric Blake]

On 03/15/2011 07:13 PM, Hu Tao wrote:
> On Tue, Mar 15, 2011 at 03:38:50PM -0600, Eric Blake wrote:
>> On 03/02/2011 06:09 PM, KAMEZAWA Hiroyuki wrote:
>>> >From b92569080a25bf0029d637327a87372bff071fae Mon Sep 17 00:00:00 2001
>>> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu at jp.fujitsu.com>
>>> Date: Thu, 3 Mar 2011 09:20:36 +0900
>>> Subject: [PATCH 1/5] report OOMError in virDomainDiskInsert()
>>>
>>> Now, virDomainDiskInsert() returns -1 at memory allocation
>>> failure but it should call virReportOOMError() by itself.
>>>
>>> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu at jp.fujitsu.com>
>>> ---
>>>  src/conf/domain_conf.c |    4 +++-
>>>  src/xen/xm_internal.c  |    4 +---
>>>  2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> This patch looks accurate, but lacks justification (why can't all
>> callers continue to call virReportOOMError() on failure)?  I'm not sure
> 
> Sure they can, but on the premise that they are sure there is an OOM
> happens. The semantics of virDomainDiskInsert is not clear here, returns
> -1 just means a failure but not an OOM. What if virDomainDiskInsert
> fails for other reasons, is it still correct for the caller to call
> virReportOOMError()?

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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110315/8c2d182d/attachment-0001.sig>