Re: [PATCH libvirt master v2] interface type: add udp socket support

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

 



On Sat, Aug 29, 2015 at 04:19:10PM -0400, Jonathan Toppins wrote:
> Adds a new interface type using UDP sockets, this seems only applicable
> to QEMU but have edited tree-wide to support the new interface type.
> 
> The interface type required the addition of a "localaddr" (local
> address), this then maps into the following xml and qemu call.
> 
> <interface type='udp'>
> 	<mac address='52:54:00:5c:67:56'/>
> 	<source address='127.0.0.1' port='11112' >
> 		<local address='127.0.0.1' port='22222'/>
> 	</source>
> 	<model type='virtio'/>
> 	<address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/>
> 	...
> 
> QEMU call:
> 	-net socket,udp=127.0.0.1:11112,localaddr=127.0.0.1:22222
> 
> Notice the xml "local" entry becomes the "localaddr" for the qemu call.
> 
> reference:
> http://lists.gnu.org/archive/html/qemu-devel/2011-11/msg00629.html
> 
> Signed-off-by: Jonathan Toppins <jtoppins@xxxxxxxxxxxxxxxxxxx>
> ---
> 
> since v1:
>  I expect there will be one more round, thanks for the comments thus far. Wanted
>  to go a head and send this out since it has been a little to long to get to
>  this point. Some final issues I am seeing:
>  * there seems to be some trouble with adding a new UDP type interface to a
>    running VM. Stanley who is CC'ed and helping me test has more details.
>  * unittests pass even though qemuxml2argvtest still fails, this appears to be
>    due to disk-drive-network-gluster failing - analysis looks to be the URI is
>    incorrect, not enough slashes - cuz more is better ;)

Works fine for me, what is your libxml version?
This could be related to
http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=8f17d0e

commit 8f17d0eaae7ee2fa3e214b79b188fc14ed5aa1eb
    util: Prepare URI formatting for libxml2 >= 2.9.2

>  * please verify I have added the schema correctly, was kinda confusing
> 
>  Code Comments:
>  [Laine Stump]
>  * [DONE] change to using "local" as a nested element inside the "source" element
>  * [DONE] enhance schema to validate the new formatting of the source and local elements
>  [Ján Tomko]
>  * [DONE] implement unit tests in tests/qemuxml2argvtest.c and tests/qemuxml2xmltest.c
>  * [DONE] increase verbosity and note when the feature was added in formatdomain.html.in
> 
>  docs/formatdomain.html.in                        | 24 ++++++++
>  docs/schemas/domaincommon.rng                    | 27 +++++++++
>  src/conf/domain_conf.c                           | 74 ++++++++++++++++++++++--
>  src/conf/domain_conf.h                           |  3 +
>  src/conf/netdev_bandwidth_conf.h                 |  1 +
>  src/libxl/libxl_conf.c                           |  1 +
>  src/lxc/lxc_controller.c                         |  1 +
>  src/lxc/lxc_process.c                            |  1 +
>  src/qemu/qemu_command.c                          | 12 ++++
>  src/qemu/qemu_hotplug.c                          |  1 +
>  src/qemu/qemu_interface.c                        |  2 +
>  src/uml/uml_conf.c                               |  5 ++
>  src/xenconfig/xen_sxpr.c                         |  1 +
>  tests/qemuxml2argvdata/qemuxml2argv-net-udp.args |  6 ++
>  tests/qemuxml2argvdata/qemuxml2argv-net-udp.xml  | 34 +++++++++++
>  tests/qemuxml2argvtest.c                         |  1 +
>  tests/qemuxml2xmltest.c                          |  1 +
>  tools/virsh-domain.c                             |  1 +
>  18 files changed, 190 insertions(+), 6 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-udp.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-udp.xml
> 

ACK

I have fixed the nits mentioned below and pushed the patch.

Jan

> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 5ca8ede..d307d4d 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -4260,6 +4260,30 @@
>    &lt;/devices&gt;
>    ...</pre>
>  
> +    <h5><a name="elementsNICSUDP">UDP unicast tunnel</a></h5>
> +
> +    <p>
> +    A UDP unicast architecture provides a virtual network which enables
> +    connections between Qemu instances using Qemu's UDP infrastructure.
> +
> +    The xml "source" address is the endpoint address to which the UDP socket
> +    packets will be sent from the host running QEMU.
> +    The xml "local" address is the address of the interface from which the
> +    UDP socket packets will originate from the qemu host.
> +    <span class="since">Since 1.2.19</span></p>

That will be 1.2.20 now. I also unified all the spellings of QEMU in the
paragraph.

> +
> +<pre>
> +  ...
> +  &lt;devices&gt;
> +    &lt;interface type='udp'&gt;
> +      &lt;mac address='52:54:00:22:c9:42'/&gt;
> +      &lt;source address='127.0.0.1' port='11115'&gt;
> +        &lt;local address='127.0.0.1' port='11116'/&gt;
> +      &lt;/source&gt;
> +    &lt;/interface&gt;
> +  &lt;/devices&gt;
> +  ...</pre>
> +
>      <h5><a name="elementsNICSModel">Setting the NIC model</a></h5>
>  
>  <pre>

> @@ -8701,10 +8706,18 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>              } else if (!address &&
>                         (def->type == VIR_DOMAIN_NET_TYPE_SERVER ||
>                          def->type == VIR_DOMAIN_NET_TYPE_CLIENT ||
> -                        def->type == VIR_DOMAIN_NET_TYPE_MCAST) &&
> +                        def->type == VIR_DOMAIN_NET_TYPE_MCAST ||
> +                        def->type == VIR_DOMAIN_NET_TYPE_UDP) &&
>                         xmlStrEqual(cur->name, BAD_CAST "source")) {
>                  address = virXMLPropString(cur, "address");
>                  port = virXMLPropString(cur, "port");
> +                if (!localaddr && def->type == VIR_DOMAIN_NET_TYPE_UDP) {
> +                    xmlNodePtr tmpnode = ctxt->node;
> +                    ctxt->node = cur;
> +                    localaddr = virXPathString("string(./local/@address)", ctxt);
> +                    localport = virXPathString("string(./local/@port)", ctxt);

virXPathString strdups the string, they need to be VIR_FREE'd in the
cleanup section.

> +                    ctxt->node = tmpnode;
> +                }
>              } else if (xmlStrEqual(cur->name, BAD_CAST "ip")) {
>                  virDomainNetIpDefPtr ip = NULL;
>  

> @@ -19959,13 +20000,34 @@ virDomainNetDefFormat(virBufferPtr buf,
>          case VIR_DOMAIN_NET_TYPE_SERVER:
>          case VIR_DOMAIN_NET_TYPE_CLIENT:
>          case VIR_DOMAIN_NET_TYPE_MCAST:
> +        case VIR_DOMAIN_NET_TYPE_UDP:
>              if (def->data.socket.address) {
> -                virBufferAsprintf(buf, "<source address='%s' port='%d'/>\n",
> -                                  def->data.socket.address, def->data.socket.port);
> +                virBufferAsprintf(buf, "<source address='%s' port='%d'",
> +                                  def->data.socket.address,
> +                                  def->data.socket.port);
>              } else {
> -                virBufferAsprintf(buf, "<source port='%d'/>\n",
> +                virBufferAsprintf(buf, "<source port='%d'",
>                                    def->data.socket.port);
>              }
> +
> +            if (def->type != VIR_DOMAIN_NET_TYPE_UDP) {
> +                virBufferAddLit(buf, "/>\n");
> +                break;
> +            }
> +
> +            virBufferAddLit(buf, ">\n");
> +            virBufferAdjustIndent(buf, 2);
> +
> +            if (def->data.socket.localaddr) {

We don't allow parsing a UDP interface without an address, this
condition can be eliminated.

> +                virBufferAsprintf(buf, "<local address='%s' port='%d'/>\n",
> +                                  def->data.socket.localaddr,
> +                                  def->data.socket.localport);
> +            } else {
> +                virBufferAsprintf(buf, "<local port='%d'/>",
> +                                  def->data.socket.localport);
> +            }
> +            virBufferAdjustIndent(buf, -2);
> +            virBufferAddLit(buf, "</source>\n");
>              break;
>  
>          case VIR_DOMAIN_NET_TYPE_INTERNAL:

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index abc57d7..1210fb2 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -5594,6 +5594,17 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
>         type_sep = ',';
>         break;
>  
> +    case VIR_DOMAIN_NET_TYPE_UDP:
> +       virBufferAsprintf(&buf, "socket%cudp=%s:%d%clocaladdr=%s:%d",
> +                         type_sep,
> +                         net->data.socket.address,
> +                         net->data.socket.port,
> +                         type_sep,

type_sep is only used for the first separator, the second one should be
a comma. But practically, type_sep will always be a comma - it is only
set to ' ' when hotplugging a device with qemu old enough not to support
-netdev.

> +                         net->data.socket.localaddr,
> +                         net->data.socket.localport);
> +       type_sep = ',';
> +       break;
> +
>      case VIR_DOMAIN_NET_TYPE_USER:
>      default:
>          virBufferAddLit(&buf, "user");

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]