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

[Hu Tao]

On Tue, Mar 15, 2011 at 07:29:24PM -0600, Eric Blake wrote:
> 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.

There is another call of virDomainDiskInsert() in patch 4 of this series.

-- 
Thanks,
Hu Tao