[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 2/4] conf: add <acpi table> to <os>




On 05/13/2016 10:52 AM, Ján Tomko wrote:
> Add a new element to <domain> XML:
> <os>
>   <acpi table="slic">/path/to/acpi/table/file</acpi>
> </os>
> 
> To supply a path to a SLIC (Software Licensing) ACPI
> table blob.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1327537
> ---
>  docs/formatdomain.html.in                          |  6 ++++
>  docs/schemas/domaincommon.rng                      | 10 +++++++
>  src/conf/domain_conf.c                             | 13 +++++++++
>  src/conf/domain_conf.h                             |  1 +
>  tests/qemuxml2argvdata/qemuxml2argv-acpi-table.xml | 28 +++++++++++++++++++
>  .../qemuxml2xmlout-acpi-table.xml                  | 32 ++++++++++++++++++++++
>  tests/qemuxml2xmltest.c                            |  3 ++
>  7 files changed, 93 insertions(+)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-acpi-table.xml
>  create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-acpi-table.xml
> 

Yet another example of a path that "could" need comma escaping:

http://wiki.libvirt.org/page/BiteSizedTasks#qemu:_Use_comma_escaping_for_more_command_line_values


> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 58b8cb6..a294978 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -277,6 +277,7 @@
>      &lt;initrd&gt;/root/f8-i386-initrd&lt;/initrd&gt;
>      &lt;cmdline&gt;console=ttyS0 ks=http://example.com/f8-i386/os/&lt;/cmdline&gt;
>      &lt;dtb&gt;/root/ppc.dtb&lt;/dtb&gt;
> +    &lt;acpi table="slic"&gt;/path/to/slic.dat&lt;/&gt;

Since you format as table='slic', this should use same format
e.g. s/"slic"/'slic'/

>    &lt;/os&gt;
>    ...</pre>
>  
> @@ -302,6 +303,11 @@
>        <dd>The contents of this element specify the fully-qualified path
>          to the (optional) device tree binary (dtb) image in the host OS.
>          <span class="since">Since 1.0.4</span></dd>
> +      <dt><code>acpi</code></dt>
> +      <dd>The contents of the <code>table</code> attribute specifies the
> +        ACPI table type (Currently only <code>slic</code> is supported)

s/Currently/currently/

> +        and the element contains a fully-qualified path to the ACPI table.
> +        <span class="since">Since 1.3.5</span></dd>

[1] Since this is only supported for 'hvm', that should be noted as well

>      </dl>
>  
>      <h4><a name="elementsOSContainer">Container boot</a></h4>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 8798001..8c4be11 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -306,6 +306,16 @@
>          <optional>
>            <ref name="bios"/>
>          </optional>
> +        <optional>
> +          <element name="acpi">
> +            <attribute name="table">
> +              <choice>
> +                <value>slic</value>
> +              </choice>
> +            </attribute>
> +            <ref name="absFilePath"/>
> +          </element>
> +        </optional>

Could be it's own define like 'bios'... 'smbios'...  

Not a requirement though.

>        </interleave>
>      </element>
>    </define>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 331ff06..6f86e46 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2619,6 +2619,7 @@ void virDomainDefFree(virDomainDefPtr def)
>      VIR_FREE(def->os.cmdline);
>      VIR_FREE(def->os.dtb);
>      VIR_FREE(def->os.root);
> +    VIR_FREE(def->os.slic_table);
>      virDomainLoaderDefFree(def->os.loader);
>      VIR_FREE(def->os.bootloader);
>      VIR_FREE(def->os.bootloaderArgs);
> @@ -15102,6 +15103,7 @@ virDomainDefParseBootOptions(virDomainDefPtr def,
>                               virHashTablePtr *bootHash)
>  {
>      xmlNodePtr *nodes = NULL;
> +    char *tmp = NULL;
>      int ret = -1;
>      size_t i;
>      int n;
> @@ -15162,6 +15164,14 @@ virDomainDefParseBootOptions(virDomainDefPtr def,
>      }
>  
>      if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {

[1] ^^^

> +        tmp = virXPathString("string(./os/acpi[1]/@table)", ctxt);
> +        if (STREQ_NULLABLE(tmp, "slic")) {
> +            VIR_FREE(tmp);
> +            tmp = virXPathString("string(./os/acpi[1])", ctxt);
> +            def->os.slic_table = virFileSanitizePath(tmp);
> +            VIR_FREE(tmp);
> +        }
> +
>          if (virDomainDefParseBootXML(ctxt, def) < 0)
>              goto error;
>          if (!(*bootHash = virHashCreate(5, NULL)))
> @@ -15172,6 +15182,7 @@ virDomainDefParseBootOptions(virDomainDefPtr def,
>  
>   error:
>      VIR_FREE(nodes);
> +    VIR_FREE(tmp);
>      return ret;
>  }
>  
> @@ -22494,6 +22505,8 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>                            def->os.dtb);
>      virBufferEscapeString(buf, "<root>%s</root>\n",
>                            def->os.root);
> +    virBufferEscapeString(buf, "<acpi table='slic'>%s</acpi>\n",
> +                          def->os.slic_table);
>  
>      if (!def->os.bootloader) {
>          for (n = 0; n < def->os.nBootDevs; n++) {
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index b9e696d..0b3f1a2 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1875,6 +1875,7 @@ struct _virDomainOSDef {
>      char *cmdline;
>      char *dtb;
>      char *root;
> +    char *slic_table;

This is a path right? Why not just slic_path to make it obvious or
could just be slic.

IDC whichever way it goes. Just wouldn't want someone to expect to
find a parsed table in there only to realize it's a path... 

Your call on this one.

ACK with doc adjustments... I also trust that you can make the code
adjustments properly...


John
>      virDomainLoaderDefPtr loader;
>      char *bootloader;
>      char *bootloaderArgs;
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-acpi-table.xml b/tests/qemuxml2argvdata/qemuxml2argv-acpi-table.xml
> new file mode 100644
> index 0000000..f807791
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-acpi-table.xml
> @@ -0,0 +1,28 @@
> +<domain type='qemu'>
> +  <name>QEMUGuest1</name>
> +  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> +  <memory unit='KiB'>219136</memory>
> +  <currentMemory unit='KiB'>219136</currentMemory>
> +  <vcpu placement='static'>1</vcpu>
> +  <os>
> +    <type arch='x86_64' machine='pc'>hvm</type>
> +    <boot dev='hd'/>
> +    <acpi table='slic'>/var/lib/libvirt/acpi/slic.dat</acpi>
> +  </os>
> +  <features>
> +    <acpi/>
> +  </features>
> +  <clock offset='utc'/>
> +  <on_poweroff>destroy</on_poweroff>
> +  <on_reboot>restart</on_reboot>
> +  <on_crash>destroy</on_crash>
> +  <devices>
> +    <emulator>/usr/bin/qemu</emulator>
> +    <controller type='usb' index='0'/>
> +    <controller type='ide' index='0'/>
> +    <controller type='pci' index='0' model='pci-root'/>
> +    <input type='mouse' bus='ps2'/>
> +    <input type='keyboard' bus='ps2'/>
> +    <memballoon model='none'/>
> +  </devices>
> +</domain>
> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-acpi-table.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-acpi-table.xml
> new file mode 100644
> index 0000000..54a4d90
> --- /dev/null
> +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-acpi-table.xml
> @@ -0,0 +1,32 @@
> +<domain type='qemu'>
> +  <name>QEMUGuest1</name>
> +  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> +  <memory unit='KiB'>219136</memory>
> +  <currentMemory unit='KiB'>219136</currentMemory>
> +  <vcpu placement='static'>1</vcpu>
> +  <os>
> +    <type arch='x86_64' machine='pc'>hvm</type>
> +    <acpi table='slic'>/var/lib/libvirt/acpi/slic.dat</acpi>
> +    <boot dev='hd'/>
> +  </os>
> +  <features>
> +    <acpi/>
> +  </features>
> +  <clock offset='utc'/>
> +  <on_poweroff>destroy</on_poweroff>
> +  <on_reboot>restart</on_reboot>
> +  <on_crash>destroy</on_crash>
> +  <devices>
> +    <emulator>/usr/bin/qemu</emulator>
> +    <controller type='usb' index='0'>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
> +    </controller>
> +    <controller type='ide' index='0'>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
> +    </controller>
> +    <controller type='pci' index='0' model='pci-root'/>
> +    <input type='mouse' bus='ps2'/>
> +    <input type='keyboard' bus='ps2'/>
> +    <memballoon model='none'/>
> +  </devices>
> +</domain>
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index 5a43fa9..0c9fd7e 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -794,6 +794,9 @@ mymain(void)
>      cfg->vncAutoUnixSocket = false;
>  
>      virObjectUnref(cfg);
> +
> +    DO_TEST("acpi-table");
> +
>      qemuTestDriverFree(&driver);
>  
>      return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]