Re: [PATCH v6 06/13] libxl: convert libxl_device_disk_add to an async op

[Roger Pau Monne <roger.pau@citrix.com>]

Ian Jackson wrote:
>>>> +void libxl__initiate_device_connection(libxl__egc *egc, libxl__ao_device *aodev)
>>>> +{
>>>> +    STATE_AO_GC(aodev->ao);
>>>> +    char *be_path = libxl__device_backend_path(gc, aodev->dev);
>>>> +    char *state_path = libxl__sprintf(gc, "%s/state", be_path);
>>>> +    int rc = 0;
>>> ...
>>>> +out_fail:
>>>> +    assert(rc);
>>>> +    aodev->rc = rc;
>>>> +    device_xsentries_remove(egc, aodev);
>>>> +    return;
>>>> +}
>>> Firstly, I'm not convinced it's really proper for
>>> libxl__initiate_device_connection to call device_xsentries_remove.
>>> After all device_xsentries_remove is part of the implementation of
>>> libxl__initiate_device_remove.
>> This is part of both flows, both device connection and disconnection.
>
> Well then it should have a proper description and a better name,
> perhaps ?

This functions is latter renamed to "device_hotplug_done", which is the 
point where the callback is done. Since this name is only going to be 
there for like two commits, can we leave it like that?

> As it is it looks like you're doing what amounts to "goto"
> from one "function" to another - only we don't notice it like that
> because it's split into multiple ao callbacks.

Yes, that's mainly one of the consequences of having the same exit point 
for all the possible scenarios. Almost every iteration of this series 
has been adding more of this stuff.

>>> And, secondly, does device_xsentries_remove really do what you want ?
>>> It has a test in it which makes it only do its work if action is
>>> DISCONNECT.
>> Yes, that's because it's called by both flows.
>
> If it is called in the connect flow, won't it be a no-op then ?
> Perhaps I'm being excessively dense here.

It is a no-op here, but in latter patches it becomes the exit point for 
the device addition/removal flow, once it is renamed to device_hotplug_done.

>
>>> Isn't the effect of this that if the xs transaction gets a conflict,
>>> we'll rerun the hotplug scripts, etc. ?  I think I may be confused
>>> here, but I don't understand how this transaction loop is supposed to
>>> work and how it is supposed to relate to the interaction with other
>>> domains, scripts, etc.
>> Yes I see your point. We should disconnect the device (execute hotplug
>> scripts) but since the xenstore entries are already gone (because the
>> transaction is not committed successfully) I don't see anyway to do it,
>> we cannot execute those scripts if the backend entries have been lost.
>>
>>> Indeed it seems to me like your arrangements in libxl__device_disk_add
>>> couldn't work if a non-null t was supplied.  libxl__device_disk_add
>>> would do all the writes in the transaction, but not commit it, so that
>>> none of them are visible to anyone, and then wait for the deivde state
>>> to change, which will never happen.
>> I'm not so sure, is it possible to add a watch to a xenstore path that
>> is inside of a not yet committed transaction? If so this will work fine,
>> the transaction will be finished correctly, and the path will be updated
>> to the correct state (2).
>
> AIUI you can add a watch for any path that you like - the path you ask
> to watch doesn't have to exist.  But you won't (necessarily) see
> events for uncommitted transactions.

Yes, but then we will hit a timeout if the transaction is not actually 
committed.

>> If the transaction is not committed successfully, we will get a timeout
>> from the devstate event and exit.
>
> I think that the only way all this could work is if you firstly, in
> one transaction, create enough of the backend for the hotplug scripts
> to run, and then in a second transaction create the rest of the
> backend plus the frontend.

The hotplug scripts require the backend to be in state 2 (it requires 
that the kernel has initialized the device) so it needs to have a full 
backend set up.

>>> I think this code is a symptom of a problem elsewhere.  When adding a
>>> disk to a domain, it should not be necessary to explicitly ask to add
>>> it to the stubdom too.  But this is not your fault, or your problem
>>> right now.
>> So I assume we can leave this for later.
>
> Yes.
>
> Thanks,
> Ian.