7:22 a.m.
Pavel Hrdina (4):
virsh-domain: use correct base for virStrToLong_ui
virsh-nodedev: makes struct and functions for NodeDevice list
available
virsh-domain: update attach-interface to support type=hostdev
virsh.pod: update and improve a attach-interface section
tools/virsh-domain.c | 120 ++++++++++++++++++++++++++++++++++++++++++--------
tools/virsh-nodedev.c | 16 +++----
tools/virsh-nodedev.h | 11 +++++
tools/virsh.pod | 85 +++++++++++++++++++++--------------
4 files changed, 170 insertions(+), 62 deletions(-)
--
2.6.2
7:22 a.m.
New subject: [libvirt] [PATCH 1/4] virsh-domain: use correct base for virStrToLong_ui
While parsing device addresses we should use correct base and don't
count on auto-detect. For example, PCI address uses hex numbers, but
each number starting with 0 will be auto-detected as octal number and
that's wrong. Another wrong use-case is for PCI address if for example
bus is 10, than it's incorrectly parsed as decimal number.
PCI and CCW addresses have all values as hex numbers, IDE and SCSI
addresses are in decimal numbers.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
tools/virsh-domain.c | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 4191548..e8503ec 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -455,19 +455,19 @@ static int str2PCIAddress(const char *str, struct PCIAddress
*pciAddr)
domain = (char *)str;
- if (virStrToLong_ui(domain, &bus, 0, &pciAddr->domain) != 0)
+ if (virStrToLong_ui(domain, &bus, 16, &pciAddr->domain) != 0)
return -1;
bus++;
- if (virStrToLong_ui(bus, &slot, 0, &pciAddr->bus) != 0)
+ if (virStrToLong_ui(bus, &slot, 16, &pciAddr->bus) != 0)
return -1;
slot++;
- if (virStrToLong_ui(slot, &function, 0, &pciAddr->slot) != 0)
+ if (virStrToLong_ui(slot, &function, 16, &pciAddr->slot) != 0)
return -1;
function++;
- if (virStrToLong_ui(function, NULL, 0, &pciAddr->function) != 0)
+ if (virStrToLong_ui(function, NULL, 16, &pciAddr->function) != 0)
return -1;
return 0;
@@ -484,15 +484,15 @@ static int str2SCSIAddress(const char *str, struct SCSIAddress
*scsiAddr)
controller = (char *)str;
- if (virStrToLong_uip(controller, &bus, 0, &scsiAddr->controller) != 0)
+ if (virStrToLong_uip(controller, &bus, 10, &scsiAddr->controller) != 0)
return -1;
bus++;
- if (virStrToLong_uip(bus, &unit, 0, &scsiAddr->bus) != 0)
+ if (virStrToLong_uip(bus, &unit, 10, &scsiAddr->bus) != 0)
return -1;
unit++;
- if (virStrToLong_ullp(unit, NULL, 0, &scsiAddr->unit) != 0)
+ if (virStrToLong_ullp(unit, NULL, 10, &scsiAddr->unit) != 0)
return -1;
return 0;
@@ -509,15 +509,15 @@ static int str2IDEAddress(const char *str, struct IDEAddress
*ideAddr)
controller = (char *)str;
- if (virStrToLong_ui(controller, &bus, 0, &ideAddr->controller) != 0)
+ if (virStrToLong_ui(controller, &bus, 10, &ideAddr->controller) != 0)
return -1;
bus++;
- if (virStrToLong_ui(bus, &unit, 0, &ideAddr->bus) != 0)
+ if (virStrToLong_ui(bus, &unit, 10, &ideAddr->bus) != 0)
return -1;
unit++;
- if (virStrToLong_ui(unit, NULL, 0, &ideAddr->unit) != 0)
+ if (virStrToLong_ui(unit, NULL, 10, &ideAddr->unit) != 0)
return -1;
return 0;
@@ -534,15 +534,15 @@ static int str2CCWAddress(const char *str, struct CCWAddress
*ccwAddr)
cssid = (char *)str;
- if (virStrToLong_ui(cssid, &ssid, 0, &ccwAddr->cssid) != 0)
+ if (virStrToLong_ui(cssid, &ssid, 16, &ccwAddr->cssid) != 0)
return -1;
ssid++;
- if (virStrToLong_ui(ssid, &devno, 0, &ccwAddr->ssid) != 0)
+ if (virStrToLong_ui(ssid, &devno, 16, &ccwAddr->ssid) != 0)
return -1;
devno++;
- if (virStrToLong_ui(devno, NULL, 0, &ccwAddr->devno) != 0)
+ if (virStrToLong_ui(devno, NULL, 16, &ccwAddr->devno) != 0)
return -1;
return 0;
@@ -739,8 +739,8 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
} else if (STRPREFIX((const char *)target, "hd")) {
if (diskAddr.type == DISK_ADDR_TYPE_IDE) {
virBufferAsprintf(&buf,
- "<address type='drive'
controller='%d'"
- " bus='%d' unit='%d'
/>\n",
+ "<address type='drive'
controller='%u'"
+ " bus='%u' unit='%u'
/>\n",
diskAddr.addr.ide.controller, diskAddr.addr.ide.bus,
diskAddr.addr.ide.unit);
} else {
--
2.6.2
10:24 a.m.
New subject: [libvirt] [PATCH 1/4] virsh-domain: use correct base for virStrToLong_ui
On 10/21/2015 08:22 AM, Pavel Hrdina wrote:
While parsing device addresses we should use correct base and
don't
count on auto-detect. For example, PCI address uses hex numbers, but
each number starting with 0 will be auto-detected as octal number and
that's wrong. Another wrong use-case is for PCI address if for example
bus is 10, than it's incorrectly parsed as decimal number.
PCI and CCW addresses have all values as hex numbers, IDE and SCSI
addresses are in decimal numbers.
I've seen examples for PCI that use decimal a number for the slot (which
is the one item that's likely to have a value > 7 unless you have a
system with a whole bunch of PCI controllers)[*], and those that use hex
numbers always prefix the number with "0x". libvirt itself always
prefixes the domain, bus, and slot with 0x, so an auto-detected base
will always get those right.
So I I think the existing code is correct, and don't see an upside to
making this restriction/invisible change in semantics, and it could
potentially lead to incorrect results if someone is thinking that
they're entering decimal numbers.
[*] There was one particular document that even went to the trouble of
explaining how to convert the hex value of slot into a decimal number to
use in the libvirt config!
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
tools/virsh-domain.c | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 4191548..e8503ec 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -455,19 +455,19 @@ static int str2PCIAddress(const char *str, struct PCIAddress
*pciAddr)
domain = (char *)str;
- if (virStrToLong_ui(domain, &bus, 0, &pciAddr->domain) != 0)
+ if (virStrToLong_ui(domain, &bus, 16, &pciAddr->domain) != 0)
return -1;
bus++;
- if (virStrToLong_ui(bus, &slot, 0, &pciAddr->bus) != 0)
+ if (virStrToLong_ui(bus, &slot, 16, &pciAddr->bus) != 0)
return -1;
slot++;
- if (virStrToLong_ui(slot, &function, 0, &pciAddr->slot) != 0)
+ if (virStrToLong_ui(slot, &function, 16, &pciAddr->slot) != 0)
return -1;
function++;
- if (virStrToLong_ui(function, NULL, 0, &pciAddr->function) != 0)
+ if (virStrToLong_ui(function, NULL, 16, &pciAddr->function) != 0)
return -1;
return 0;
@@ -484,15 +484,15 @@ static int str2SCSIAddress(const char *str, struct SCSIAddress
*scsiAddr)
controller = (char *)str;
- if (virStrToLong_uip(controller, &bus, 0, &scsiAddr->controller) != 0)
+ if (virStrToLong_uip(controller, &bus, 10, &scsiAddr->controller) != 0)
return -1;
bus++;
- if (virStrToLong_uip(bus, &unit, 0, &scsiAddr->bus) != 0)
+ if (virStrToLong_uip(bus, &unit, 10, &scsiAddr->bus) != 0)
return -1;
unit++;
- if (virStrToLong_ullp(unit, NULL, 0, &scsiAddr->unit) != 0)
+ if (virStrToLong_ullp(unit, NULL, 10, &scsiAddr->unit) != 0)
return -1;
return 0;
@@ -509,15 +509,15 @@ static int str2IDEAddress(const char *str, struct IDEAddress
*ideAddr)
controller = (char *)str;
- if (virStrToLong_ui(controller, &bus, 0, &ideAddr->controller) != 0)
+ if (virStrToLong_ui(controller, &bus, 10, &ideAddr->controller) != 0)
return -1;
bus++;
- if (virStrToLong_ui(bus, &unit, 0, &ideAddr->bus) != 0)
+ if (virStrToLong_ui(bus, &unit, 10, &ideAddr->bus) != 0)
return -1;
unit++;
- if (virStrToLong_ui(unit, NULL, 0, &ideAddr->unit) != 0)
+ if (virStrToLong_ui(unit, NULL, 10, &ideAddr->unit) != 0)
return -1;
return 0;
@@ -534,15 +534,15 @@ static int str2CCWAddress(const char *str, struct CCWAddress
*ccwAddr)
cssid = (char *)str;
- if (virStrToLong_ui(cssid, &ssid, 0, &ccwAddr->cssid) != 0)
+ if (virStrToLong_ui(cssid, &ssid, 16, &ccwAddr->cssid) != 0)
return -1;
ssid++;
- if (virStrToLong_ui(ssid, &devno, 0, &ccwAddr->ssid) != 0)
+ if (virStrToLong_ui(ssid, &devno, 16, &ccwAddr->ssid) != 0)
return -1;
devno++;
- if (virStrToLong_ui(devno, NULL, 0, &ccwAddr->devno) != 0)
+ if (virStrToLong_ui(devno, NULL, 16, &ccwAddr->devno) != 0)
return -1;
return 0;
@@ -739,8 +739,8 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
} else if (STRPREFIX((const char *)target, "hd")) {
if (diskAddr.type == DISK_ADDR_TYPE_IDE) {
virBufferAsprintf(&buf,
- "<address type='drive'
controller='%d'"
- " bus='%d' unit='%d'
/>\n",
+ "<address type='drive'
controller='%u'"
+ " bus='%u' unit='%u'
/>\n",
diskAddr.addr.ide.controller, diskAddr.addr.ide.bus,
diskAddr.addr.ide.unit);
} else {
2:33 a.m.
New subject: [libvirt] [PATCH 1/4] virsh-domain: use correct base for virStrToLong_ui
On Thu, Oct 22, 2015 at 11:24:02 -0400, Laine Stump wrote:
On 10/21/2015 08:22 AM, Pavel Hrdina wrote:
> While parsing device addresses we should use correct base and don't
> count on auto-detect. For example, PCI address uses hex numbers, but
> each number starting with 0 will be auto-detected as octal number and
> that's wrong. Another wrong use-case is for PCI address if for example
> bus is 10, than it's incorrectly parsed as decimal number.
>
> PCI and CCW addresses have all values as hex numbers, IDE and SCSI
> addresses are in decimal numbers.
I've seen examples for PCI that use decimal a number for the slot (which
is the one item that's likely to have a value > 7 unless you have a
system with a whole bunch of PCI controllers)[*], and those that use hex
numbers always prefix the number with "0x". libvirt itself always a
lspci doesn't really use 0x prefix:
00:16.0 Communication controller: Intel Corporation Wildcat Point-LP MEI Controller #1
(rev 03)
00:19.0 Ethernet controller: Intel Corporation Ethernet Connection (3) I218-LM (rev 03)
00:1b.0 Audio device: Intel Corporation Wildcat Point-LP High Definition Audio Controller
(rev 03)
00:1c.0 PCI bridge: Intel Corporation Wildcat Point-LP PCI Express Root Port #6 (rev e3)
Btw that's a laptop, not a super special system.
prefixes the domain, bus, and slot with 0x, so an auto-detected base
will always get those right.
Well, not entirely:
nodedev identificators use hex, possibly padded with zeroes:
pci_0000_00_03_0
pci_0000_00_14_0
pci_0000_00_16_0
pci_0000_00_19_0
pci_0000_00_1b_0
Which may result also in some octal 'fun' on boxes with 16 buses ;).
nodedev XML uses decimal in the parsed address:
$ virsh nodedev-dumpxml pci_0000_00_19_0
<device>
<name>pci_0000_00_19_0</name>
<path>/sys/devices/pci0000:00/0000:00:19.0</path>
<parent>computer</parent>
<driver>
<name>e1000e</name>
</driver>
<capability type='pci'>
<domain>0</domain>
<bus>0</bus>
<slot>25</slot>
<function>0</function>
<product id='0x15a2'>Ethernet Connection (3) I218-LM</product>
<vendor id='0x8086'>Intel Corporation</vendor>
</capability>
</device>
(Uhhh, so 19 ... or 25? ... or perhaps 31?)
And for hostdevs we are promoting the use of 0x prefixed hex:
<address domain='0x0000' bus='0x06' slot='0x02'
function='0x0'/>
As well as with target addresses. Fortunately.
<sarcasm>Well that's very consistent.</sarcasm>
So I I think the existing code is correct, and don't see an upside to
making this restriction/invisible change in semantics, and it could
potentially lead to incorrect results if someone is thinking that
they're entering decimal numbers.
The existing code is correct only perhaps from a historical point of
view. From a functional it's more than flawed. ...
[*] There was one particular document that even went to the trouble of
explaining how to convert the hex value of slot into a decimal number to
use in the libvirt config!
Well, this hints that we do actually something wrong here. The ambiguity
in parsing of numbers with virStrToLong_ui has already bitten us (I
won't bother looking up the commit though).
The problem is that if you use 0 as a base argument it's not really
clear what you've parsed and that will result in situations like this.
The developer may be lucky in trying numbers that were parsed correctly
misleading him that he used the correct base.
I think that we shouldn't be using 0 as base in ANY place in our code.
As of this particular case: If we format it in base 16, we should parse
it in base 16. Having ambiguity in the parser code will only end up in
problems.
>
> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
> ---
> tools/virsh-domain.c | 30 +++++++++++++++---------------
> 1 file changed, 15 insertions(+), 15 deletions(-)
>
Peter
2:59 a.m.
New subject: [libvirt] [PATCH 1/4] virsh-domain: use correct base for virStrToLong_ui
On Fri, Oct 23, 2015 at 09:33:49AM +0200, Peter Krempa wrote:
On Thu, Oct 22, 2015 at 11:24:02 -0400, Laine Stump wrote:
> On 10/21/2015 08:22 AM, Pavel Hrdina wrote:
> > While parsing device addresses we should use correct base and don't
> > count on auto-detect. For example, PCI address uses hex numbers, but
> > each number starting with 0 will be auto-detected as octal number and
> > that's wrong. Another wrong use-case is for PCI address if for example
> > bus is 10, than it's incorrectly parsed as decimal number.
> >
> > PCI and CCW addresses have all values as hex numbers, IDE and SCSI
> > addresses are in decimal numbers.
>
> I've seen examples for PCI that use decimal a number for the slot (which
> is the one item that's likely to have a value > 7 unless you have a
> system with a whole bunch of PCI controllers)[*], and those that use hex
> numbers always prefix the number with "0x". libvirt itself always a
lspci doesn't really use 0x prefix:
00:16.0 Communication controller: Intel Corporation Wildcat Point-LP MEI Controller #1
(rev 03)
00:19.0 Ethernet controller: Intel Corporation Ethernet Connection (3) I218-LM (rev 03)
00:1b.0 Audio device: Intel Corporation Wildcat Point-LP High Definition Audio Controller
(rev 03)
00:1c.0 PCI bridge: Intel Corporation Wildcat Point-LP PCI Express Root Port #6 (rev e3)
Btw that's a laptop, not a super special system.
> prefixes the domain, bus, and slot with 0x, so an auto-detected base
> will always get those right.
Well, not entirely:
nodedev identificators use hex, possibly padded with zeroes:
pci_0000_00_03_0
pci_0000_00_14_0
pci_0000_00_16_0
pci_0000_00_19_0
pci_0000_00_1b_0
Which may result also in some octal 'fun' on boxes with 16 buses ;).
nodedev XML uses decimal in the parsed address:
$ virsh nodedev-dumpxml pci_0000_00_19_0
<device>
<name>pci_0000_00_19_0</name>
<path>/sys/devices/pci0000:00/0000:00:19.0</path>
<parent>computer</parent>
<driver>
<name>e1000e</name>
</driver>
<capability type='pci'>
<domain>0</domain>
<bus>0</bus>
<slot>25</slot>
<function>0</function>
<product id='0x15a2'>Ethernet Connection (3) I218-LM</product>
<vendor id='0x8086'>Intel Corporation</vendor>
</capability>
</device>
(Uhhh, so 19 ... or 25? ... or perhaps 31?)
And for hostdevs we are promoting the use of 0x prefixed hex:
<address domain='0x0000' bus='0x06' slot='0x02'
function='0x0'/>
As well as with target addresses. Fortunately.
<sarcasm>Well that's very consistent.</sarcasm>
>
> So I I think the existing code is correct, and don't see an upside to
> making this restriction/invisible change in semantics, and it could
> potentially lead to incorrect results if someone is thinking that
> they're entering decimal numbers.
The existing code is correct only perhaps from a historical point of
view. From a functional it's more than flawed. ...
>
> [*] There was one particular document that even went to the trouble of
> explaining how to convert the hex value of slot into a decimal number to
> use in the libvirt config!
Well, this hints that we do actually something wrong here. The ambiguity
in parsing of numbers with virStrToLong_ui has already bitten us (I
won't bother looking up the commit though).
The problem is that if you use 0 as a base argument it's not really
clear what you've parsed and that will result in situations like this.
The developer may be lucky in trying numbers that were parsed correctly
misleading him that he used the correct base.
I think that we shouldn't be using 0 as base in ANY place in our code.
As of this particular case: If we format it in base 16, we should parse
it in base 16. Having ambiguity in the parser code will only end up in
problems.
>
> >
> > Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
> > ---
> > tools/virsh-domain.c | 30 +++++++++++++++---------------
> > 1 file changed, 15 insertions(+), 15 deletions(-)
> >
Peter
A agree with Peter, as there is no reasonable point to represent PCI address in
decimal numbers, only libvirt does that and as said, it's wrong. I don't know
any other place, where the PCI address is printed as decimal number.
I've been motivated to update this behavior after I've used '0000:05:10.1'
as
an argument for the '--srouce' and it parsed the slot as decimal number and
printed in to the XML as '0x0a'.
Pavel
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
11:48 a.m.
New subject: [libvirt] [PATCH 1/4] virsh-domain: use correct base for virStrToLong_ui
On 10/23/2015 03:59 AM, Pavel Hrdina wrote:
On Fri, Oct 23, 2015 at 09:33:49AM +0200, Peter Krempa wrote:
> On Thu, Oct 22, 2015 at 11:24:02 -0400, Laine Stump wrote:
>> On 10/21/2015 08:22 AM, Pavel Hrdina wrote:
>>> While parsing device addresses we should use correct base and don't
>>> count on auto-detect. For example, PCI address uses hex numbers, but
>>> each number starting with 0 will be auto-detected as octal number and
>>> that's wrong. Another wrong use-case is for PCI address if for example
>>> bus is 10, than it's incorrectly parsed as decimal number.
>>>
>>> PCI and CCW addresses have all values as hex numbers, IDE and SCSI
>>> addresses are in decimal numbers.
>> I've seen examples for PCI that use decimal a number for the slot (which
>> is the one item that's likely to have a value > 7 unless you have a
>> system with a whole bunch of PCI controllers)[*], and those that use hex
>> numbers always prefix the number with "0x". libvirt itself always a
> lspci doesn't really use 0x prefix:
My statement above was for any libvirt documentation, not "any program
anywhere displaying a PCI address". The standard notation for a "unified
PCI address" (coining my own term :-) is as they are displayed in lspci
(and in all the entries in sysfs).
>
> 00:16.0 Communication controller: Intel Corporation Wildcat Point-LP MEI Controller
#1 (rev 03)
> 00:19.0 Ethernet controller: Intel Corporation Ethernet Connection (3) I218-LM (rev
03)
> 00:1b.0 Audio device: Intel Corporation Wildcat Point-LP High Definition Audio
Controller (rev 03)
> 00:1c.0 PCI bridge: Intel Corporation Wildcat Point-LP PCI Express Root Port #6 (rev
e3)
>
> Btw that's a laptop, not a super special system.
>
>> prefixes the domain, bus, and slot with 0x, so an auto-detected base
>> will always get those right.
> Well, not entirely:
>
> nodedev identificators use hex, possibly padded with zeroes:
>
> pci_0000_00_03_0
> pci_0000_00_14_0
> pci_0000_00_16_0
> pci_0000_00_19_0
> pci_0000_00_1b_0
>
> Which may result also in some octal 'fun' on boxes with 16 buses ;).
Sigh. Yes, I was only considering the <address> element, not the stuff
in node device XML and virsh nodedev-* output :-(. What you have pointed
out makes me even more wary of changing libvirt's parsing behavior.
>
>
> nodedev XML uses decimal in the parsed address:
>
> $ virsh nodedev-dumpxml pci_0000_00_19_0
> <device>
> <name>pci_0000_00_19_0</name>
> <path>/sys/devices/pci0000:00/0000:00:19.0</path>
> <parent>computer</parent>
> <driver>
> <name>e1000e</name>
> </driver>
> <capability type='pci'>
> <domain>0</domain>
> <bus>0</bus>
> <slot>25</slot>
> <function>0</function>
> <product id='0x15a2'>Ethernet Connection (3)
I218-LM</product>
> <vendor id='0x8086'>Intel Corporation</vendor>
> </capability>
> </device>
>
> (Uhhh, so 19 ... or 25? ... or perhaps 31?)
>
>
> And for hostdevs we are promoting the use of 0x prefixed hex:
>
> <address domain='0x0000' bus='0x06' slot='0x02'
function='0x0'/>
>
> As well as with target addresses. Fortunately.
>
> <sarcasm>Well that's very consistent.</sarcasm>
Yes, the inconsistency is actually part of my point. If we silently
change the parsing to now recognize "25" (no "0x" prefix) as a hex
number, it's going to give the wrong results for any XML written using a
decimal slot# that worked properly on "old" libvirt, and it's going to
make everything confusing in a different way since everyone will need to
keep straight which version of libvirt interprets it which way - you'll
*never* be able to write examples for documentation that omit the "0x"
anyway, since you can't be sure the reader of the documentation has a
new enough version of libvirt to force parsing as hex.
For example, if someone has a script that uses XML they created "a long
time ago" by copy/pasting the slot# from the nodedev-dumpxml output (or
maybe just hand-writing it) into XML that they use for "virsh create" or
"virsh attach-device" (i.e. the XML is stored/generated external to
libvirt in its original form, *not* normalized by the libvirt
parse/format cycle), and upgraded libvirt will magically/silently begin
interpreting any slot# > 9 differently, leading to anything from a
different layout of devices on the guest PCI bus, to failure to attach a
device because it wasn't found (or even worse, silently/erroneously
attaching the *wrong* host device to the guest). I don't think that is
acceptable.
If we want to get rid of the inconsistency, without creating the
possibility of *silently* performing incorrect operations, we should:
* require the input in the <address> element to have a "0x" prefix to be
sure that there is no assumption on the part of the user that they are
entering a decimal number.
* change the node device XML (nodedev-dumpxml) to output the domain,
bus, and slot elements as hex, including the 0x (function is irrelevant,
since by definition it can never be > 7)
Even doing that would carry the danger of causing existing functioning
systems to fail though (although they would at least fail in a vocal
manner).
(BTW, I looked for any places in libvirt that currently parse a number
*in config* as hex without the 0x prefix - the only one I can find is this:
* <address type='spapr-vio' reg='0x3000'/> <- reg is forced
base16
(and a deprecated "devaddr" in the status (so probably unused in many
years) that parses a "unified" address). Anything else that is read from
config is either forced to base 10, or is base 0. So there isn't much
precedence for interpreting numbers in libvirt config with no "0x"
prefix as hex.)
>
>> So I I think the existing code is correct, and don't see an upside to
>> making this restriction/invisible change in semantics, and it could
>> potentially lead to incorrect results if someone is thinking that
>> they're entering decimal numbers.
> The existing code is correct only perhaps from a historical point of
> view. From a functional it's more than flawed. ...
>
>> [*] There was one particular document that even went to the trouble of
>> explaining how to convert the hex value of slot into a decimal number to
>> use in the libvirt config!
> Well, this hints that we do actually something wrong here. The ambiguity
> in parsing of numbers with virStrToLong_ui has already bitten us (I
> won't bother looking up the commit though).
It's not ambiguous to the parser. Any given string is parsed to exactly
one result. The problem is that the users expect the parser to behave
differently than it does. Well, *some* of the users do. Maybe even
*most* of them. But not *all* of them :-).
As for previous problems with differing bases, this reminded me of a
very troublesome problem that I spent the time to look up just to fill
the blank in my memory - commit 8efebd176. This is a case where libvirt
was formatting the bus/device numbers of a USB device on the qemu
commandline with %.3d (forcing leading 0's) and qemu was then
interpreting the numbers as octal, so silently/magically attaching the
wrong USB device. This problem wasn't solved by making qemu always parse
USB bus/device as decimal though; it was instead solved by making
libvirt aware of / follow the rules of qemu's parser. (I realize it's a
bit more difficult to "patch the user", but that is also an option :-)
>
> The problem is that if you use 0 as a base argument it's not really
> clear what you've parsed and that will result in situations like this.
> The developer may be lucky in trying numbers that were parsed correctly
> misleading him that he used the correct base.
>
> I think that we shouldn't be using 0 as base in ANY place in our code.
I think that it is dangerous to have a mix of base 16 and base 10
numbers in a file while not requiring a clear indicator on every number
of what base was used to encode them. Sure there are some numbers that
are obviously hex (an address in memory) and there are some that are
obviously decimal (timeout in seconds), but there are some where it's
just not clear to the user. The case of PCI addresses is one of those
(mostly due to the node device XML. But simply changing that won't
eliminate all the existing examples out on the internet (nor will it
immediately upgrade all the existing installations of libvirt).
(BTW, I also think there is absolutely *no* place for octal
representations of numbers anywhere in anything libvirt does. This is
not 1975, and we're not all (waiting in line for our turn at) sitting in
front of a TTY connected to a PDP-11 :-P)
>
> As of this particular case: If we format it in base 16, we should parse
> it in base 16. Having ambiguity in the parser code will only end up in
> problems.
Okay, then to take that to its logical conclusion - if we format it with
a leading 0x, then we should require the leading 0x when parsing.
>
>>> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
>>> ---
>>> tools/virsh-domain.c | 30 +++++++++++++++---------------
>>> 1 file changed, 15 insertions(+), 15 deletions(-)
>>>
> Peter
A agree with Peter, as there is no reasonable point to represent PCI address in
decimal numbers, only libvirt does that and as said, it's wrong. I don't know
any other place, where the PCI address is printed as decimal number.
I've been motivated to update this behavior after I've used
'0000:05:10.1' as
an argument for the '--srouce' and it parsed the slot as decimal number and
printed in to the XML as '0x0a'.
Yeah I can understand that happening (and your annoyance when it did),
and if all pci address info everywhere in libvirt and its documentation
was (and always had been) output in hex (*without* the leading "0x") I
would probably agree with this patch. But due to the history (and
current state) I think it's just going to create as many problems as it
solves. I *might* agree with it if it was done by requiring a leading
'0x' (having the effect of causing failure for any existing XML that had
been using decimal - I'm not 100% sure that would be acceptable either,
but at least it is a *visible* failure, rather than a silent one), and I
would be more prone to agree if the node device XML was modified to
output the bus/slot/port in hex (with leading 0x - again, I can't say
for sure that even that wouldn't screw up some existing management
application though, so I'm *still* hesitant).
4:53 a.m.
New subject: [libvirt] [PATCH 1/4] virsh-domain: use correct base for virStrToLong_ui
On Fri, Oct 23, 2015 at 12:48:07PM -0400, Laine Stump wrote:
On 10/23/2015 03:59 AM, Pavel Hrdina wrote:
> On Fri, Oct 23, 2015 at 09:33:49AM +0200, Peter Krempa wrote:
>> On Thu, Oct 22, 2015 at 11:24:02 -0400, Laine Stump wrote:
>>> On 10/21/2015 08:22 AM, Pavel Hrdina wrote:
>>>> While parsing device addresses we should use correct base and don't
>>>> count on auto-detect. For example, PCI address uses hex numbers, but
>>>> each number starting with 0 will be auto-detected as octal number and
>>>> that's wrong. Another wrong use-case is for PCI address if for
example
>>>> bus is 10, than it's incorrectly parsed as decimal number.
>>>>
>>>> PCI and CCW addresses have all values as hex numbers, IDE and SCSI
>>>> addresses are in decimal numbers.
>>> I've seen examples for PCI that use decimal a number for the slot
(which
>>> is the one item that's likely to have a value > 7 unless you have a
>>> system with a whole bunch of PCI controllers)[*], and those that use hex
>>> numbers always prefix the number with "0x". libvirt itself always
a
>> lspci doesn't really use 0x prefix:
My statement above was for any libvirt documentation, not "any program
anywhere displaying a PCI address". The standard notation for a "unified
PCI address" (coining my own term :-) is as they are displayed in lspci
(and in all the entries in sysfs).
>>
>> 00:16.0 Communication controller: Intel Corporation Wildcat Point-LP MEI
Controller #1 (rev 03)
>> 00:19.0 Ethernet controller: Intel Corporation Ethernet Connection (3) I218-LM
(rev 03)
>> 00:1b.0 Audio device: Intel Corporation Wildcat Point-LP High Definition Audio
Controller (rev 03)
>> 00:1c.0 PCI bridge: Intel Corporation Wildcat Point-LP PCI Express Root Port #6
(rev e3)
>>
>> Btw that's a laptop, not a super special system.
>>
>>> prefixes the domain, bus, and slot with 0x, so an auto-detected base
>>> will always get those right.
>> Well, not entirely:
>>
>> nodedev identificators use hex, possibly padded with zeroes:
>>
>> pci_0000_00_03_0
>> pci_0000_00_14_0
>> pci_0000_00_16_0
>> pci_0000_00_19_0
>> pci_0000_00_1b_0
>>
>> Which may result also in some octal 'fun' on boxes with 16 buses ;).
Sigh. Yes, I was only considering the <address> element, not the stuff
in node device XML and virsh nodedev-* output :-(. What you have pointed
out makes me even more wary of changing libvirt's parsing behavior.
>>
>>
>> nodedev XML uses decimal in the parsed address:
>>
>> $ virsh nodedev-dumpxml pci_0000_00_19_0
>> <device>
>> <name>pci_0000_00_19_0</name>
>> <path>/sys/devices/pci0000:00/0000:00:19.0</path>
>> <parent>computer</parent>
>> <driver>
>> <name>e1000e</name>
>> </driver>
>> <capability type='pci'>
>> <domain>0</domain>
>> <bus>0</bus>
>> <slot>25</slot>
>> <function>0</function>
>> <product id='0x15a2'>Ethernet Connection (3)
I218-LM</product>
>> <vendor id='0x8086'>Intel Corporation</vendor>
>> </capability>
>> </device>
>>
>> (Uhhh, so 19 ... or 25? ... or perhaps 31?)
>>
>>
>> And for hostdevs we are promoting the use of 0x prefixed hex:
>>
>> <address domain='0x0000' bus='0x06' slot='0x02'
function='0x0'/>
>>
>> As well as with target addresses. Fortunately.
>>
>> <sarcasm>Well that's very consistent.</sarcasm>
Yes, the inconsistency is actually part of my point. If we silently
change the parsing to now recognize "25" (no "0x" prefix) as a hex
number, it's going to give the wrong results for any XML written using a
decimal slot# that worked properly on "old" libvirt, and it's going to
make everything confusing in a different way since everyone will need to
keep straight which version of libvirt interprets it which way - you'll
*never* be able to write examples for documentation that omit the "0x"
anyway, since you can't be sure the reader of the documentation has a
new enough version of libvirt to force parsing as hex.
For example, if someone has a script that uses XML they created "a long
time ago" by copy/pasting the slot# from the nodedev-dumpxml output (or
maybe just hand-writing it) into XML that they use for "virsh create" or
"virsh attach-device" (i.e. the XML is stored/generated external to
libvirt in its original form, *not* normalized by the libvirt
parse/format cycle), and upgraded libvirt will magically/silently begin
interpreting any slot# > 9 differently, leading to anything from a
different layout of devices on the guest PCI bus, to failure to attach a
device because it wasn't found (or even worse, silently/erroneously
attaching the *wrong* host device to the guest). I don't think that is
acceptable.
If we want to get rid of the inconsistency, without creating the
possibility of *silently* performing incorrect operations, we should:
* require the input in the <address> element to have a "0x" prefix to be
sure that there is no assumption on the part of the user that they are
entering a decimal number.
* change the node device XML (nodedev-dumpxml) to output the domain,
bus, and slot elements as hex, including the 0x (function is irrelevant,
since by definition it can never be > 7)
Even doing that would carry the danger of causing existing functioning
systems to fail though (although they would at least fail in a vocal
manner).
(BTW, I looked for any places in libvirt that currently parse a number
*in config* as hex without the 0x prefix - the only one I can find is this:
* <address type='spapr-vio' reg='0x3000'/> <- reg is forced
base16
(and a deprecated "devaddr" in the status (so probably unused in many
years) that parses a "unified" address). Anything else that is read from
config is either forced to base 10, or is base 0. So there isn't much
precedence for interpreting numbers in libvirt config with no "0x"
prefix as hex.)
>>
>>> So I I think the existing code is correct, and don't see an upside to
>>> making this restriction/invisible change in semantics, and it could
>>> potentially lead to incorrect results if someone is thinking that
>>> they're entering decimal numbers.
>> The existing code is correct only perhaps from a historical point of
>> view. From a functional it's more than flawed. ...
>>
>>> [*] There was one particular document that even went to the trouble of
>>> explaining how to convert the hex value of slot into a decimal number to
>>> use in the libvirt config!
>> Well, this hints that we do actually something wrong here. The ambiguity
>> in parsing of numbers with virStrToLong_ui has already bitten us (I
>> won't bother looking up the commit though).
It's not ambiguous to the parser. Any given string is parsed to exactly
one result. The problem is that the users expect the parser to behave
differently than it does. Well, *some* of the users do. Maybe even
*most* of them. But not *all* of them :-).
As for previous problems with differing bases, this reminded me of a
very troublesome problem that I spent the time to look up just to fill
the blank in my memory - commit 8efebd176. This is a case where libvirt
was formatting the bus/device numbers of a USB device on the qemu
commandline with %.3d (forcing leading 0's) and qemu was then
interpreting the numbers as octal, so silently/magically attaching the
wrong USB device. This problem wasn't solved by making qemu always parse
USB bus/device as decimal though; it was instead solved by making
libvirt aware of / follow the rules of qemu's parser. (I realize it's a
bit more difficult to "patch the user", but that is also an option :-)
>>
>> The problem is that if you use 0 as a base argument it's not really
>> clear what you've parsed and that will result in situations like this.
>> The developer may be lucky in trying numbers that were parsed correctly
>> misleading him that he used the correct base.
>>
>> I think that we shouldn't be using 0 as base in ANY place in our code.
I think that it is dangerous to have a mix of base 16 and base 10
numbers in a file while not requiring a clear indicator on every number
of what base was used to encode them. Sure there are some numbers that
are obviously hex (an address in memory) and there are some that are
obviously decimal (timeout in seconds), but there are some where it's
just not clear to the user. The case of PCI addresses is one of those
(mostly due to the node device XML. But simply changing that won't
eliminate all the existing examples out on the internet (nor will it
immediately upgrade all the existing installations of libvirt).
(BTW, I also think there is absolutely *no* place for octal
representations of numbers anywhere in anything libvirt does. This is
not 1975, and we're not all (waiting in line for our turn at) sitting in
front of a TTY connected to a PDP-11 :-P)
>>
>> As of this particular case: If we format it in base 16, we should parse
>> it in base 16. Having ambiguity in the parser code will only end up in
>> problems.
Okay, then to take that to its logical conclusion - if we format it with
a leading 0x, then we should require the leading 0x when parsing.
>>
>>>> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
>>>> ---
>>>> tools/virsh-domain.c | 30 +++++++++++++++---------------
>>>> 1 file changed, 15 insertions(+), 15 deletions(-)
>>>>
>> Peter
> A agree with Peter, as there is no reasonable point to represent PCI address in
> decimal numbers, only libvirt does that and as said, it's wrong. I don't
know
> any other place, where the PCI address is printed as decimal number.
>
> I've been motivated to update this behavior after I've used
'0000:05:10.1' as
> an argument for the '--srouce' and it parsed the slot as decimal number and
> printed in to the XML as '0x0a'.
Yeah I can understand that happening (and your annoyance when it did),
and if all pci address info everywhere in libvirt and its documentation
was (and always had been) output in hex (*without* the leading "0x") I
would probably agree with this patch. But due to the history (and
current state) I think it's just going to create as many problems as it
solves. I *might* agree with it if it was done by requiring a leading
'0x' (having the effect of causing failure for any existing XML that had
been using decimal - I'm not 100% sure that would be acceptable either,
but at least it is a *visible* failure, rather than a silent one), and I
would be more prone to agree if the node device XML was modified to
output the bus/slot/port in hex (with leading 0x - again, I can't say
for sure that even that wouldn't screw up some existing management
application though, so I'm *still* hesitant).
The current situation is not ideal, it's not documented anywhere and for users
you can only try the command and see what happens. Yes, it can and probably
will break some scripts for some users, but I think we should make it somehow
consistent and document it properly how to format it.
The output of nodedev-dumpxml should be definitely fixed to print the PCI
address using only hex numbers.
For the parsing part, this code is currently used only for 'attach-disk' and
the 'attach-interface' will be a new functionality and we can easily restrict
the format of PCI address to be provided only in hex numbers with '0x' prefix.
The only issue here is the 'attach-disk' where we could break some user's
use-cases.
Pavel
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
8:48 a.m.
New subject: [libvirt] [PATCH 1/4] virsh-domain: use correct base for virStrToLong_ui
On 10/26/2015 05:53 AM, Pavel Hrdina wrote:
The current situation is not ideal, it's not documented anywhere
and for users
you can only try the command and see what happens. Yes, it can and probably
will break some scripts for some users, but I think we should make it somehow
consistent and document it properly how to format it.
The output of nodedev-dumpxml should be definitely fixed to print the PCI
address using only hex numbers.
For the parsing part, this code is currently used only for 'attach-disk' and
the 'attach-interface' will be a new functionality and we can easily restrict
the format of PCI address to be provided only in hex numbers with '0x' prefix.
/me wakes up Monday morning, reads this paragraph, and realizes that he
should have looked at the entire patch before commenting :-/
Yeah, forget everything I said. I hadn't scrolled all the way to the
bottom of the patch, but just assumed from the commit message that it
was going to fix the base at 16 for the XML parser too. Pretty stupid of
me - sorry for all the noise over nothing.
Definitely any time someone is writing a unified PCI address
(DDDD:BB:SS.F) I think we can safely assume/require they are using hex.
Really, I'm surprised this didn't trip anyone up before now.
ACK.
9:26 a.m.
New subject: [libvirt] [PATCH 1/4] virsh-domain: use correct base for virStrToLong_ui
On Mon, Oct 26, 2015 at 09:48:20AM -0400, Laine Stump wrote:
On 10/26/2015 05:53 AM, Pavel Hrdina wrote:
> The current situation is not ideal, it's not documented anywhere and for users
> you can only try the command and see what happens. Yes, it can and probably
> will break some scripts for some users, but I think we should make it somehow
> consistent and document it properly how to format it.
>
> The output of nodedev-dumpxml should be definitely fixed to print the PCI
> address using only hex numbers.
>
> For the parsing part, this code is currently used only for 'attach-disk'
and
> the 'attach-interface' will be a new functionality and we can easily
restrict
> the format of PCI address to be provided only in hex numbers with '0x'
prefix.
/me wakes up Monday morning, reads this paragraph, and realizes that he
should have looked at the entire patch before commenting :-/
Yeah, forget everything I said. I hadn't scrolled all the way to the
bottom of the patch, but just assumed from the commit message that it
was going to fix the base at 16 for the XML parser too. Pretty stupid of
me - sorry for all the noise over nothing.
Definitely any time someone is writing a unified PCI address
(DDDD:BB:SS.F) I think we can safely assume/require they are using hex.
Really, I'm surprised this didn't trip anyone up before now.
ACK.
Thanks :) pushed now
Pavel
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
7:22 a.m.
New subject: [libvirt] [PATCH 2/4] virsh-nodedev: makes struct and functions for NodeDevice list available
Next patch will use those function to collect NodeDevice list and find a
specific device. Make functions virshNodeDeviceListCollect() and
virshNodeDeviceListFree() together with struct virshNodeDeviceList
available to reuse existing code.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
tools/virsh-nodedev.c | 16 +++++-----------
tools/virsh-nodedev.h | 11 +++++++++++
2 files changed, 16 insertions(+), 11 deletions(-)
diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c
index cc359e2..26f2c7b 100644
--- a/tools/virsh-nodedev.c
+++ b/tools/virsh-nodedev.c
@@ -194,13 +194,7 @@ virshNodeDeviceSorter(const void *a, const void *b)
virNodeDeviceGetName(*nb));
}
-struct virshNodeDeviceList {
- virNodeDevicePtr *devices;
- size_t ndevices;
-};
-typedef struct virshNodeDeviceList *virshNodeDeviceListPtr;
-
-static void
+void
virshNodeDeviceListFree(virshNodeDeviceListPtr list)
{
size_t i;
@@ -215,11 +209,11 @@ virshNodeDeviceListFree(virshNodeDeviceListPtr list)
VIR_FREE(list);
}
-static virshNodeDeviceListPtr
+virshNodeDeviceListPtr
virshNodeDeviceListCollect(vshControl *ctl,
- char **capnames,
- int ncapnames,
- unsigned int flags)
+ char **capnames,
+ int ncapnames,
+ unsigned int flags)
{
virshNodeDeviceListPtr list = vshMalloc(ctl, sizeof(*list));
size_t i;
diff --git a/tools/virsh-nodedev.h b/tools/virsh-nodedev.h
index c64f7df..1d2337b 100644
--- a/tools/virsh-nodedev.h
+++ b/tools/virsh-nodedev.h
@@ -30,4 +30,15 @@
extern const vshCmdDef nodedevCmds[];
+struct virshNodeDeviceList {
+ virNodeDevicePtr *devices;
+ size_t ndevices;
+};
+typedef struct virshNodeDeviceList *virshNodeDeviceListPtr;
+
+virshNodeDeviceListPtr virshNodeDeviceListCollect(vshControl *ctl,
+ char **capnames,
+ int ncapnames,
+ unsigned int flags);
+void virshNodeDeviceListFree(virshNodeDeviceListPtr list);
#endif /* VIRSH_NODEDEV_H */
--
2.6.2
7:22 a.m.
New subject: [libvirt] [PATCH 3/4] virsh-domain: update attach-interface to support type=hostdev
Adding this feature will allow users to easily attach a hostdev network
interface using PCI passthrough.
The interface can be attached using --type=hostdev and PCI address or
network device name as --source. This command also allows you to tell,
whether the interface should be managed and to choose a assignment
driver.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=997561
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
tools/virsh-domain.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 86 insertions(+), 4 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index e8503ec..b124441 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -56,6 +56,7 @@
#include "virtime.h"
#include "virtypedparam.h"
#include "virxml.h"
+#include "virsh-nodedev.h"
/* Gnulib doesn't guarantee SA_SIGINFO support. */
#ifndef SA_SIGINFO
@@ -866,6 +867,14 @@ static const vshCmdOptDef opts_attach_interface[] = {
.type = VSH_OT_BOOL,
.help = N_("print XML document rather than attach the interface")
},
+ {.name = "managed",
+ .type = VSH_OT_BOOL,
+ .help = N_("set the interface to be managed by libvirt")
+ },
+ {.name = "driver",
+ .type = VSH_OT_STRING,
+ .help = N_("set driver for hostdev interface, default is 'kvm'")
+ },
{.name = NULL}
};
@@ -919,7 +928,7 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
virDomainPtr dom = NULL;
const char *mac = NULL, *target = NULL, *script = NULL,
*type = NULL, *source = NULL, *model = NULL,
- *inboundStr = NULL, *outboundStr = NULL;
+ *inboundStr = NULL, *outboundStr = NULL, *driver = NULL;
virNetDevBandwidthRate inbound, outbound;
virDomainNetType typ;
int ret;
@@ -931,6 +940,7 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
bool config = vshCommandOptBool(cmd, "config");
bool live = vshCommandOptBool(cmd, "live");
bool persistent = vshCommandOptBool(cmd, "persistent");
+ bool managed = vshCommandOptBool(cmd, "managed");
VSH_EXCLUSIVE_OPTIONS_VAR(persistent, current);
@@ -949,7 +959,8 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
vshCommandOptStringReq(ctl, cmd, "script", &script) < 0 ||
vshCommandOptStringReq(ctl, cmd, "model", &model) < 0 ||
vshCommandOptStringReq(ctl, cmd, "inbound", &inboundStr) < 0 ||
- vshCommandOptStringReq(ctl, cmd, "outbound", &outboundStr) < 0)
+ vshCommandOptStringReq(ctl, cmd, "outbound", &outboundStr) < 0
||
+ vshCommandOptStringReq(ctl, cmd, "driver", &driver) < 0)
goto cleanup;
/* check interface type */
@@ -982,8 +993,23 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
}
}
+ if (typ != VIR_DOMAIN_NET_TYPE_HOSTDEV) {
+ if (managed) {
+ vshError(ctl, _("--managed is usable only with --type=hostdev"));
+ goto cleanup;
+ }
+ if (driver) {
+ vshError(ctl, _("--driver is usable only with --type=hostdev"));
+ goto cleanup;
+ }
+ }
+
/* Make XML of interface */
- virBufferAsprintf(&buf, "<interface type='%s'>\n",
type);
+ virBufferAsprintf(&buf, "<interface type='%s'", type);
+ if (managed)
+ virBufferAddLit(&buf, " managed='yes'>\n");
+ else
+ virBufferAddLit(&buf, ">\n");
virBufferAdjustIndent(&buf, 2);
switch (typ) {
@@ -995,6 +1021,63 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
case VIR_DOMAIN_NET_TYPE_DIRECT:
virBufferAsprintf(&buf, "<source dev='%s'/>\n",
source);
break;
+ case VIR_DOMAIN_NET_TYPE_HOSTDEV:
+ {
+ struct PCIAddress pciAddr = {0, 0, 0, 0};
+
+ if (str2PCIAddress(source, &pciAddr) < 0) {
+ const char *caps[] = {"net"};
+ char *tmpName = NULL;
+ virshNodeDeviceListPtr list = NULL;
+ virNodeDevicePtr netDev = NULL;
+ size_t i;
+
+ list = virshNodeDeviceListCollect(ctl, (char **)caps, 1,
+ VIR_CONNECT_LIST_NODE_DEVICES_CAP_NET);
+
+ if (!list) {
+ vshError(ctl, _("cannot list network devices"));
+ goto cleanup;
+ }
+
+ if (virAsprintf(&tmpName, "net_%s", source) < 0)
+ goto cleanup;
+
+ for (i = 0; i < list->ndevices; i++) {
+ if (STREQLEN(tmpName, virNodeDeviceGetName(list->devices[i]),
+ strlen(tmpName)))
+ netDev = list->devices[i];
+ }
+ VIR_FREE(tmpName);
+
+ if (!netDev) {
+ vshError(ctl, _("network interface '%s' doesn't
exist"),
+ source);
+ goto cleanup;
+ }
+
+ if (str2PCIAddress(virNodeDeviceGetParent(netDev)+4, &pciAddr) < 0) {
+ virshNodeDeviceListFree(list);
+ vshError(ctl, _("cannot parse pci address for network "
+ "interface '%s'"), source);
+ goto cleanup;
+ }
+
+ virshNodeDeviceListFree(list);
+ }
+ if (driver)
+ virBufferAsprintf(&buf, "<driver name='%s'/>\n",
driver);
+
+ virBufferAddLit(&buf, "<source>\n");
+ virBufferAdjustIndent(&buf, 2);
+ virBufferAsprintf(&buf, "<address type='pci'
domain='0x%.4x'"
+ " bus='0x%.2x' slot='0x%.2x'
function='0x%.1x'/>\n",
+ pciAddr.domain, pciAddr.bus,
+ pciAddr.slot, pciAddr.function);
+ virBufferAdjustIndent(&buf, -2);
+ virBufferAddLit(&buf, "</source>\n");
+ break;
+ }
case VIR_DOMAIN_NET_TYPE_USER:
case VIR_DOMAIN_NET_TYPE_ETHERNET:
@@ -1004,7 +1087,6 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
case VIR_DOMAIN_NET_TYPE_MCAST:
case VIR_DOMAIN_NET_TYPE_UDP:
case VIR_DOMAIN_NET_TYPE_INTERNAL:
- case VIR_DOMAIN_NET_TYPE_HOSTDEV:
case VIR_DOMAIN_NET_TYPE_LAST:
vshError(ctl, _("No support for %s in command
'attach-interface'"),
type);
--
2.6.2
3:05 a.m.
New subject: [libvirt] [PATCH 3/4] virsh-domain: update attach-interface to support type=hostdev
On 10/21/2015 08:22 AM, Pavel Hrdina wrote:
Adding this feature will allow users to easily attach a hostdev
network
interface using PCI passthrough.
The interface can be attached using --type=hostdev and PCI address or
network device name as --source. This command also allows you to tell,
whether the interface should be managed and to choose a assignment
driver.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=997561
I think the PCI address version of this command is fairly
straightforward, so it can and should go in. But I've been thinking more
about the "netdev name" version since our exchange in the BZ, and even
had a very long treatise prepared defending my position from there, but
then I decided to look at it all again from the beginning and came to
the conclusion that we are *both* wrong :-)
The main aim here is convenience, and while I still have the position
that if we're going to make it more convenient, we should make it more
convenient at the XML level so that all consumers of libvirt can take
advantage without needing a ton of extra code, I also have realized that
specifying the netdev name is not very convenient anyway.
Why?
* Keep in mind that <interface type='hostdev'> will only work with SRIOV
VFs (because you can't set the MAC address of a normal netdev from the
host and have that MAC address persist through the guest driver's device
init).
* So any device that you assign in this way is a VF.
* A user will know which PF ("Physical Function", but from their point
of view it's "the physical port used for the connection") they want a VF
from; they don't care which VF (they're all functionally equivalent),
and anyway the standard utilities don't even tell you the netdev names
of the VFs associated with a particular PF. So it's not that simple to
learn the netdev name of the VF you want to use:
* virsh nodedev-list --tree doesn't group the VFs under their PF
(because its hierarchy is according to the PCI bus layout, which has
all the PFs and VFs at the same level)
* virsh nodedev-dumpxml for the PF device shows the VFs'
PCI addresses, NOT their netdev names.
<capability type='virt_functions'>
<address domain='0x0000' bus='0x02' slot='0x10'
function='0x0'/>
<address domain='0x0000' bus='0x02' slot='0x10'
function='0x2'/>
<address domain='0x0000' bus='0x02' slot='0x10'
function='0x4'/>
<address domain='0x0000' bus='0x02' slot='0x10'
function='0x6'/>
<address domain='0x0000' bus='0x02' slot='0x11'
function='0x0'/>
<address domain='0x0000' bus='0x02' slot='0x11'
function='0x2'/>
<address domain='0x0000' bus='0x02' slot='0x11'
function='0x4'/>
(Note the leading "0x" on these values :-P)
* "ip link show" lists the VFs directly under their PF,
but shows the VF#, not the netdev name
11: p4p2: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq
state UP mode DEFAULT group default qlen 1000
link/ether 90:e2:ba:02:22:01 brd ff:ff:ff:ff:ff:ff
vf 0 MAC 00:00:00:00:00:00, spoof checking on, link-state auto
vf 1 MAC 00:00:00:00:00:00, spoof checking on, link-state auto
vf 2 MAC 00:00:00:00:00:00, spoof checking on, link-state auto
vf 3 MAC 00:00:00:00:00:00, spoof checking on, link-state auto
vf 4 MAC 00:00:00:00:00:00, spoof checking on, link-state auto
vf 5 MAC 00:00:00:00:00:00, spoof checking on, link-state auto
vf 6 MAC 00:00:00:00:00:00, spoof checking on, link-state auto
So if there's not a simple way to get a list of the netdev names of the
VFs for a PF, then what convenience is gained by allowing use of netdev
name? What exactly was I thinking when I wrote comment 1 in that BZ???
What *could* be useful would be the ability to specify the PF name and
VF# of the device you want to assign, e.g.: "<source pf='p4p2'
vf='3'/>", since that's info you can easily get from "ip link
show".
Anymore, though, I don't even know how useful *that* is, since you can
already get the PCI addresses of the VFs from the output of virsh
nodedev-dumpxml (in almost exactly the format you need - just add
"type='pci'".
(I'm now even wondering if I misunderstood what the original reporter
was asking for, but unfortunately it's not possible to ask him, because
his account has been closed :-( Thinking back to what he said - there is
a *different* place where it's common to know the netdev name and want
to translate it into a PCI address - when you are assigning a
*non-SRIOV* ethernet device using plain <hostdev> (not <interface
type='hostdev'>, which only works for SRIOV VFs). In this case you
probably know the netdev name but not the PCI address. So for this case
a translation from netdev name to PCI address would make sense, and here
I would agree that the translation should happen in virsh rather than
the generic <hostdev> XML understanding what a netdev name is (contrary
to <interface type='hostdev'>, where it is well established that the
device you're assigning is a network device, and there are already
"netdev-y" config attributes, up until now <hostdev> has not contained
any config items specific to a particular type of PCI device, and I
don't think it should have any added)).
TL;DR of my opinion:
1) this patch minus the netdev-->PCI address translation is good
2) we don't need the netdev-->PCI translation for <interface
type='hostdev'>;
it's just extra code for (in my newly revised opinion) no gain.
3) a netdev-->PCI translation in virsh that creates a plain
<hostdev> might be useful (hmm, maybe keep (2) as you have it,
with an additional check for
4) adding <source pf='ethX' vf='n'/> support to <interface
type='hostdev'>
might be useful.
How much sense does any of that make?
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
tools/virsh-domain.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 86 insertions(+), 4 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index e8503ec..b124441 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -56,6 +56,7 @@
#include "virtime.h"
#include "virtypedparam.h"
#include "virxml.h"
+#include "virsh-nodedev.h"
/* Gnulib doesn't guarantee SA_SIGINFO support. */
#ifndef SA_SIGINFO
@@ -866,6 +867,14 @@ static const vshCmdOptDef opts_attach_interface[] = {
.type = VSH_OT_BOOL,
.help = N_("print XML document rather than attach the interface")
},
+ {.name = "managed",
+ .type = VSH_OT_BOOL,
+ .help = N_("set the interface to be managed by libvirt")
Maybe a more descriptive help - "have libvirt automatically manage
detach/attach of device from host driver" (or something like that; I
know that's too long).
+ },
+ {.name = "driver",
+ .type = VSH_OT_STRING,
+ .help = N_("set driver for hostdev interface, default is 'kvm'")
Actually the default behavior depends on what is available on the host -
if only one of legacy-kvm and vfio is available, then that is the one
used, but if both are available, then vfio is used (so vfio is closer to
being "default" than kvm). The kvm method of assigning devices is
deprecated, and it has been completely disabled in some distros
(definitely RHEL7 and CentOS7, not sure about Fedora). These days
manually specifying which driver to use is mostly pointless; I vaguely
recall that it was initially added because when VFIO was first added
some were nervous about not having a simple way to fallback to old
behavior if something went wrong while using VFIO; those days are long
gone though, and I can't think of a situation where anyone would
want/need to specify which driver to use (i.e. I'm not sure we even need
to expose this option in virsh; it may confuse more than help).
+ },
{.name = NULL}
};
@@ -919,7 +928,7 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
virDomainPtr dom = NULL;
const char *mac = NULL, *target = NULL, *script = NULL,
*type = NULL, *source = NULL, *model = NULL,
- *inboundStr = NULL, *outboundStr = NULL;
+ *inboundStr = NULL, *outboundStr = NULL, *driver = NULL;
virNetDevBandwidthRate inbound, outbound;
virDomainNetType typ;
int ret;
@@ -931,6 +940,7 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
bool config = vshCommandOptBool(cmd, "config");
bool live = vshCommandOptBool(cmd, "live");
bool persistent = vshCommandOptBool(cmd, "persistent");
+ bool managed = vshCommandOptBool(cmd, "managed");
VSH_EXCLUSIVE_OPTIONS_VAR(persistent, current);
@@ -949,7 +959,8 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
vshCommandOptStringReq(ctl, cmd, "script", &script) < 0 ||
vshCommandOptStringReq(ctl, cmd, "model", &model) < 0 ||
vshCommandOptStringReq(ctl, cmd, "inbound", &inboundStr) < 0
||
- vshCommandOptStringReq(ctl, cmd, "outbound", &outboundStr) <
0)
+ vshCommandOptStringReq(ctl, cmd, "outbound", &outboundStr) < 0
||
+ vshCommandOptStringReq(ctl, cmd, "driver", &driver) < 0)
goto cleanup;
/* check interface type */
@@ -982,8 +993,23 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
}
}
+ if (typ != VIR_DOMAIN_NET_TYPE_HOSTDEV) {
+ if (managed) {
+ vshError(ctl, _("--managed is usable only with --type=hostdev"));
+ goto cleanup;
+ }
+ if (driver) {
+ vshError(ctl, _("--driver is usable only with --type=hostdev"));
+ goto cleanup;
+ }
+ }
+
/* Make XML of interface */
- virBufferAsprintf(&buf, "<interface type='%s'>\n",
type);
+ virBufferAsprintf(&buf, "<interface type='%s'", type);
+ if (managed)
+ virBufferAddLit(&buf, " managed='yes'>\n");
+ else
+ virBufferAddLit(&buf, ">\n");
virBufferAdjustIndent(&buf, 2);
switch (typ) {
@@ -995,6 +1021,63 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
case VIR_DOMAIN_NET_TYPE_DIRECT:
virBufferAsprintf(&buf, "<source dev='%s'/>\n",
source);
break;
+ case VIR_DOMAIN_NET_TYPE_HOSTDEV:
+ {
+ struct PCIAddress pciAddr = {0, 0, 0, 0};
+
+ if (str2PCIAddress(source, &pciAddr) < 0) {
+ const char *caps[] = {"net"};
+ char *tmpName = NULL;
+ virshNodeDeviceListPtr list = NULL;
+ virNodeDevicePtr netDev = NULL;
+ size_t i;
+
+ list = virshNodeDeviceListCollect(ctl, (char **)caps, 1,
+ VIR_CONNECT_LIST_NODE_DEVICES_CAP_NET);
+
+ if (!list) {
+ vshError(ctl, _("cannot list network devices"));
+ goto cleanup;
+ }
+
+ if (virAsprintf(&tmpName, "net_%s", source) < 0)
+ goto cleanup;
+
+ for (i = 0; i < list->ndevices; i++) {
+ if (STREQLEN(tmpName, virNodeDeviceGetName(list->devices[i]),
+ strlen(tmpName)))
+ netDev = list->devices[i];
+ }
+ VIR_FREE(tmpName);
+
+ if (!netDev) {
+ vshError(ctl, _("network interface '%s' doesn't
exist"),
+ source);
+ goto cleanup;
+ }
+
+ if (str2PCIAddress(virNodeDeviceGetParent(netDev)+4, &pciAddr) < 0)
{
+ virshNodeDeviceListFree(list);
+ vshError(ctl, _("cannot parse pci address for network "
+ "interface '%s'"), source);
+ goto cleanup;
+ }
+
+ virshNodeDeviceListFree(list);
+ }
+ if (driver)
+ virBufferAsprintf(&buf, "<driver name='%s'/>\n",
driver);
+
+ virBufferAddLit(&buf, "<source>\n");
+ virBufferAdjustIndent(&buf, 2);
+ virBufferAsprintf(&buf, "<address type='pci'
domain='0x%.4x'"
+ " bus='0x%.2x' slot='0x%.2x'
function='0x%.1x'/>\n",
+ pciAddr.domain, pciAddr.bus,
+ pciAddr.slot, pciAddr.function);
+ virBufferAdjustIndent(&buf, -2);
+ virBufferAddLit(&buf, "</source>\n");
+ break;
+ }
case VIR_DOMAIN_NET_TYPE_USER:
case VIR_DOMAIN_NET_TYPE_ETHERNET:
@@ -1004,7 +1087,6 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
case VIR_DOMAIN_NET_TYPE_MCAST:
case VIR_DOMAIN_NET_TYPE_UDP:
case VIR_DOMAIN_NET_TYPE_INTERNAL:
- case VIR_DOMAIN_NET_TYPE_HOSTDEV:
case VIR_DOMAIN_NET_TYPE_LAST:
vshError(ctl, _("No support for %s in command
'attach-interface'"),
type);
8:38 a.m.
New subject: [libvirt] [PATCH 3/4] virsh-domain: update attach-interface to support type=hostdev
On Mon, Oct 26, 2015 at 04:05:28AM -0400, Laine Stump wrote:
On 10/21/2015 08:22 AM, Pavel Hrdina wrote:
> Adding this feature will allow users to easily attach a hostdev network
> interface using PCI passthrough.
>
> The interface can be attached using --type=hostdev and PCI address or
> network device name as --source. This command also allows you to tell,
> whether the interface should be managed and to choose a assignment
> driver.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=997561
I think the PCI address version of this command is fairly
straightforward, so it can and should go in. But I've been thinking more
about the "netdev name" version since our exchange in the BZ, and even
had a very long treatise prepared defending my position from there, but
then I decided to look at it all again from the beginning and came to
the conclusion that we are *both* wrong :-)
The main aim here is convenience, and while I still have the position
that if we're going to make it more convenient, we should make it more
convenient at the XML level so that all consumers of libvirt can take
advantage without needing a ton of extra code, I also have realized that
specifying the netdev name is not very convenient anyway.
Why?
* Keep in mind that <interface type='hostdev'> will only work with SRIOV
VFs (because you can't set the MAC address of a normal netdev from the
host and have that MAC address persist through the guest driver's device
init).
* So any device that you assign in this way is a VF.
* A user will know which PF ("Physical Function", but from their point
of view it's "the physical port used for the connection") they want a VF
from; they don't care which VF (they're all functionally equivalent),
and anyway the standard utilities don't even tell you the netdev names
of the VFs associated with a particular PF. So it's not that simple to
learn the netdev name of the VF you want to use:
* virsh nodedev-list --tree doesn't group the VFs under their PF
(because its hierarchy is according to the PCI bus layout, which has
all the PFs and VFs at the same level)
* virsh nodedev-dumpxml for the PF device shows the VFs'
PCI addresses, NOT their netdev names.
<capability type='virt_functions'>
<address domain='0x0000' bus='0x02' slot='0x10'
function='0x0'/>
<address domain='0x0000' bus='0x02' slot='0x10'
function='0x2'/>
<address domain='0x0000' bus='0x02' slot='0x10'
function='0x4'/>
<address domain='0x0000' bus='0x02' slot='0x10'
function='0x6'/>
<address domain='0x0000' bus='0x02' slot='0x11'
function='0x0'/>
<address domain='0x0000' bus='0x02' slot='0x11'
function='0x2'/>
<address domain='0x0000' bus='0x02' slot='0x11'
function='0x4'/>
(Note the leading "0x" on these values :-P)
* "ip link show" lists the VFs directly under their PF,
but shows the VF#, not the netdev name
11: p4p2: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq
state UP mode DEFAULT group default qlen 1000
link/ether 90:e2:ba:02:22:01 brd ff:ff:ff:ff:ff:ff
vf 0 MAC 00:00:00:00:00:00, spoof checking on, link-state auto
vf 1 MAC 00:00:00:00:00:00, spoof checking on, link-state auto
vf 2 MAC 00:00:00:00:00:00, spoof checking on, link-state auto
vf 3 MAC 00:00:00:00:00:00, spoof checking on, link-state auto
vf 4 MAC 00:00:00:00:00:00, spoof checking on, link-state auto
vf 5 MAC 00:00:00:00:00:00, spoof checking on, link-state auto
vf 6 MAC 00:00:00:00:00:00, spoof checking on, link-state auto
So if there's not a simple way to get a list of the netdev names of the
VFs for a PF, then what convenience is gained by allowing use of netdev
name? What exactly was I thinking when I wrote comment 1 in that BZ???
What *could* be useful would be the ability to specify the PF name and
VF# of the device you want to assign, e.g.: "<source pf='p4p2'
vf='3'/>", since that's info you can easily get from "ip link
show".
Anymore, though, I don't even know how useful *that* is, since you can
already get the PCI addresses of the VFs from the output of virsh
nodedev-dumpxml (in almost exactly the format you need - just add
"type='pci'".
(I'm now even wondering if I misunderstood what the original reporter
was asking for, but unfortunately it's not possible to ask him, because
his account has been closed :-( Thinking back to what he said - there is
a *different* place where it's common to know the netdev name and want
to translate it into a PCI address - when you are assigning a
*non-SRIOV* ethernet device using plain <hostdev> (not <interface
type='hostdev'>, which only works for SRIOV VFs). In this case you
probably know the netdev name but not the PCI address. So for this case
a translation from netdev name to PCI address would make sense, and here
I would agree that the translation should happen in virsh rather than
the generic <hostdev> XML understanding what a netdev name is (contrary
to <interface type='hostdev'>, where it is well established that the
device you're assigning is a network device, and there are already
"netdev-y" config attributes, up until now <hostdev> has not contained
any config items specific to a particular type of PCI device, and I
don't think it should have any added)).
TL;DR of my opinion:
1) this patch minus the netdev-->PCI address translation is good
I don't mind if the patch goes in with or without the translation. The main
purpose of this extension is to make it easier for users so they don't have to
use the generic 'attach-device' with XML definition but only specify some
arguments. But see the comment below for the 2).
2) we don't need the netdev-->PCI translation for <interface
type='hostdev'>;
it's just extra code for (in my newly revised opinion) no gain.
Actually it's not entirely true, the netdev names of VFs are listed by
'ip link show' command, but it's not clear to guess the relation to PFs:
3: enp5s0f1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP
mode DEFAULT qlen 1000
link/ether b4:b5:2f:66:3c:4f brd ff:ff:ff:ff:ff:ff
vf 0 MAC 22:67:e2:b0:28:b0, spoof checking on, link-state auto
vf 1 MAC 0a:d3:63:e6:06:45, spoof checking on, link-state auto
vf 2 MAC 52:54:00:32:93:5c, spoof checking on, link-state auto
vf 3 MAC 42:9a:79:f6:9d:cb, spoof checking on, link-state auto
14: enp5s16f1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast
state UP mode DEFAULT qlen 1000
link/ether 22:67:e2:b0:28:b0 brd ff:ff:ff:ff:ff:ff
One could benefit using the netdev name also for VF.
3) a netdev-->PCI translation in virsh that creates a plain
<hostdev> might be useful (hmm, maybe keep (2) as you have it,
with an additional check for
Yes, this might be a useful extension of 'attach-interface --type=hostdev', that
we would ether detect, whether the interface is a VF or we would use <hostdev>
as default, unless a mac address is specified or we can combine both approaches.
In this case, the command will remain the same, but it will be able to attach
VF and non-VF interfaces to the guest. This would probably require to extend
a detach-interface to be able to detach also a network interface attached as
<hostdev>.
4) adding <source pf='ethX' vf='n'/> support to
<interface type='hostdev'>
might be useful.
I don't like to extend our XMLs to introduce another way how to represent some
source device if we already have one. And I don't see the value of the PF and
VF in XML.
How much sense does any of that make?
>
> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
> ---
> tools/virsh-domain.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 86 insertions(+), 4 deletions(-)
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index e8503ec..b124441 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -56,6 +56,7 @@
> #include "virtime.h"
> #include "virtypedparam.h"
> #include "virxml.h"
> +#include "virsh-nodedev.h"
>
> /* Gnulib doesn't guarantee SA_SIGINFO support. */
> #ifndef SA_SIGINFO
> @@ -866,6 +867,14 @@ static const vshCmdOptDef opts_attach_interface[] = {
> .type = VSH_OT_BOOL,
> .help = N_("print XML document rather than attach the interface")
> },
> + {.name = "managed",
> + .type = VSH_OT_BOOL,
> + .help = N_("set the interface to be managed by libvirt")
Maybe a more descriptive help - "have libvirt automatically manage
detach/attach of device from host driver" (or something like that; I
know that's too long).
That makes sense, I'll try to come up with something better.
> + },
> + {.name = "driver",
> + .type = VSH_OT_STRING,
> + .help = N_("set driver for hostdev interface, default is
'kvm'")
Actually the default behavior depends on what is available on the host -
if only one of legacy-kvm and vfio is available, then that is the one
used, but if both are available, then vfio is used (so vfio is closer to
being "default" than kvm). The kvm method of assigning devices is
deprecated, and it has been completely disabled in some distros
(definitely RHEL7 and CentOS7, not sure about Fedora). These days
manually specifying which driver to use is mostly pointless; I vaguely
recall that it was initially added because when VFIO was first added
some were nervous about not having a simple way to fallback to old
behavior if something went wrong while using VFIO; those days are long
gone though, and I can't think of a situation where anyone would
want/need to specify which driver to use (i.e. I'm not sure we even need
to expose this option in virsh; it may confuse more than help).
In that case, the documentation for <interface type='hostdev'> is not
correct,
because there is a statement '(or simply omit the <driver> element, since
"kvm"
is currently the default)'. Looking at the <hostdev> section in our
documentation, this is already updated and there is a correct description, when
is which driver used as default. In that case, I'll remove it from virsh.
Pavel
7:22 a.m.
New subject: [libvirt] [PATCH 4/4] virsh.pod: update and improve a attach-interface section
Rewrite the attach-interface section in man page to be more readable and
add the new hostdev functionality.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
tools/virsh.pod | 85 +++++++++++++++++++++++++++++++++++----------------------
1 file changed, 53 insertions(+), 32 deletions(-)
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 0212e7a..063b0d2 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -2507,51 +2507,72 @@ Likewise, I<--shareable> is an alias for I<--mode
shareable>.
[[[I<--live>] [I<--config>] | [I<--current>]] |
[I<--persistent>]]
[I<--target target>] [I<--mac mac>] [I<--script script>] [I<--model
model>]
[I<--inbound average,peak,burst,floor>] [I<--outbound average,peak,burst>]
-[I<--print-xml>]
-
-Attach a new network interface to the domain. I<type> can be
-I<network> to indicate connection via a libvirt virtual network, or
-I<bridge> to indicate connection via a bridge device on the host, or
-I<direct> to indicate connection directly to one of the host's network
-interfaces or bridges. I<source> indicates the source of the
-connection (the name of a network, or of a bridge device, or the
-host's network interfaces or bridges). I<target> is used to specify
-the tap/macvtap device to be used to connect the domain to the
-source. Names starting with 'vnet' are considered as auto-generated
-and are blanked out/regenerated each time the interface is attached.
-I<mac> specifies the MAC address of the network interface; if a MAC
+[I<--managed>] [I<--driver driver>] [I<--print-xml>]
+
+Attach a new network interface to the domain.
+
+B<--type> can be one of the: I<network> to indicate connection via a libvirt
+virtual network, I<bridge> to indicate connection via a bridge device
+on the host, I<direct> to indicate connection directly to one of the host's
+network interfaces or bridges, I<hostdev> to indicate connection using a
+passthrough of PCI device on the host.
+
+B<--source> indicates the source of the connection. The source depends
+on the type of the interface: I<network> name of the virtual network,
+I<bridge> the name of the bridge device, I<direct> the name of the
host's
+interface or bridge, I<hostdev> the name of the host's interface or
+it's PCI address.
+
+B<--target> is used to specify the tap/macvtap device to be used to
+connect the domain to the source. Names starting with 'vnet' are
+considered as auto-generated and are blanked out/regenerated each
+time the interface is attached.
+
+B<--mac> specifies the MAC address of the network interface; if a MAC
address is not given, a new address will be automatically generated
(and stored in the persistent configuration if "--config" is given on
-the commandline). I<script> is used to specify a path to a custom
-script to be called while attaching to a bridge - this will be called
-instead of the default script not in addition to it; --script is valid
-only for interfaces of type I<bridge> and only for Xen domains.
-I<model> specifies the network device model to be presented to the
-domain. I<inbound> and I<outbound> control the bandwidth of the
-interface. At least one from the I<average>, I<floor> pair must be
-specified. The other two I<peak> and I<burst> are optional, so
+the command line).
+
+B<--script> is used to specify a path to a custom script to be called
+while attaching to a bridge - this will be called instead of the default
+script not in addition to it; --script is valid only for interfaces of
+type I<bridge> and only for Xen domains.
+
+B<--model> specifies the network device model to be presented to the
+domain.
+
+B<--inbound> and B<--outbound> control the bandwidth of the
+interface. At least one from the I<average>, I<floor> pair must be
+specified. The other two I<peak> and I<burst> are optional, so
"average,peak", "average,,burst", "average,,,floor",
"average" and
-",,,floor" are also legal. Values for I<average>, I<floor> and
I<peak>
+",,,floor" are also legal. Values for I<average>, I<floor> and
I<peak>
are expressed in kilobytes per second, while I<burst> is expressed in
kilobytes in a single burst at I<peak> speed as described in the
Network XML documentation at
L<http://libvirt.org/formatnetwork.html#elementQoS>;.
-If I<--print-xml> is specified, then the XML of the interface that would be
+B<--managed> is usable only for I<hostdev> type and tells libvirt
+that the interface should be managed, which means detached and reattached
+from/to the guest by libvirt.
+
+B<--driver> is usable only for I<hostdev> type. Specifies which driver
should
+be used, it could be one of I<kvm> (default) and I<vfio>.
+
+If B<--print-xml> is specified, then the XML of the interface that would be
attached is printed instead.
-If I<--live> is specified, affect a running domain.
-If I<--config> is specified, affect the next startup of a persistent domain.
-If I<--current> is specified, affect the current domain state.
-Both I<--live> and I<--config> flags may be given, but I<--current> is
-exclusive. When no flag is specified legacy API is used whose behavior depends
-on the hypervisor driver.
+If B<--live> is specified, affect a running domain.
+If B<--config> is specified, affect the next startup of a persistent domain.
+If B<--current> is specified, affect the current domain state.
+Both B<--live> and B<--config> flags may be given, but B<--current> is
+exclusive. When no flag is specified legacy API is used whose behavior
+depends on the hypervisor driver.
-For compatibility purposes, I<--persistent> behaves like I<--config> for
-an offline domain, and like I<--live> I<--config> for a running domain.
+For compatibility purposes, B<--persistent> behaves like B<--config> for
+an offline domain, and like B<--live> B<--config> for a running domain.
B<Note>: the optional target value is the name of a device to be created
-as the back-end on the node. If not provided a device named "vnetN" or
"vifN"
+as the back-end on the node. If not provided a device named "vnetN" or
"vifN"
will be created automatically.
=item B<detach-device> I<domain> I<FILE>
--
2.6.2
3314
days inactive
3320
days old
13 comments
3 participants
participants (3)
-
Laine Stump -
Pavel Hrdina -
Peter Krempa