|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH V4 22/24] xl: update domain configuration when we hotplug a device
On Tue, May 06, 2014 at 04:44:56PM +0100, Ian Campbell wrote:
> On Thu, 2014-05-01 at 13:58 +0100, Wei Liu wrote:
> > A convenient macro is written to accomplish following tasks:
>
> "convenience macro" (a convenient macro isn't wrong, it's just not what
> we usually say).
>
Ack.
> >
> > 1. load domain configuration
> > 2. allocate a new device structure NEW
> > 3. copy the parsed device strcture PARSED to NEW
>
> typo strcture.
>
Thanks.
> > 4. call libxl_device_TYPE_add(PARSED)
> > 5. pull from PARSED any fields that might be touched by libxl to NEW
> > 6. store domain configuration
> >
> > Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> > ---
> > tools/libxl/xl_cmdimpl.c | 85
> > +++++++++++++++++++++++++++++++++++++---------
> > 1 file changed, 69 insertions(+), 16 deletions(-)
> >
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index 83f058e..91172c5 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -381,6 +381,36 @@ static void *xrealloc(void *ptr, size_t sz) {
> > &(array)[array_extend_old_count]; \
> > })
> >
> > +#define ARRAY_EXTEND_INIT_NODEVID(array,count,initfn) \
> > + ({ \
> > + typeof((count)) array_extend_old_count = (count); \
> > + (count)++; \
> > + (array) = xrealloc((array), sizeof(*array) * (count)); \
> > + (initfn)(&(array)[array_extend_old_count]); \
> > + &(array)[array_extend_old_count]; \
> > + })
> > +
> > +/* Add a device and update the stored configuration */
> > +#define ADD_DEVICE(devtype,domid,dev,d_config,ptr,allocate,maybe_fixup) \
> > + do { \
> > + libxl_domain_config_init((d_config)); \
> > + load_domain_config((domid), (d_config)); \
> > + \
> > + allocate; \
> > + \
> > + libxl_device_ ## devtype ## _copy(ctx,(ptr),&(dev)); \
> > + \
> > + if (libxl_device_ ## devtype ## _add(ctx,(domid),&(dev), 0)) { \
> > + fprintf(stderr, "libxl_device_%s_add failed.\n", #devtype); \
> > + exit(1); \
> > + } \
> > + \
> > + maybe_fixup; \
> > + \
> > + store_domain_config((domid), (d_config)); \
> > + libxl_domain_config_dispose((d_config)); \
> > + } while (0)
> > +
> > #define LOG(_f, _a...) dolog(__FILE__, __LINE__, __func__, _f "\n", ##_a)
> >
> > static void dolog(const char *file, int line, const char *func, char *fmt,
> > ...)
> > @@ -3047,7 +3077,8 @@ int main_pcidetach(int argc, char **argv)
> > }
> > static void pciattach(uint32_t domid, const char *bdf, const char *vs)
> > {
> > - libxl_device_pci pcidev;
> > + libxl_domain_config d_config;
>
> Is d_config needed outside the scope of the helper macros? If not then
> perhaps push down?
>
The allocation and fixup part in the macro need to access fields in
d_config. It can be pushed down to the macro, but it's less obvious when
you access d_config in allocation and fixup code. I chose to be explicit
about this.
> > + libxl_device_pci pcidev, *ppcidev;
>
> Likewise ppcidev here, perhaps meaning you would need to pass the type
> rather then an instance to the variable.
>
> > XLU_Config *config;
> >
> > libxl_device_pci_init(&pcidev);
> > @@ -3059,7 +3090,12 @@ static void pciattach(uint32_t domid, const char
> > *bdf, const char *vs)
> > fprintf(stderr, "pci-attach: malformed BDF specification
> > \"%s\"\n", bdf);
> > exit(2);
> > }
> > - libxl_device_pci_add(ctx, domid, &pcidev, 0);
> > +
> > + ADD_DEVICE(pci, domid, pcidev, &d_config, ppcidev, {
> > + ppcidev = ARRAY_EXTEND_INIT_NODEVID(d_config.pcidevs,
> > + d_config.num_pcidevs,
> > + libxl_device_pci_init);
>
> Do you really need to do a deep copy of pcidev into the new array slot?
> If you just memcpy the thing over and then do not dispose the original
> then the copy will simply take over those allocations, which is fine I
> think.
>
The copy is done so that we know what user provides at the beginning,
i.e. this is the template that we need to keep. After the structure is
handed back by libxl the content might be altered already, say, all
"default" values replaced by libxl with some values that it deems
sensible. Then fixup will run to copy those bits we care about back to
the template.
> > + }, {});
> >
> > libxl_device_pci_dispose(&pcidev);
> > xlu_cfg_destroy(config);
> > @@ -5959,7 +5995,8 @@ int main_networkattach(int argc, char **argv)
> > {
> > uint32_t domid;
> > int opt;
> > - libxl_device_nic nic;
> > + libxl_domain_config d_config;
> > + libxl_device_nic nic, *pnic;
> > XLU_Config *config = 0;
> > char *endptr, *oparg;
> > const char *tok;
> > @@ -6040,10 +6077,16 @@ int main_networkattach(int argc, char **argv)
> > return 0;
> > }
> >
> > - if (libxl_device_nic_add(ctx, domid, &nic, 0)) {
> > - fprintf(stderr, "libxl_device_nic_add failed.\n");
> > - return 1;
> > - }
> > + /* libxl allocates devid and may generate mac */
> > + ADD_DEVICE(nic, domid, nic, &d_config, pnic, {
> > + pnic = ARRAY_EXTEND_INIT(d_config.nics,
> > + d_config.num_nics,
> > + libxl_device_nic_init);
> > + }, {
> > + pnic->devid = nic.devid;
> > + libxl_mac_copy(ctx, &pnic->mac, &nic.mac);
>
> Doesn't the copy handle this already? If not then why not?
>
This is the fixup. If user doesn't supply uuid, libxl generates one for
this device and we really want to save it in our state. If user supplies
before hand, it just copies the original value back.
The fixup is device dependent. You may find nic has different fixup and
disk has no fixup at all.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |