Re: [libvirt] [PATCH 1/2] libxl: support syntax <interface type="hostdev">

>>> On 5/10/2014 at 06:18 AM, in message <536D541D.5040107(a)suse.com&gt;, Jim Fehlig >>> <jfehlig(a)suse.com&gt; wrote: > Chunyan Liu wrote: > >> Signed-off-by: Chunyan Liu <cyliu(a)suse.com&gt; >> >> > > A while back when testing Chunyan's "common hostdev library" series, I > mentioned that <interface type='hostdev'> was not working with the libxl > driver. Chunyan later privately sent a "v1" of this patch for testing > in my setup. In addition to testing, I provided some private comments. > I see those have been incorporated in this patch and functionally it > looks good, but I do have one additional question about the commit > message... > > >> --- >> src/libxl/libxl_conf.c | 16 +++++++++++----- >> 1 file changed, 11 insertions(+), 5 deletions(-) >> >> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c >> index 298c8a1..b7fed7f 100644 >> --- a/src/libxl/libxl_conf.c >> +++ b/src/libxl/libxl_conf.c >> @@ -921,25 +921,31 @@ static int >> libxlMakeNicList(virDomainDefPtr def, libxl_domain_config *d_config) >> { >> virDomainNetDefPtr *l_nics = def->nets; >> - int nnics = def->nnets; >> + size_t nnics = def->nnets; >> libxl_device_nic *x_nics; >> - size_t i; >> + size_t i, nvnics = 0; >> >> if (VIR_ALLOC_N(x_nics, nnics) < 0) >> return -1; >> >> for (i = 0; i < nnics; i++) { >> - if (libxlMakeNic(def, l_nics[i], &x_nics[i])) >> + if (l_nics[i]->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) >> + continue; >> >> > > After looking at this again, it seems we are really *fixing* <interface > type='hostdev'>. The driver already supports creating the hostdev > device, but without this patch a libxl_device_nic is created too. Is > that a fair statement? If so, the commit message should be changed to > reflect this. Thanks! > A NET_TYPE_HOSTDEV device is really a hostdev device, the driver will create a hostdev device for it, so no need to create a libxl_device_nic again. Before this patch, it tried to call libxlMakeNic to create a libxl_device_nic for it but failed since NET_TYPE_HOSTDEV is not supported there.

I'll add this to commit message. Is that OK?