From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wei Liu Subject: Re: [PATCH v2 04/13] libxc: Implementation of XEN_XSPLICE_op in libxc (v4). Date: Tue, 19 Jan 2016 11:14:20 +0000 Message-ID: <20160119111420.GO1691@citrix.com> References: <1452808031-706-1-git-send-email-konrad.wilk@oracle.com> <1452808031-706-5-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 1aLUEz-0004dL-6n for xen-devel@lists.xenproject.org; Tue, 19 Jan 2016 11:14:25 +0000 Content-Disposition: inline In-Reply-To: <1452808031-706-5-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: wei.liu2@citrix.com, ian.campbell@citrix.com, andrew.cooper3@citrix.com, ian.jackson@eu.citrix.com, mpohlack@amazon.com, ross.lagerwall@citrix.com, stefano.stabellini@citrix.com, jbeulich@suse.com, sasha.levin@oracle.com, xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org On Thu, Jan 14, 2016 at 04:47:02PM -0500, Konrad Rzeszutek Wilk wrote: [...] > +int xc_xsplice_upload(xc_interface *xch, > + char *name, > + char *payload, > + uint32_t size) > +{ > + int rc; > + DECLARE_SYSCTL; > + DECLARE_HYPERCALL_BOUNCE(payload, size, XC_HYPERCALL_BUFFER_BOUNCE_IN); > + DECLARE_HYPERCALL_BOUNCE(name, 0 /* adjust later */, XC_HYPERCALL_BUFFER_BOUNCE_IN); > + xen_xsplice_name_t def_name = { .pad = { 0, 0, 0 } }; > + > + if ( !name || !payload ) > + return -1; > + > + def_name.size = strlen(name); > + if ( def_name.size > XEN_XSPLICE_NAME_SIZE ) > + return -1; > + > + HYPERCALL_BOUNCE_SET_SIZE(name, def_name.size ); > + > + if ( xc_hypercall_bounce_pre(xch, name) ) > + return -1; > + > + if ( xc_hypercall_bounce_pre(xch, payload) ) > + return -1; > + xc_hypercall_bounce_pre can allocate memory so please clean up after failure instead of returning -1 directly. > + sysctl.cmd = XEN_SYSCTL_xsplice_op; > + sysctl.u.xsplice.cmd = XEN_SYSCTL_XSPLICE_UPLOAD; > + sysctl.u.xsplice.pad = 0; > + sysctl.u.xsplice.u.upload.size = size; > + set_xen_guest_handle(sysctl.u.xsplice.u.upload.payload, payload); > + > + sysctl.u.xsplice.u.upload.name = def_name; > + set_xen_guest_handle(sysctl.u.xsplice.u.upload.name.name, name); > + > + rc = do_sysctl(xch, &sysctl); > + > + xc_hypercall_bounce_post(xch, payload); > + xc_hypercall_bounce_post(xch, name); > + > + return rc; > +} > + > +int xc_xsplice_get(xc_interface *xch, > + char *name, > + xen_xsplice_status_t *status) > +{ > + int rc; > + DECLARE_SYSCTL; > + DECLARE_HYPERCALL_BOUNCE(name, 0 /*adjust later */, XC_HYPERCALL_BUFFER_BOUNCE_IN); > + xen_xsplice_name_t def_name = { .pad = { 0, 0, 0 } }; > + > + if ( !name ) > + return -1; > + > + def_name.size = strlen(name); > + if ( def_name.size > XEN_XSPLICE_NAME_SIZE ) > + return -1; > + > + HYPERCALL_BOUNCE_SET_SIZE(name, def_name.size ); > + > + if ( xc_hypercall_bounce_pre(xch, name) ) > + return -1; > + > + sysctl.cmd = XEN_SYSCTL_xsplice_op; > + sysctl.u.xsplice.cmd = XEN_SYSCTL_XSPLICE_GET; > + sysctl.u.xsplice.pad = 0; > + > + sysctl.u.xsplice.u.get.status.state = 0; > + sysctl.u.xsplice.u.get.status.rc = 0; > + > + sysctl.u.xsplice.u.get.name = def_name; > + set_xen_guest_handle(sysctl.u.xsplice.u.get.name.name, name); > + > + rc = do_sysctl(xch, &sysctl); > + > + xc_hypercall_bounce_post(xch, name); > + > + memcpy(status, &sysctl.u.xsplice.u.get.status, sizeof(*status)); > + > + return rc; > +} > + > +int xc_xsplice_list(xc_interface *xch, unsigned int max, unsigned int start, > + xen_xsplice_status_t *info, > + char *name, uint32_t *len, > + unsigned int *done, > + unsigned int *left) Can you please add some comment before this function to document what each of the parameters means? I have to admit I fail to grok the algorithm of this function. > +{ > + int rc; > + DECLARE_SYSCTL; > + DECLARE_HYPERCALL_BOUNCE(info, 0 /* adjust later. */, XC_HYPERCALL_BUFFER_BOUNCE_OUT); > + DECLARE_HYPERCALL_BOUNCE(name, 0 /* adjust later. */, XC_HYPERCALL_BUFFER_BOUNCE_OUT); > + DECLARE_HYPERCALL_BOUNCE(len, 0 /* adjust later. */, XC_HYPERCALL_BUFFER_BOUNCE_OUT); Lines too long. > + uint32_t max_batch_sz, nr; > + uint32_t version = 0, retries = 0; > + uint32_t adjust = 0; > + > + if ( !max || !info || !name || !len ) > + return -1; > + > + sysctl.cmd = XEN_SYSCTL_xsplice_op; > + sysctl.u.xsplice.cmd = XEN_SYSCTL_XSPLICE_LIST; > + sysctl.u.xsplice.pad = 0; > + sysctl.u.xsplice.u.list.version = 0; > + sysctl.u.xsplice.u.list.idx = start; > + sysctl.u.xsplice.u.list.pad = 0; > + > + max_batch_sz = max; > + > + *done = 0; > + *left = 0; > + do { > + if ( adjust ) > + adjust = 0; /* Used when adjusting the 'max_batch_sz' or 'retries'. */ > + > + nr = min(max - *done, max_batch_sz); > + > + sysctl.u.xsplice.u.list.nr = nr; > + /* Fix the size (may vary between hypercalls). */ > + HYPERCALL_BOUNCE_SET_SIZE(info, nr * sizeof(*info)); > + HYPERCALL_BOUNCE_SET_SIZE(name, nr * sizeof(*name) * XEN_XSPLICE_NAME_SIZE); Line too long. > + HYPERCALL_BOUNCE_SET_SIZE(len, nr * sizeof(*len)); > + /* Move the pointer to proper offset into 'info'. */ > + (HYPERCALL_BUFFER(info))->ubuf = info + *done; > + (HYPERCALL_BUFFER(name))->ubuf = name + (sizeof(*name) * XEN_XSPLICE_NAME_SIZE * *done); > + (HYPERCALL_BUFFER(len))->ubuf = len + *done; > + /* Allocate memory. */ > + rc = xc_hypercall_bounce_pre(xch, info); > + if ( rc ) > + return rc; > + > + rc = xc_hypercall_bounce_pre(xch, name); > + if ( rc ) > + { > + xc_hypercall_bounce_post(xch, info); > + return rc; > + } > + rc = xc_hypercall_bounce_pre(xch, len); > + if ( rc ) > + { > + xc_hypercall_bounce_post(xch, info); > + xc_hypercall_bounce_post(xch, name); > + return rc; > + } Can you just use "break" instead of three "return"s? That should simplify code a lot. > + set_xen_guest_handle(sysctl.u.xsplice.u.list.status, info); > + set_xen_guest_handle(sysctl.u.xsplice.u.list.name, name); > + set_xen_guest_handle(sysctl.u.xsplice.u.list.len, len); > + > + rc = do_sysctl(xch, &sysctl); > + /* > + * From here on we MUST call xc_hypercall_bounce. If rc < 0 we > + * end up doing it (outside the loop), so using a break is OK. > + */ > + if ( rc < 0 && errno == E2BIG ) > + { > + if ( max_batch_sz <= 1 ) > + break; > + max_batch_sz >>= 1; > + adjust = 1; /* For the loop conditional to let us loop again. */ > + /* No memory leaks! */ > + xc_hypercall_bounce_post(xch, info); > + xc_hypercall_bounce_post(xch, name); > + xc_hypercall_bounce_post(xch, len); > + continue; > + } > + else if ( rc < 0 ) /* For all other errors we bail out. */ > + break; > + > + if ( !version ) > + version = sysctl.u.xsplice.u.list.version; > + > + if ( sysctl.u.xsplice.u.list.version != version ) > + { > + /* TODO: retries should be configurable? */ > + if ( retries++ > 3 ) > + { > + rc = -1; > + errno = EBUSY; > + break; > + } > + *done = 0; /* Retry from scratch. */ > + version = sysctl.u.xsplice.u.list.version; > + adjust = 1; /* And make sure we continue in the loop. */ > + /* No memory leaks. */ > + xc_hypercall_bounce_post(xch, info); > + xc_hypercall_bounce_post(xch, name); > + xc_hypercall_bounce_post(xch, len); > + continue; > + } > + > + /* We should never hit this, but just in case. */ > + if ( rc > nr ) > + { > + errno = EINVAL; /* Overflow! */ > + rc = -1; > + break; > + } > + *left = sysctl.u.xsplice.u.list.nr; /* Total remaining count. */ > + /* Copy only up 'rc' of data' - we could add 'min(rc,nr) if desired. */ > + HYPERCALL_BOUNCE_SET_SIZE(info, (rc * sizeof(*info))); > + HYPERCALL_BOUNCE_SET_SIZE(name, (rc * sizeof(*name) * XEN_XSPLICE_NAME_SIZE)); Line too long. > + HYPERCALL_BOUNCE_SET_SIZE(len, (rc * sizeof(*len))); > + /* Bounce the data and free the bounce buffer. */ > + xc_hypercall_bounce_post(xch, info); > + xc_hypercall_bounce_post(xch, name); > + xc_hypercall_bounce_post(xch, len); > + /* And update how many elements of info we have copied into. */ > + *done += rc; > + /* Update idx. */ > + sysctl.u.xsplice.u.list.idx = *done; > + } while ( adjust || (*done < max && *left != 0) ); > + > + if ( rc < 0 ) > + { > + xc_hypercall_bounce_post(xch, len); > + xc_hypercall_bounce_post(xch, name); > + xc_hypercall_bounce_post(xch, info); > + } > + > + return rc > 0 ? 0 : rc; > +} > + > +static int _xc_xsplice_action(xc_interface *xch, > + char *name, > + unsigned int action, > + uint32_t timeout) > +{ > + int rc; > + DECLARE_SYSCTL; > + DECLARE_HYPERCALL_BOUNCE(name, 0 /* adjust later */, XC_HYPERCALL_BUFFER_BOUNCE_IN); Line too long. Wei.