From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v2 1/5] libxl: Add ERROR_NOTFOUND for libxl_domain_info when it cannot find the domain Date: Wed, 18 Mar 2015 13:03:28 +0000 Message-ID: <1426683808.14291.12.camel@citrix.com> References: <1426278403-12959-1-git-send-email-konrad.wilk@oracle.com> <1426278403-12959-2-git-send-email-konrad.wilk@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YYDdr-00053E-Cx for xen-devel@lists.xenproject.org; Wed, 18 Mar 2015 13:04:11 +0000 In-Reply-To: <1426278403-12959-2-git-send-email-konrad.wilk@oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Konrad Rzeszutek Wilk Cc: xen-devel@lists.xenproject.org, ian.jackson@eu.citrix.com, wei.liu2@citrix.com, stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org On Fri, 2015-03-13 at 16:26 -0400, Konrad Rzeszutek Wilk wrote: > And use that for all of its callers in the tree. I wonder -- is it better to have a generic ERROR_NOTFOUND for anything which is not found, or to have a specific ERROR_DOMAIN_NOTFOUND (likewise for other things). Any opinions? I'm inclined towards the latter, e.g. imagine disk_remove(domid, disk) -- ERROR_NOTFOUND doesn't tell you if the disk or the domain is missing. (this example may or may not be hypothetical, I didn't chekc). > @@ -454,6 +454,8 @@ int libxl__domain_rename(libxl__gc *gc, uint32_t domid, > > /* update /vm//name */ > rc = libxl_domain_info(ctx, &info, domid); > + if (rc == ERROR_NOTFOUND) > + goto x_rc; > if (rc) > goto x_fail; Might as well us x_rc for all failures, there seem little point in squashing whatever libxl_domain_info returned into ERROR_FAIL for the other cases either. > @@ -5415,11 +5417,12 @@ static int libxl__set_vcpuonline_xenstore(libxl__gc *gc, uint32_t domid, > libxl_dominfo info; > char *dompath; > xs_transaction_t t; > - int i, rc = ERROR_FAIL; > + int i, rc; > > libxl_dominfo_init(&info); > > - if (libxl_domain_info(CTX, &info, domid) < 0) { > + rc = libxl_domain_info(CTX, &info, domid); > + if (rc < 0) { > LOGE(ERROR, "getting domain info list"); > goto out; > } I think rc needs setting to ERROR_FAIL after this to retain the same behaviour for the subsequent failures of e.g. libxl__xs_get_dompath which doesn't otherwise set rc and previous relied on the ERROR_FAIL from above. > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h > index 5eec092..5164371 100644 > --- a/tools/libxl/libxl.h > +++ b/tools/libxl/libxl.h > @@ -1088,7 +1088,9 @@ int libxl_console_get_tty(libxl_ctx *ctx, uint32_t domid, int cons_num, > */ > int libxl_primary_console_get_tty(libxl_ctx *ctx, uint32_t domid_vm, char **path); > > -/* May be called with info_r == NULL to check for domain's existance */ > +/* May be called with info_r == NULL to check for domain's existance. Could you fix the spelling of "existence" while you touch this line please ;-) > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > index 47af340..69a91cc 100644 > --- a/tools/libxl/libxl_types.idl > +++ b/tools/libxl/libxl_types.idl > @@ -63,6 +63,7 @@ libxl_error = Enumeration("error", [ > (-17, "DEVICE_EXISTS"), > (-18, "REMUS_DEVOPS_DOES_NOT_MATCH"), > (-19, "REMUS_DEVICE_NOT_SUPPORTED"), > + (-20, "NOTFOUND"), > ], value_namespace = "") Do we need a #define LIBXL_HAVE_ERROR_NOTFOUND to indicate this is available, I think we probably do. > @@ -7495,7 +7495,8 @@ int main_cpupoolnumasplit(int argc, char **argv) > goto out; > } > for (c = 0; c < 10; c++) { > - if (libxl_domain_info(ctx, &info, 0)) { > + ret = -libxl_domain_info(ctx, &info, 0); > + if (ret) { I think we don't need to do this bit. (The current inconsistent exit codes form xl notwithstanding)