Re: [Xen-devel] [PATCH V4 22/24] xl: update domain configuration when we hotplug a device

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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).

> 
> 1. load domain configuration
> 2. allocate a new device structure NEW
> 3. copy the parsed device strcture PARSED to NEW

typo strcture.

> 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?

> +    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.

> +        }, {});
>  
>      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?



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel