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@xxxxxxxxxxxxxx> > >>> 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@xxxxxxxxxxxxxx> > >>> --- > >>> 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list