[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v6 06/13] libxl: convert libxl_device_disk_add to an async op
Ian Jackson wrote:
+void libxl__initiate_device_connection(libxl__egc *egc, libxl__ao_device
+ char *be_path = libxl__device_backend_path(gc, aodev->dev);
+ char *state_path = libxl__sprintf(gc, "%s/state", be_path);
+ int rc = 0;
+ aodev->rc = rc;
+ device_xsentries_remove(egc, aodev);
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
This is part of both flows, both device connection and disconnection.
Well then it should have a proper description and a better name,
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
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
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
So I assume we can leave this for later.
Xen-devel mailing list