[libvirt] [PATCH 1/2] libxl: support syntax <interface type="hostdev">
Signed-off-by: Chunyan Liu <cyliu@suse.com> --- 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; + + if (libxlMakeNic(def, l_nics[i], &x_nics[nvnics])) goto error; /* * The devid (at least right now) will not get initialized by * libxl in the setup case but is required for starting the * device-model. */ - if (x_nics[i].devid < 0) - x_nics[i].devid = i; + if (x_nics[nvnics].devid < 0) + x_nics[nvnics].devid = nvnics; + + nvnics++; } + VIR_SHRINK_N(x_nics, nnics, nnics - nvnics); d_config->nics = x_nics; d_config->num_nics = nnics; -- 1.8.4.5
<interface type='hostdev' managed='yes'> is supported, but nowhere mentions 'managed' in <interface type='hostdev'> syntax. Update documentation to cover it. Signed-off-by: Chunyan Liu <cyliu@suse.com> --- docs/formatdomain.html.in | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 4f19473..c3b9f5b 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3510,7 +3510,7 @@ <pre> ... <devices> - <interface type='hostdev'> + <interface type='hostdev' managed='yes'> <driver name='vfio'/> <source> <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> @@ -3523,6 +3523,18 @@ </devices> ...</pre> + <p> + Similar to the functionality of a standard <hostdev> device, + when <code>managed</code> is "yes", it is detached from the host + before being passed on to the guest, and reattached to the host + after the guest exits. If <code>managed</code> is omitted or "no", + the user is responsible to call <code>virNodeDeviceDettach</code> + (or <code>virsh nodedev-dettach</code>) before starting the guest + or hot-plugging the device, and <code>virNodeDeviceReAttach</code> + (or <code>virsh nodedev-reattach</code>) after hot-unplug or + stopping the guest. + </p> + <h5><a name="elementsNICSMulticast">Multicast tunnel</a></h5> -- 1.8.4.5
Chunyan Liu wrote:
<interface type='hostdev' managed='yes'> is supported, but nowhere mentions 'managed' in <interface type='hostdev'> syntax. Update documentation to cover it.
Yep, I see the managed attribute is in the schema.
Signed-off-by: Chunyan Liu <cyliu@suse.com> --- docs/formatdomain.html.in | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 4f19473..c3b9f5b 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3510,7 +3510,7 @@ <pre> ... <devices> - <interface type='hostdev'> + <interface type='hostdev' managed='yes'> <driver name='vfio'/> <source> <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> @@ -3523,6 +3523,18 @@ </devices> ...</pre>
+ <p> + Similar to the functionality of a standard <hostdev> device, + when <code>managed</code> is "yes", it is detached from the host + before being passed on to the guest, and reattached to the host + after the guest exits. If <code>managed</code> is omitted or "no", + the user is responsible to call <code>virNodeDeviceDettach</code> + (or <code>virsh nodedev-dettach</code>) before starting the guest + or hot-plugging the device, and <code>virNodeDeviceReAttach</code> + (or <code>virsh nodedev-reattach</code>) after hot-unplug or + stopping the guest. + </p> +
I suppose the only question I have is whether this paragraph should be before the example XML snippet or after as you have placed it? Anyone else have a comment about that? Looks good otherwise. Regards, Jim
<h5><a name="elementsNICSMulticast">Multicast tunnel</a></h5>
On 05/09/2014 05:09 PM, Jim Fehlig wrote:
Chunyan Liu wrote:
<interface type='hostdev' managed='yes'> is supported, but nowhere mentions 'managed' in <interface type='hostdev'> syntax. Update documentation to cover it.
Yep, I see the managed attribute is in the schema.
Signed-off-by: Chunyan Liu <cyliu@suse.com> --- docs/formatdomain.html.in | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 4f19473..c3b9f5b 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3510,7 +3510,7 @@ <pre> ... <devices> - <interface type='hostdev'> + <interface type='hostdev' managed='yes'> <driver name='vfio'/> <source> <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> @@ -3523,6 +3523,18 @@ </devices> ...</pre>
+ <p> + Similar to the functionality of a standard <hostdev> device, + when <code>managed</code> is "yes", it is detached from the host + before being passed on to the guest, and reattached to the host + after the guest exits. If <code>managed</code> is omitted or "no", + the user is responsible to call <code>virNodeDeviceDettach</code> + (or <code>virsh nodedev-dettach</code>) before starting the guest + or hot-plugging the device, and <code>virNodeDeviceReAttach</code> + (or <code>virsh nodedev-reattach</code>) after hot-unplug or + stopping the guest. + </p> +
I suppose the only question I have is whether this paragraph should be before the example XML snippet or after as you have placed it? Anyone else have a comment about that? Looks good otherwise.
The most important thing of course is just having the information there at all :-). I think it is at least more common to have the the descriptions before examples using them rather than after, though, so I agree with you that the paragraph would be better before the example. BTW, it's really cool that <interface type='hostdev'> essentially "just works" (with the one small bug fix) after turning the hostdev support functions into a hypervisor-agnostic library :-)
Laine Stump wrote:
On 05/09/2014 05:09 PM, Jim Fehlig wrote:
Chunyan Liu wrote:
<interface type='hostdev' managed='yes'> is supported, but nowhere mentions 'managed' in <interface type='hostdev'> syntax. Update documentation to cover it.
Yep, I see the managed attribute is in the schema.
Signed-off-by: Chunyan Liu <cyliu@suse.com> --- docs/formatdomain.html.in | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 4f19473..c3b9f5b 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3510,7 +3510,7 @@ <pre> ... <devices> - <interface type='hostdev'> + <interface type='hostdev' managed='yes'> <driver name='vfio'/> <source> <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> @@ -3523,6 +3523,18 @@ </devices> ...</pre>
+ <p> + Similar to the functionality of a standard <hostdev> device, + when <code>managed</code> is "yes", it is detached from the host + before being passed on to the guest, and reattached to the host + after the guest exits. If <code>managed</code> is omitted or "no", + the user is responsible to call <code>virNodeDeviceDettach</code> + (or <code>virsh nodedev-dettach</code>) before starting the guest + or hot-plugging the device, and <code>virNodeDeviceReAttach</code> + (or <code>virsh nodedev-reattach</code>) after hot-unplug or + stopping the guest. + </p> +
I suppose the only question I have is whether this paragraph should be before the example XML snippet or after as you have placed it? Anyone else have a comment about that? Looks good otherwise.
The most important thing of course is just having the information there at all :-). I think it is at least more common to have the the descriptions before examples using them rather than after, though, so I agree with you that the paragraph would be better before the example.
Thanks Laine. I moved the text before the example and pushed the patch.
BTW, it's really cool that <interface type='hostdev'> essentially "just works" (with the one small bug fix) after turning the hostdev support functions into a hypervisor-agnostic library :-)
Agreed. Took a while to get that work done, but well worth it in the long run :-). Regards, Jim
Chunyan Liu wrote:
Signed-off-by: Chunyan Liu <cyliu@suse.com>
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! Regards, Jim
+ + if (libxlMakeNic(def, l_nics[i], &x_nics[nvnics])) goto error; /* * The devid (at least right now) will not get initialized by * libxl in the setup case but is required for starting the * device-model. */ - if (x_nics[i].devid < 0) - x_nics[i].devid = i; + if (x_nics[nvnics].devid < 0) + x_nics[nvnics].devid = nvnics; + + nvnics++; }
+ VIR_SHRINK_N(x_nics, nnics, nnics - nvnics); d_config->nics = x_nics; d_config->num_nics = nnics;
On 5/10/2014 at 06:18 AM, in message <536D541D.5040107@suse.com>, Jim Fehlig <jfehlig@suse.com> wrote: Chunyan Liu wrote: Signed-off-by: Chunyan Liu <cyliu@suse.com>
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? - Chunyan
Regards, Jim
+ + if (libxlMakeNic(def, l_nics[i], &x_nics[nvnics])) goto error; /* * The devid (at least right now) will not get initialized by * libxl in the setup case but is required for starting the * device-model. */ - if (x_nics[i].devid < 0) - x_nics[i].devid = i; + if (x_nics[nvnics].devid < 0) + x_nics[nvnics].devid = nvnics; + + nvnics++; }
+ VIR_SHRINK_N(x_nics, nnics, nnics - nvnics); d_config->nics = x_nics; d_config->num_nics = nnics;
participants (4)
-
Chun Yan Liu -
Chunyan Liu
-
Jim Fehlig -
Laine Stump