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. -- Eric Blake eblake@xxxxxxxxxx +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list