-
Notifications
You must be signed in to change notification settings - Fork 482
Extending the command line
This page will walk through an example of patching virt-install and virt-xml to support a new libvirt domain XML value.
A bit of background: libvirt VM configuration is in XML format. It has quite an extensive XML schema. For QEMU/KVM guests, most of the XML attributes map to qemu command line values. QEMU is always adding new emulated hardware and new features, which in turn require the XML schema to be extended. For example, this libvirt commit allows disabling SPICE graphics drag and drop support by adding <filetransfer enable='no/> to <domain><devices><graphics>.
For this example patch, we are going to expose a similar property: <graphics><zlib compression='auto|never|always/>
The details of what this option does is actually not important for this exercise; when extending virt-xml/virt-install we actually don't need to care much about how the underlying feature works. The job of these tools is largely just to build and process the XML correctly. It is the job of libvirt and qemu to make sure the feature actually works.
You can see the complete upstream commit at: https://github.com/virt-manager/virt-manager/commit/6394ab7f9a7c539d80933d561355df6d47426041
Glance through CONTRIBUTING.md first, and then run the test suite with:
pytestIf you aren't running with latest libvirt, you may see test skips here or even failures. It is helpful to report test suite failures to the mailing list, but it's not required. For the sake of your patch, the main thing to check is that your changes do not cause any additional or different failures.
Starting with virt-manager git, first we extend the internal API to map a python class property to its associated XML value.
The virtinst/ directory contains the internal XML building API used by all the tools shipped with virt-manager. There's generally a single file and class per XML object, for example
- virtinst/devices/graphics.py:
<domain><devices><graphics> - virtinst/devices/disk.py:
<domain><devices><disk> - virtinst/domain/cpu.py:
<domain><cpu> - virtinst/domain/os.py:
<domain><os> - ...
If you aren't sure what file or class you need to alter, try grepping for a property you know that virt-install already supports. So, for example, using virt-install --graphics=? I see that there's a property named passwdValidTo. Doing git grep passwdValidTo will point to virtinst/device/graphics.py. Command line names don't always map to internal names but it's a starting point.
XMLProperty is some custom glue that maps a python class property to an XML value, for both reading and writing. The value passed to XMLProperty is an XML xpath string, which is used to identify a location in an XML document. For our needs the main cases you need to know are:
- To set X, use XMLProperty("./foo/bar")
- To set , use XMLProperty("./foo/@bar")
The latter case is the pattern we want, so we add zlip_compression = XMLProperty("./zlib/@compression"). The full change is:
diff --git a/virtinst/devices/graphics.py b/virtinst/devices/graphics.py
index c0a28dd6..7b3c1e6b 100644
--- a/virtinst/devices/graphics.py
+++ b/virtinst/devices/graphics.py
@@ -193,6 +193,7 @@ class DeviceGraphics(Device):
filetransfer_enable = XMLProperty("./filetransfer/@enable", is_yesno=True)
gl = XMLProperty("./gl/@enable", is_yesno=True)
rendernode = XMLProperty("./gl/@rendernode")
+ zlib_compression = XMLProperty("./zlib/@compression")
################## Notice that this patch doesn't do much else, like validate that the value passed in is actually valid. The general rule is to leave this up to libvirt/qemu to complain. We should only add extra validation if it's very important to prevent users from shooting themselves in the foot.
The next step is to set up command line handling. In this case we are adding a sub option to the --graphics command.
Check out the output of ./virt-xml --graphics=help to see the available sub options. Open up virtinst/cli.py and search for '--graphics', you'll find a comment with a ParserGraphics class defined after it. That's where we plug in new sub options.
The cls.add_arg registers the sub option: first argument is the name we want to expose on the cli (zlib.compression), second argument is the python class property name we defined above (zlib_compression). The cliname should mirror the XML, with segments separated by dot.
Some options need to do special handling. Look at the surrounding code for examples if you need inspiration.
Our patch looks like:
diff --git a/virtinst/cli.py b/virtinst/cli.py
index 3fa32e31..eef85f49 100644
--- a/virtinst/cli.py
+++ b/virtinst/cli.py
@@ -3205,6 +3205,7 @@ class ParserGraphics(VirtCLIParser):
cls.add_arg("mouse.mode", "mouse_mode")
cls.add_arg("filetransfer.enable", "filetransfer_enable",
is_onoff=True)
+ cls.add_arg("zlib.compression", "zlib_compression")
cls.add_arg("gl.enable", "gl", is_onoff=True)
cls.add_arg("gl.rendernode", "rendernode")After this bit is applied, you'll see zlib.compression appear in the virt-xml --graphics=help output.
A simple way which often works to test the sub option is with virt-xml --build-xml:
./virt-xml --connect test:///default --build-xml --graphics zlib.compression=FOO
<graphics type="spice" port="-1" tlsPort="-1" autoport="yes">
<zlib compression="foo"/>
<image compression="off"/>
</graphics>If you run the test suite right now, you may see a new failure. This is because the test suite has some special tracking to ensure that every command line option is tested. You just added a new option and it isn't tested yet, so you will see failures (if you started with a fully passing test suite). We will fix that now.
Open tests/test_cli.py, and search for --graphics. You'll find multiple command line usages of --graphics being tested. Find the one with the most --graphics instances (typically the test called test-many-devices), pick one of the test lines and tack on your addition. For this to be accepted by libvirt you will need to know a valid value that is the XML property. Looking at the libvirt formatdomain graphics docs, I see that zlib compression accepts 'auto', 'always', and 'never'. So I picked one. Note, this diff is against older code so the test file name is different. The current name is tests/test_cli.py
diff --git a/tests/clitest.py b/tests/clitest.py
index 8fd7b752..775d72fb 100644
--- a/tests/clitest.py
+++ b/tests/clitest.py
@@ -551,7 +551,7 @@ source.reservations.managed=no,source.reservations.source.type=unix,source.reser
--graphics spice,keymap=none
--graphics vnc,port=5950,listen=1.2.3.4,keymap=ja,password=foo
--graphics spice,port=5950,tlsport=5950,listen=1.2.3.4,keymap=ja
---graphics spice,image_compression=glz,streaming_mode=filter,clipboard_copypaste=yes,mouse_mode=client,filetransfer_enable=on
+--graphics spice,image_compression=glz,streaming_mode=filter,clipboard_copypaste=yes,mouse_mode=client,filetransfer_enable=on,zlib.compression=always
--graphics spice,gl=yes,listen=socket,image.compression=glz,streaming.mode=filter,clipboard.copypaste=yes,mouse.mode=client,filetransfer.enable=on,tlsPort=6000,passwd=testpass,passwdValidTo=2010-04-09T15:51:00,passwordValidTo=2010-04-09T15:51:01,defaultMode=insecure
--graphics spice,gl=yes,listen=none
--graphics spice,gl.enable=yes,listen=none,rendernode=/dev/dri/foo,gl.rendernode=/dev/dri/foo2After that, if you rerun the test suite, you are likely to see a different error: this is because the output from the tested command has now changed. You can tell the test suite to regenerate output with:
pytest --regenerate-outputAfter this, I see I new chunk in git diff output, and subsequent test runs are back to normal.
This diff shows the old path, on new code the file name will be slightly different.
diff --git a/tests/cli-test-xml/compare/virt-install-many-devices.xml b/tests/cli-test-xml/compare/v
irt-install-many-devices.xml
index bb316c82..46f1f34f 100644
--- a/tests/cli-test-xml/compare/virt-install-many-devices.xml
+++ b/tests/cli-test-xml/compare/virt-install-many-devices.xml
@@ -369,6 +369,7 @@
<clipboard copypaste="yes"/>
<mouse mode="client"/>
<filetransfer enable="yes"/>
+ <zlib compression="always"/>
</graphics>
<graphics type="spice" tlsPort="6000" passwd="testpass" passwdValidTo="2010-04-09T15:51:01" defaultMode="insecure">
<gl enable="yes" rendernode="/dev/dri/by-path/pci-0000:00:02.0-render"/>If you see more changed output generated in tests/, it's probably a mistake due to running on an older libvirt version. Remove those pieces from your patch before submitting. Ask the devs if you have any questions.
For each new option sub property, the general rule is we don't need to explicitly list it in the man page or virt-install/virt-xml --help output. The idea is that command line introspection and libvirt XML documentation should be sufficient and we don't want our man page to just duplicate libvirt documentation. However, if your command line option has some special behavior, or is particularly important, consider extending man/virt-install.pod. ./setup.py build will then generate man/virt-install.1, which can be inspected with man ./man/virt-install.1
Model your commit message after the example commit listed at the start. See CONTRIBUTING.md for more details about submitting patches.
Not all command line extensions are simple. Here's some less obvious examples:
-
Adding a multi option which can take optional [0-9]* regex:
-
Converting old single options to multioption style