Xen devel - Dec 2009 - [PATCH] libxenlight: clone context to avoid GC corruption

Provide a function to clone a context. This is necessary
because simply copying the structs will eventually
corrup the GC: maxsize is updated in the cloned context
but not in the originating one, yet they have the same
array of referenced pointers alloc_ptrs.

Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Wed, 2 Dec 2009, Andres Lagar-Cavilla wrote:
> Provide a function to clone a context. This is necessary
> because simply copying the structs will eventually
> corrup the GC: maxsize is updated in the cloned context
> but not in the originating one, yet they have the same
> array of referenced pointers alloc_ptrs.
> 
> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.com>
> 
> 
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Thu, 3 Dec 2009, Stefano Stabellini wrote:
> On Wed, 2 Dec 2009, Andres Lagar-Cavilla wrote:
> > Provide a function to clone a context. This is necessary
> > because simply copying the structs will eventually
> > corrup the GC: maxsize is updated in the cloned context
> > but not in the originating one, yet they have the same
> > array of referenced pointers alloc_ptrs.
> > 
> > Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.com>
> > 
> > 
> 
> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
Actually I missed one thing: I think you should move

clone.xsh = xs_daemon_open();

into libxl_clone_context and

xs_daemon_close(clone.xsh);

into libxl_discard_cloned_context.

The rest is fine, thanks for finding and fixing this bug!

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

All right, I cut this patch. There''s a function to clone a context, and
there''s a function to clone a context and open a new xenstore handle. 
And there are the ''discard'' counterparts.
Ack/Nack them and I''ll resubmit the remaining two patches still in the 
pipeline.
Thanks for reviewing everything else
Andres
Stefano Stabellini wrote:
> On Thu, 3 Dec 2009, Stefano Stabellini wrote:
>   
>> On Wed, 2 Dec 2009, Andres Lagar-Cavilla wrote:
>>     
>>> Provide a function to clone a context. This is necessary
>>> because simply copying the structs will eventually
>>> corrup the GC: maxsize is updated in the cloned context
>>> but not in the originating one, yet they have the same
>>> array of referenced pointers alloc_ptrs.
>>>
>>> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.com>
>>>
>>>
>>>       
>> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>
>>     
>
> Actually I missed one thing: I think you should move
>
> clone.xsh = xs_daemon_open();
>
> into libxl_clone_context and
>
> xs_daemon_close(clone.xsh);
>
> into libxl_discard_cloned_context.
>
> The rest is fine, thanks for finding and fixing this bug!
>   


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Thu, 3 Dec 2009, Andres Lagar-Cavilla wrote:
> All right, I cut this patch. There''s a function to clone a
context, and
> there''s a function to clone a context and open a new xenstore
handle.
> And there are the ''discard'' counterparts.
> Ack/Nack them and I''ll resubmit the remaining two patches still in
the
> pipeline.
Good job.

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Wed, Dec 02, 2009 at 10:09:10PM +0000, Andres Lagar-Cavilla
wrote:
> Provide a function to clone a context. This is necessary
> because simply copying the structs will eventually
> corrup the GC: maxsize is updated in the cloned context
> but not in the originating one, yet they have the same
> array of referenced pointers alloc_ptrs.
I think this doesn''t answer the question of why you need this function
at all. I don''t think you should clone the context nor share it.

-- 
Vincent

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Thu, 3 Dec 2009, Vincent Hanquez wrote:
> On Wed, Dec 02, 2009 at 10:09:10PM +0000, Andres Lagar-Cavilla wrote:
> > Provide a function to clone a context. This is necessary
> > because simply copying the structs will eventually
> > corrup the GC: maxsize is updated in the cloned context
> > but not in the originating one, yet they have the same
> > array of referenced pointers alloc_ptrs.
> 
> I think this doesn''t answer the question of why you need this
function
> at all. I don''t think you should clone the context nor share it.
> 
 
At the moment we need to clone the context in few places to open a new
xenstore connection and add temporary watches on xenstore nodes to
synchronously wait for events.

It is a simple solution to avoid conflicts when reading watches and I
think is OK as long as we don''t have too many of these temporary
watches
(we only have two places right now). 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Well. I''m just a humble contributor :)
But if you feel like nacking this patch, it''d be nice if you provided
either a new "GC", or an alternate way to keep using the old
"GC" while
using a separate xenstore handle in functions like libxl_xs_read. Or an
alternate solution to the watch problem.
Thanks,
Andres
> On Wed, Dec 02, 2009 at 10:09:10PM +0000, Andres Lagar-Cavilla wrote:
>> Provide a function to clone a context. This is necessary
>> because simply copying the structs will eventually
>> corrup the GC: maxsize is updated in the cloned context
>> but not in the originating one, yet they have the same
>> array of referenced pointers alloc_ptrs.
>
> I think this doesn''t answer the question of why you need this
function
> at all. I don''t think you should clone the context nor share it.
>
> --
> Vincent
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel