From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Ian Campbell <ian.campbell@citrix.com>,
Wei Liu <wei.liu2@citrix.com>,
Xen-devel <xen-devel@lists.xenproject.org>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>,
David Scott <dave.scott@eu.citrix.com>
Subject: Re: [PATCH for-4.6 12/13] tools/ocaml: call libxl_dominfo_{init, dispose} in stub
Date: Thu, 23 Jul 2015 11:00:05 +0100 [thread overview]
Message-ID: <55B0BB25.7030600@citrix.com> (raw)
In-Reply-To: <1437645310.19412.87.camel@citrix.com>
On 23/07/15 10:55, Ian Campbell wrote:
> On Thu, 2015-07-23 at 09:32 +0100, Andrew Cooper wrote:
>> On 23/07/2015 08:59, Wei Liu wrote:
>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>>> ---
>>> Cc: David Scott <dave.scott@eu.citrix.com>
>>>
>>> As far as I can tell, all Val_$foo function does deep-copy, so we
>>> can
>>> safely call dispose in said function.
>> Sadly this is insufficient. failwith_xl() longjump()s back into the
>> ocaml runtime, which ends up leaking any allocations made for
>> dominfo.
>>
>> This is a systemic problem with the Ocaml bindings and I have a
>> proposed
>> solution but it involves rewriting quite a lot of this code and is
>> definitely not 4.6 material.
> Is it not sufficient to treat failwith_xl as a longjump statement (or
> any sort of "return-y" thing), which would simply necessitate doing the
> cleanup before calling it?
>
> Perhaps Coverity could model it as such and would therefore warn about
> the dead code being added here?
>
Part of my Ocaml series is to properly mark failwith_xl() as a Noreturn
function. Currently as far as the compiler and Coverity can tell,
failwith_xl() may return normally.
While it is possible to rearrange this code to avoid leaking in the ret
!= 0 case, it is not possible to rearrange it to avoid leaking if
Val_dominfo() uses failwith_xl()/caml_out_of_memory() itself.
The solution I have in mind is to wrap all libxl IDL objects in Ocaml
Custom blocks, which allows the Ocaml runtime to garbage collect them.
~Andrew
next prev parent reply other threads:[~2015-07-23 10:15 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-23 7:59 [PATCH for-4.6 00/13] tools: fixes inspired by Coverity scan Wei Liu
2015-07-23 7:59 ` [PATCH for-4.6 01/13] libxl: use thread-safe localtime_t and handle NULL Wei Liu
2015-07-23 9:14 ` Ian Campbell
2015-07-23 9:34 ` Wei Liu
2015-07-23 7:59 ` [PATCH for-4.6 02/13] libxl: properly clean up array in libxl_list_cpupool failure path Wei Liu
2015-07-23 9:22 ` Ian Campbell
2015-07-23 14:29 ` Dario Faggioli
2015-07-23 7:59 ` [PATCH for-4.6 03/13] xl/libxl: remove a bunch of pointless assignments Wei Liu
2015-07-23 9:25 ` Ian Campbell
2015-07-23 7:59 ` [PATCH for-4.6 04/13] xl: free pid string in do_daemonize Wei Liu
2015-07-23 9:34 ` Ian Campbell
2015-07-23 7:59 ` [PATCH for-4.6 05/13] xl: check json string is not null before printing in create_domain Wei Liu
2015-07-23 9:36 ` Ian Campbell
2015-07-23 7:59 ` [PATCH for-4.6 06/13] xl: call libxl_dominfo_{init, dispose} in psr_cmt_show Wei Liu
2015-07-23 9:36 ` Ian Campbell
2015-07-23 7:59 ` [PATCH for-4.6 07/13] xl: call libxl_dominfo_{init, dispose} in main_cpupoolnumasplit Wei Liu
2015-07-23 9:41 ` Ian Campbell
2015-07-23 10:34 ` Wei Liu
2015-07-23 7:59 ` [PATCH for-4.6 08/13] xl: call libxl_dominfo_init in main_list Wei Liu
2015-07-23 9:44 ` Ian Campbell
2015-07-23 7:59 ` [PATCH for-4.6 09/13] xl: call libxl_bitmap_{init, dispose} in main_cpupoolcreate Wei Liu
2015-07-23 9:46 ` Ian Campbell
2015-07-23 7:59 ` [PATCH for-4.6 10/13] xl: valid fd can be 0 in main_loadpolicy Wei Liu
2015-07-23 9:48 ` Ian Campbell
2015-07-23 7:59 ` [PATCH for-4.6 11/13] xl: free event struct after use in main_shutdown_or_reboot Wei Liu
2015-07-23 9:52 ` Ian Campbell
2015-07-23 7:59 ` [PATCH for-4.6 12/13] tools/ocaml: call libxl_dominfo_{init, dispose} in stub Wei Liu
2015-07-23 8:32 ` Andrew Cooper
2015-07-23 8:38 ` Wei Liu
2015-07-23 9:07 ` Andrew Cooper
2015-07-23 9:55 ` Ian Campbell
2015-07-23 10:00 ` Andrew Cooper [this message]
2015-07-23 7:59 ` [PATCH for-4.6 13/13] tools/ocaml: handle strdup failure in stub_xl_device_disk_of_vdev Wei Liu
2015-07-23 8:38 ` Andrew Cooper
2015-07-23 9:12 ` Dave Scott
2015-07-23 9:06 ` Dave Scott
2015-07-24 11:06 ` [PATCH for-4.6 00/13] tools: fixes inspired by Coverity scan Ian Campbell
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55B0BB25.7030600@citrix.com \
--to=andrew.cooper3@citrix.com \
--cc=Ian.Jackson@eu.citrix.com \
--cc=dave.scott@eu.citrix.com \
--cc=ian.campbell@citrix.com \
--cc=wei.liu2@citrix.com \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).