[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH,RFC]: Introduce libxl_domain_create()
On Tue, 2010-12-21 at 13:28 +0000, Ian Campbell wrote: > On Tue, 2010-12-21 at 13:01 +0000, Gianni Tedesco wrote: > > On Tue, 2010-12-21 at 11:02 +0000, Ian Campbell wrote: > > > On Tue, 2010-12-21 at 08:44 +0000, Gianni Tedesco wrote: > > > > On Fri, 2010-12-17 at 09:06 +0000, Ian Campbell wrote: > > > > > On Tue, 2010-12-14 at 19:00 +0000, Gianni Tedesco wrote: > > > > > We probably need IDL support for enumerations and other constants. > > > > > > > > Might be a good idea. We also rely on a few xc constants. In the case of > > > > the python binding I had been adding them manually. If we did this via > > > > IDL it'd be an idea to generate type-safety macros for that stuff too. > > > > > > What sort of macros? > > > > Well just to have them as typed structs and macros to get/set the > > integer values. > > Seem like overkill to me. > > > Beyond that I don't see much point in putting them in > > the IDL at all. > > So that language bindings can make a variable/constant with the > appropriate name and value available to the user, without needing to > manually code a list in each set of bindings. Fair enough > > > > > > > > +typedef struct { > > > > [...] > > > > > > +} libxl_domain_config; > > > > > > > > > > Should be in IDL so it gets a destructor? Could require adding an > > > > > Array > > > > > construct to handle the foo + num_foo style stuff. > > > > > > > > I've thought about that and rejected it because C arrays don't map to > > > > anything useful in language bindings. It makes sense to me to keep this > > > > as a builtin and use functions to fill these domain creation related > > > > structures in for us. > > > > > > OK > > > > > > > But then what you get is like two versions of: > > > > - libxl_device_add_(nic|block|etc) > > > > One for a live domain and one for domain creation. > > > > > > > > I have been toying with the idea of using polymorphism (is that what > > > > it's called?) So that such a function would multiplex to different > > > > implementations depending on whether this is a live domain or a > > > > description of a domain for creation. It might need a bit of thinking > > > > through as how it would be used. > > > > > > > > Not sure what the others think of that? > > > > > > Proper polymorphism is a bit tricky in C, since you can't have multiple > > > variations of the same function with different parameters and you simply > > > end up with multiple different functions -- i.e. back where you started. > > > > > > The need for a version of libxl_device_add_FOO for the create case is > > > simply to support automatically extending the array while filing in the > > > structure etc? I don't see a useful way to have a function which works > > > like this for both live and in-creation domains. > > > > Maybe I'm using the wrong OOP term? But you would have a libxl_domain > > struct and operations on it would be redirected via virtual methods. > > Either to update a struct or a live domain. But the input parameters > > would always be the same. > > Hmm, so in the live domain case the struct would be mostly empty apart > from a domid and in the domain build case it would contains all the > various info structures? Exactly yes. > Polymorphic would mean you had libxl_add_nic(int domid, ...) and > libxl_add_nic(struct libxl_domain, ...) and the compiler would pick the > right one from the arguments you give. > > I think what you describe is more like inheritance or something. > > > > > > > +static pid_t autoconnect_console(libxl_ctx *ctx, int domid) > > > > > > +{ > > > > > [...] > > > > > > +} > > > > > > > > > > I think console connect should be under toolstack control (i.e. stay > > > > > in > > > > > xl). exec'ing the xenconsole client is only one way of connecting the > > > > > console, e.g. xapi might want to launch vncterm instead. > > > > > > > > > > I think libxl_domain_create should take a callback function pointer to > > > > > connect the console. It's possible that libxl might also provide a > > > > > convenience function which launches xenconsole which is suitable for > > > > > use > > > > > as this callback but ultimately it should be the toolstack's decision > > > > > what to do here. > > > > > > > > I think you're right. I had just been trying to avoid having a mega > > > > function with 12 arguments, 6 of them callbacks. > > > > > > A structure containing the callback functions is the answer there. > > > > Hmm, my displeasure is not so much how to pass the arguments but that > > API's based around a lot of callbacks tend to be difficult to understand > > and hard to maintain. They pretend to make it such that you don't need > > to know what order things are done in but then you end up relying on the > > order things are done in... > > > > I have already factored out the other nastiness of the API I proposed > > (the flags) but autoconnect seems to be the final sticking point. > > > > > > I had the idea that the original domain_create() was very fragile and I > > > > didn't want to move things around. But on the other hand it seems to me > > > > that there's no reason to start the console at two different points > > > > depending on pv or hvm and perhaps I should just try to move the code > > > > around. > > > > > > I'm pretty certain Stefano did this deliberately when he introduced > > > console support for HVM, I don't remember the specific constraint he > > > found on HVM though. It seems to arise from 22100:fde833c66948 but the > > > commit message doesn't say why just "it needs to be this way". > > > > > > > Domains start paused anyway so why can't we just: > > > > > > > > libxl_domain_create(); > > > > do_console_stuff(); > > > > libxl_domain_unpause(); > > > > > > Not quite because for a PV domain we need to do the console before the > > > bootloader runs (so the user can interact with pygrub) and the > > > bootloader provides the input to libxl_domain_create(). > > > > > > So for PV it ends up as > > > libxl_domain_make() // returns a domid > > > do_console_stuff() // launches console client > > > libxl_run_bootloader() // potentially interact with console, > > > return kernel etc > > > libxl_domain_create() // build domain using kernel > > > libxl_domain_unpause() // go go go > > > > > > My guess is that there is some reason you can't create the console for > > > an HVM guest before libxl_domain_create but I don't specifically know > > > why, perhaps qemu needs to be running? > > > > > > In theory at least the do_console_stuff should cause a client to start > > > and wait for the server end to appear, rather than insist on connecting > > > immediately and it already behaves that way for PV guests, I don't see > > > any fundamental reason HVM couldn't be the same. > > > > Yes, the points above make sense. I think I would rather go down the > > road of libxl callers setting up their console stuff before creating the > > domain, providing a wait_for_console_stuff() API. Or at worst, give them > > the control back over bootloader and do a 2/3-phase domain creation API. > > That's pretty close to what we have today, isn't it? > > We have (roughly): > libxl_domain_make() > do_the_pv_console() > libxl_run_bootloader() > libxl_domain_create() > for each device: libxl_device_blah_add > do_the_hvm_console() > libxl_domain_unpause() > > Your initial patch moved all of that into libxl_domain_create but now > you are suggesting that only the "for each device" and/or > libxl_run_bootloader bits gets pushed down? e.g. the caller does > libxl_domain_make() > do_the_pv_console() > libxl_domain_create() > libxl_run_bootloader() > libxl_domain_actually_create() > for each device: libxl_device_blah_add > do_the_hvm_console() > libxl_domain_unpause() > > seems semi plausible, although how to do the do_the_pv_console vs hvm > console in a clean way. Yeah I don't like it because I already think xl is doing a helluva lot of stuff in its create_domain() function. If we can have pv and hvm cases unified so that both consoles can be done a lot earlier (ie. before libxl_domain_create()) then that would let us keep the bulk of the work in libxl and we can zap: - libxl_domain(make|create|restore|run_bootloader) Then again, it may complicate the callers because then they have to do something if domain_create() fails to clean that up. So, domain creation really is hard. Let's go shopping :S Gianni _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxx http://lists.xensource.com/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |