libxl_device handling for nic and vtmp

libxl_device handling for nic and vtmp

[Olaf Hering]

What is the point of libxl__update_config_nic and
libxl__update_config_vtmp?

In libxl__device_type_add (called from DEFINE_DEVICE_ADD) the input
type is copied with libxl_device_type_copy to type_saved, which is a
1:1 copy. If needed, a new devid is assigned to the input. Later the
copy is updated with one of the two helper functions mentioned above.
But the helpers do not only update devid, also mac or uuid.

To me it looks like the double assignment can be removed. The new
pvusb code does not do it this way, it makes a copy of the fully
initialized type.

Perhaps the two helpers are useful in the context of domcreate_complete,
I have not reviewed that part of the code.


Olaf

Re: libxl_device handling for nic and vtmp

[Wei Liu]

On Thu, Feb 18, 2016 at 04:13:21PM +0100, Olaf Hering wrote:
> What is the point of libxl__update_config_nic and
> libxl__update_config_vtmp?
> 
> In libxl__device_type_add (called from DEFINE_DEVICE_ADD) the input
> type is copied with libxl_device_type_copy to type_saved, which is a
> 1:1 copy. If needed, a new devid is assigned to the input. Later the
> copy is updated with one of the two helper functions mentioned above.
> But the helpers do not only update devid, also mac or uuid.
> 
> To me it looks like the double assignment can be removed. The new
> pvusb code does not do it this way, it makes a copy of the fully
> initialized type.
> 
> Perhaps the two helpers are useful in the context of domcreate_complete,
> I have not reviewed that part of the code.
> 

Because in the process of domain building some configurations are
autogenerated and you want to preserve them.

For example, user might not have specified mac address so the library
generates one for (s)he. You don't want mac address to regenerate after
save / restore or migration.  But you don't want to preserve all
autogenerated state, so you use the original copy as template and fill
it up as you see fit.

BTW, I look at my inbox far more often than I look at xen-devel so if
you CC me (relevant maintainers in general) in relevant emails in the
future they are less likely to fall through the crack.

Wei.

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

Re: libxl_device handling for nic and vtmp

[Olaf Hering]

On Thu, Feb 18, Wei Liu wrote:

> For example, user might not have specified mac address so the library
> generates one for (s)he. You don't want mac address to regenerate after
> save / restore or migration.  But you don't want to preserve all
> autogenerated state, so you use the original copy as template and fill
> it up as you see fit.

How does that fit into DEFINE_DEVICE_ADD? The functions do 1:1 copies,
calling libxl__update_config_* looks unnecessary.

Olaf

Re: libxl_device handling for nic and vtmp

[Wei Liu]

On Thu, Feb 18, 2016 at 05:54:38PM +0100, Olaf Hering wrote:
> On Thu, Feb 18, Wei Liu wrote:
> 
> > For example, user might not have specified mac address so the library
> > generates one for (s)he. You don't want mac address to regenerate after
> > save / restore or migration.  But you don't want to preserve all
> > autogenerated state, so you use the original copy as template and fill
> > it up as you see fit.
> 
> How does that fit into DEFINE_DEVICE_ADD? The functions do 1:1 copies,
> calling libxl__update_config_* looks unnecessary.
> 

Sorry I don't follow. What do you mean by 1:1 copy? Why does it make the
update unnecessary?

As said, we want some of the states but not all.

Wei.

> Olaf

Re: libxl_device handling for nic and vtmp

[Olaf Hering]

On Thu, Feb 18, Wei Liu wrote:

> Sorry I don't follow. What do you mean by 1:1 copy? Why does it make the
> update unnecessary?

The current code does:

  libxl_device_nic_init(&nic_saved);
  libxl_device_nic_copy(CTX, &nic_saved, nic);
  nic->devid = libxl__device_nextid(gc, domid, "vif");
  libxl__update_config_nic(gc, &nic_saved, nic);

Which is equivalent to the more obvious version:

  libxl_device_nic_init(&nic_saved);
  nic->devid = libxl__device_nextid(gc, domid, "vif");
  libxl_device_nic_copy(CTX, &nic_saved, nic);

The input gets modified either way. Is it expected that future versions
of libxl_device_nic/vtmp_copy will not duplicate the mac/uuid parts?


Olaf

Re: libxl_device handling for nic and vtmp

[Wei Liu]

On Thu, Feb 18, 2016 at 06:19:07PM +0100, Olaf Hering wrote:
> On Thu, Feb 18, Wei Liu wrote:
> 
> > Sorry I don't follow. What do you mean by 1:1 copy? Why does it make the
> > update unnecessary?
> 
> The current code does:
> 
>   libxl_device_nic_init(&nic_saved);
>   libxl_device_nic_copy(CTX, &nic_saved, nic);
>   nic->devid = libxl__device_nextid(gc, domid, "vif");
>   libxl__update_config_nic(gc, &nic_saved, nic);
> 
> Which is equivalent to the more obvious version:
> 
>   libxl_device_nic_init(&nic_saved);
>   nic->devid = libxl__device_nextid(gc, domid, "vif");
>   libxl_device_nic_copy(CTX, &nic_saved, nic);
> 
> The input gets modified either way. Is it expected that future versions
> of libxl_device_nic/vtmp_copy will not duplicate the mac/uuid parts?
> 

You missed libxl__device_nic_setdefault -- it changes fields in the
structure.

Wei.

> 
> Olaf