3:11 a.m.
This patch series resolves the following bug:
https://bugzilla.redhat.com/show_bug.cgi?id=703851
In short, it implements support for a new "listenNetwork" attribute in
the domain's <graphics> element. When listenNetwork is specified,
libvirt will look for a network of that name (from the networks that
can be created within libvirt), and derive a listen address from the
physical interface used to connect that network to the outside
world. (More details are given in each commit message).
This is V2. The main differences from V1 are:
1) everything was rebased
2) I eliminated #if WITH_NETWORK in the .c files by defining the one
missing function as static inline when WITH_NETWORK is false.
This series is meant to be applied on top of the "network physical
device abstraction" patch series I sent a couple hours ago:
https://www.redhat.com/archives/libvir-list/2011-July/msg01294.html
Don't attempt to apply it by itself!
3:11 a.m.
New subject: [libvirt] [PATCHv2 1/4] util: add an ifaceGetIPAddress to the interface utilies
This function uses ioctl(SIOCGIFADDR), which limits it to returning
the first IPv4 address of an interface, but that's what we want right
now (the place we're going to use the address only accepts one).
---
src/libvirt_private.syms | 1 +
src/util/interface.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++
src/util/interface.h | 3 ++
3 files changed, 64 insertions(+), 0 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 1b6d1b0..ff34456 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -526,6 +526,7 @@ ifaceCtrl;
ifaceGetFlags;
ifaceGetIndex;
ifaceGetMacAddress;
+ifaceGetIPAddress;
ifaceGetNthParent;
ifaceGetVlanID;
ifaceIsUp;
diff --git a/src/util/interface.c b/src/util/interface.c
index 837ecce..a714bad 100644
--- a/src/util/interface.c
+++ b/src/util/interface.c
@@ -29,6 +29,7 @@
#include <sys/socket.h>
#include <sys/ioctl.h>
#include <fcntl.h>
+#include <netinet/in.h>
#ifdef __linux__
# include <linux/if.h>
@@ -511,6 +512,65 @@ ifaceSetMacAddress(const char *ifname ATTRIBUTE_UNUSED,
/**
+ * ifaceGetIPAddress:
+ * @ifname: name of the interface whose IP address we want
+ * @macaddr: MAC address (VIR_MAC_BUFLEN in size)
+ *
+ * This function gets the @macaddr for a given interface @ifname.
+ *
+ * Returns 0 in case of success or an errno code in case of failure.
+ */
+#ifdef __linux__
+int
+ifaceGetIPAddress(const char *ifname,
+ virSocketAddrPtr addr)
+{
+ struct ifreq ifr;
+ int fd;
+ int rc = 0;
+
+ if (!ifname || !addr)
+ return EINVAL;
+
+ memset (addr, 0, sizeof(*addr));
+ addr->data.stor.ss_family = AF_UNSPEC;
+
+ fd = socket(AF_INET, SOCK_STREAM, 0);
+ if (fd < 0)
+ return errno;
+
+ memset(&ifr, 0, sizeof(struct ifreq));
+ if (virStrcpyStatic(ifr.ifr_name, ifname) == NULL) {
+ rc = EINVAL;
+ goto err_exit;
+ }
+
+ if (ioctl(fd, SIOCGIFADDR, (char *)&ifr) != 0) {
+ rc = errno;
+ goto err_exit;
+ }
+
+ addr->data.stor.ss_family = AF_INET;
+ addr->len = sizeof(addr->data.inet4);
+ memcpy(&addr->data.inet4, &ifr.ifr_addr, addr->len);
+
+err_exit:
+ VIR_FORCE_CLOSE(fd);
+ return rc;
+}
+
+#else
+
+int
+ifaceGetIPAddress(const char *ifname ATTRIBUTE_UNUSED,
+ virSocketAddrPtr addr ATTRIBUTE_UNUSED)
+{
+ return ENOSYS;
+}
+
+#endif /* __linux__ */
+
+/**
* ifaceLinkAdd
*
* @type: The type of device, i.e., "macvtap"
diff --git a/src/util/interface.h b/src/util/interface.h
index 5cbf5c1..9647653 100644
--- a/src/util/interface.h
+++ b/src/util/interface.h
@@ -24,6 +24,7 @@ struct nlattr;
# endif
# include "datatypes.h"
+# include "network.h"
int ifaceGetFlags(const char *name, short *flags);
int ifaceIsUp(const char *name, bool *up);
@@ -49,6 +50,8 @@ int ifaceSetMacAddress(const char *ifname, const unsigned char
*macaddr);
int ifaceGetMacAddress(const char *ifname, unsigned char *macaddr);
+int ifaceGetIPAddress(const char *ifname, virSocketAddrPtr addr);
+
int ifaceMacvtapLinkAdd(const char *type,
const unsigned char *macaddress, int macaddrsize,
const char *ifname,
--
1.7.3.4
6:16 p.m.
New subject: [libvirt] [PATCHv2 1/4] util: add an ifaceGetIPAddress to the interface utilies
On 07/20/2011 02:11 AM, Laine Stump wrote:
In the subject:
s/utilies/utilities/
This function uses ioctl(SIOCGIFADDR), which limits it to returning
the first IPv4 address of an interface, but that's what we want right
now (the place we're going to use the address only accepts one).
---
src/libvirt_private.syms | 1 +
src/util/interface.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++
src/util/interface.h | 3 ++
3 files changed, 64 insertions(+), 0 deletions(-)
@@ -511,6 +512,65 @@ ifaceSetMacAddress(const char *ifname ATTRIBUTE_UNUSED,
/**
+ * ifaceGetIPAddress:
+ * @ifname: name of the interface whose IP address we want
+ * @macaddr: MAC address (VIR_MAC_BUFLEN in size)
+ *
+ * This function gets the @macaddr for a given interface @ifname.
+ *
+ * Returns 0 in case of success or an errno code in case of failure.
Positive on error? Should we instead be returing -errno, so callers can
check < 0, per our typical usage?
Conditional ACK, depending on the answer to that question (the code
looks correct, and converting to negative return on failure seems
trivial enough if we decide to go that way).
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
6:52 p.m.
New subject: [libvirt] [PATCHv2 1/4] util: add an ifaceGetIPAddress to the interface utilies
On 07/21/2011 07:16 PM, Eric Blake wrote:
On 07/20/2011 02:11 AM, Laine Stump wrote:
In the subject:
s/utilies/utilities/
> This function uses ioctl(SIOCGIFADDR), which limits it to returning
> the first IPv4 address of an interface, but that's what we want right
> now (the place we're going to use the address only accepts one).
> ---
> src/libvirt_private.syms | 1 +
> src/util/interface.c | 60
> ++++++++++++++++++++++++++++++++++++++++++++++
> src/util/interface.h | 3 ++
> 3 files changed, 64 insertions(+), 0 deletions(-)
>
> @@ -511,6 +512,65 @@ ifaceSetMacAddress(const char *ifname
> ATTRIBUTE_UNUSED,
>
>
> /**
> + * ifaceGetIPAddress:
> + * @ifname: name of the interface whose IP address we want
> + * @macaddr: MAC address (VIR_MAC_BUFLEN in size)
> + *
> + * This function gets the @macaddr for a given interface @ifname.
> + *
> + * Returns 0 in case of success or an errno code in case of failure.
Positive on error? Should we instead be returing -errno, so callers
can check < 0, per our typical usage?
Conditional ACK, depending on the answer to that question (the code
looks correct, and converting to negative return on failure seems
trivial enough if we decide to go that way).
On one hand I would like to be consistent with most of the rest of
libvirt in returning < 0 on error. On the other hand, most of the other
functions in this file return errno on error.
Maybe the proper thing to do is first post a patch that changes the
existing functions to return -errno, then my patches.
Yeah, I'll do that.
11:03 p.m.
New subject: [libvirt] [PATCH] util: make interface.c functions consistently return < 0 on error
(This patch must be pushed before the "listenNetwork" patches can be
pushed)
All of the functions in util/interface.c were returning 0 on success,
but some returned -1 on error, and some returned a positive value
(usually the value of errno, but sometimes just 1). Libvirt's standard
is to return < 0 on error (in the case of functions that need to
return errno, -errno is returned.
This patch modifies all functions in interface.c to consistently
return < 0 on error, and makes changes to callers of those functions
where necessary.
---
src/nwfilter/nwfilter_gentech_driver.c | 8 +-
src/nwfilter/nwfilter_learnipaddr.c | 4 +-
src/util/interface.c | 123 ++++++++++++++++----------------
src/util/macvtap.c | 10 ++--
4 files changed, 73 insertions(+), 72 deletions(-)
diff --git a/src/nwfilter/nwfilter_gentech_driver.c
b/src/nwfilter/nwfilter_gentech_driver.c
index bf44fc4..7d9871a 100644
--- a/src/nwfilter/nwfilter_gentech_driver.c
+++ b/src/nwfilter/nwfilter_gentech_driver.c
@@ -715,7 +715,7 @@ virNWFilterInstantiate(virConnectPtr conn,
if (teardownOld && rc == 0)
techdriver->tearOldRules(conn, ifname);
- if (rc == 0 && ifaceCheck(false, ifname, NULL, ifindex)) {
+ if (rc == 0 && (ifaceCheck(false, ifname, NULL, ifindex) < 0)) {
/* interface changed/disppeared */
techdriver->allTeardown(ifname);
rc = 1;
@@ -898,7 +898,7 @@ _virNWFilterInstantiateFilter(virConnectPtr conn,
int ifindex;
int rc;
- if (ifaceGetIndex(true, net->ifname, &ifindex))
+ if (ifaceGetIndex(true, net->ifname, &ifindex) < 0)
return 1;
virNWFilterLockFilterUpdates();
@@ -954,8 +954,8 @@ virNWFilterInstantiateFilterLate(virConnectPtr conn,
&foundNewFilter);
if (rc) {
/* something went wrong... 'DOWN' the interface */
- if (ifaceCheck(false, ifname, NULL, ifindex) != 0 ||
- ifaceDown(ifname)) {
+ if ((ifaceCheck(false, ifname, NULL, ifindex) < 0) ||
+ (ifaceDown(ifname) < 0)) {
/* assuming interface disappeared... */
_virNWFilterTeardownFilter(ifname);
}
diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c
index cf818a2..034eedb 100644
--- a/src/nwfilter/nwfilter_learnipaddr.c
+++ b/src/nwfilter/nwfilter_learnipaddr.c
@@ -431,7 +431,7 @@ learnIPAddressThread(void *arg)
req->status = 0;
/* anything change to the VM's interface -- check at least once */
- if (ifaceCheck(false, req->ifname, NULL, req->ifindex)) {
+ if (ifaceCheck(false, req->ifname, NULL, req->ifindex) < 0) {
req->status = ENODEV;
goto done;
}
@@ -501,7 +501,7 @@ learnIPAddressThread(void *arg)
}
/* check whether VM's dev is still there */
- if (ifaceCheck(false, req->ifname, NULL, req->ifindex)) {
+ if (ifaceCheck(false, req->ifname, NULL, req->ifindex) < 0) {
req->status = ENODEV;
showError = false;
break;
diff --git a/src/util/interface.c b/src/util/interface.c
index 7b1a296..72c7f3d 100644
--- a/src/util/interface.c
+++ b/src/util/interface.c
@@ -59,10 +59,10 @@ getFlags(int fd, const char *ifname, struct ifreq *ifr) {
if (virStrncpy(ifr->ifr_name,
ifname, strlen(ifname), sizeof(ifr->ifr_name)) == NULL)
- return ENODEV;
+ return -ENODEV;
if (ioctl(fd, SIOCGIFFLAGS, ifr) < 0)
- return errno;
+ return -errno;
return 0;
}
@@ -74,7 +74,7 @@ getFlags(int fd, const char *ifname, struct ifreq *ifr) {
* @ifname : name of the interface
* @flags : pointer to short holding the flags on success
*
- * Get the flags of the interface. Returns 0 on success, error code on failure.
+ * Get the flags of the interface. Returns 0 on success, -errno on failure.
*/
int
ifaceGetFlags(const char *ifname, short *flags) {
@@ -83,7 +83,7 @@ ifaceGetFlags(const char *ifname, short *flags) {
int fd = socket(PF_PACKET, SOCK_DGRAM, 0);
if (fd < 0)
- return errno;
+ return -errno;
rc = getFlags(fd, ifname, &ifr);
@@ -100,7 +100,7 @@ ifaceIsUp(const char *ifname, bool *up) {
short flags = 0;
int rc = ifaceGetFlags(ifname, &flags);
- if (rc)
+ if (rc < 0)
return rc;
*up = ((flags & IFF_UP) == IFF_UP);
@@ -112,11 +112,12 @@ ifaceIsUp(const char *ifname, bool *up) {
/* Note: Showstopper on cygwin is only missing PF_PACKET */
int
+
ifaceGetFlags(const char *ifname ATTRIBUTE_UNUSED,
short *flags ATTRIBUTE_UNUSED) {
ifaceError(VIR_ERR_INTERNAL_ERROR, "%s",
_("ifaceGetFlags is not supported on non-linux platforms"));
- return ENOSYS;
+ return -ENOSYS;
}
int
@@ -125,7 +126,7 @@ ifaceIsUp(const char *ifname ATTRIBUTE_UNUSED,
ifaceError(VIR_ERR_INTERNAL_ERROR, "%s",
_("ifaceIsUp is not supported on non-linux platforms"));
- return ENOSYS;
+ return -ENOSYS;
}
#endif /* __linux__ */
@@ -141,7 +142,7 @@ ifaceIsUp(const char *ifname ATTRIBUTE_UNUSED,
* flagmask = (~0 ^ flagclear)
* newflags = (curflags & flagmask) | flagset;
*
- * Returns 0 on success, errno on failure.
+ * Returns 0 on success, -errno on failure.
*/
#ifdef __linux__
static int chgIfaceFlags(const char *ifname, short flagclear, short flagset) {
@@ -152,10 +153,10 @@ static int chgIfaceFlags(const char *ifname, short flagclear, short
flagset) {
int fd = socket(PF_PACKET, SOCK_DGRAM, 0);
if (fd < 0)
- return errno;
+ return -errno;
rc = getFlags(fd, ifname, &ifr);
- if (rc != 0)
+ if (rc < 0)
goto cleanup;
flags = (ifr.ifr_flags & flagmask) | flagset;
@@ -164,7 +165,7 @@ static int chgIfaceFlags(const char *ifname, short flagclear, short
flagset) {
ifr.ifr_flags = flags;
if (ioctl(fd, SIOCSIFFLAGS, &ifr) < 0)
- rc = errno;
+ rc = -errno;
}
cleanup:
@@ -180,7 +181,7 @@ cleanup:
*
* Function to control if an interface is activated (up, 1) or not (down, 0)
*
- * Returns 0 in case of success or an errno code in case of failure.
+ * Returns 0 on success, -errno on failure.
*/
int
ifaceCtrl(const char *name, bool up)
@@ -195,7 +196,7 @@ ifaceCtrl(const char *name, bool up)
int
ifaceCtrl(const char *name ATTRIBUTE_UNUSED, bool up ATTRIBUTE_UNUSED)
{
- return ENOSYS;
+ return -ENOSYS;
}
#endif /* __linux__ */
@@ -212,10 +213,10 @@ ifaceCtrl(const char *name ATTRIBUTE_UNUSED, bool up
ATTRIBUTE_UNUSED)
* it must have the given MAC address and if an interface index is
* passed, it must also match the interface index.
*
- * Returns 0 on success, an error code on failure.
- * ENODEV : if interface with given name does not exist or its interface
- * index is different than the one passed
- * EINVAL : if interface name is invalid (too long)
+ * Returns 0 on success, -errno on failure.
+ * -ENODEV : if interface with given name does not exist or its interface
+ * index is different than the one passed
+ * -EINVAL : if interface name is invalid (too long)
*/
#ifdef __linux__
int
@@ -230,7 +231,7 @@ ifaceCheck(bool reportError, const char *ifname,
if (macaddr != NULL) {
fd = socket(PF_PACKET, SOCK_DGRAM, 0);
if (fd < 0)
- return errno;
+ return -errno;
memset(&ifr, 0, sizeof(ifr));
@@ -240,7 +241,7 @@ ifaceCheck(bool reportError, const char *ifname,
ifaceError(VIR_ERR_INTERNAL_ERROR,
_("invalid interface name %s"),
ifname);
- rc = EINVAL;
+ rc = -EINVAL;
goto cleanup;
}
@@ -249,12 +250,12 @@ ifaceCheck(bool reportError, const char *ifname,
ifaceError(VIR_ERR_INTERNAL_ERROR,
_("coud not get MAC address of interface %s"),
ifname);
- rc = errno;
+ rc = -errno;
goto cleanup;
}
if (memcmp(&ifr.ifr_hwaddr.sa_data, macaddr, VIR_MAC_BUFLEN) != 0) {
- rc = ENODEV;
+ rc = -ENODEV;
goto cleanup;
}
}
@@ -262,7 +263,7 @@ ifaceCheck(bool reportError, const char *ifname,
if (ifindex != -1) {
rc = ifaceGetIndex(reportError, ifname, &idx);
if (rc == 0 && idx != ifindex)
- rc = ENODEV;
+ rc = -ENODEV;
}
cleanup:
@@ -279,7 +280,7 @@ ifaceCheck(bool reportError ATTRIBUTE_UNUSED,
const unsigned char *macaddr ATTRIBUTE_UNUSED,
int ifindex ATTRIBUTE_UNUSED)
{
- return ENOSYS;
+ return -ENOSYS;
}
#endif /* __linux__ */
@@ -294,9 +295,9 @@ ifaceCheck(bool reportError ATTRIBUTE_UNUSED,
*
* Get the index of an interface given its name.
*
- * Returns 0 on success, an error code on failure.
- * ENODEV : if interface with given name does not exist
- * EINVAL : if interface name is invalid (too long)
+ * Returns 0 on success, -errno on failure.
+ * -ENODEV : if interface with given name does not exist
+ * -EINVAL : if interface name is invalid (too long)
*/
#ifdef __linux__
int
@@ -307,7 +308,7 @@ ifaceGetIndex(bool reportError, const char *ifname, int *ifindex)
int fd = socket(PF_PACKET, SOCK_DGRAM, 0);
if (fd < 0)
- return errno;
+ return -errno;
memset(&ifreq, 0, sizeof(ifreq));
@@ -317,7 +318,7 @@ ifaceGetIndex(bool reportError, const char *ifname, int *ifindex)
ifaceError(VIR_ERR_INTERNAL_ERROR,
_("invalid interface name %s"),
ifname);
- rc = EINVAL;
+ rc = -EINVAL;
goto cleanup;
}
@@ -328,7 +329,7 @@ ifaceGetIndex(bool reportError, const char *ifname, int *ifindex)
ifaceError(VIR_ERR_INTERNAL_ERROR,
_("interface %s does not exist"),
ifname);
- rc = ENODEV;
+ rc = -ENODEV;
}
cleanup:
@@ -349,7 +350,7 @@ ifaceGetIndex(bool reportError,
_("ifaceGetIndex is not supported on non-linux
platforms"));
}
- return ENOSYS;
+ return -ENOSYS;
}
#endif /* __linux__ */
@@ -364,15 +365,15 @@ ifaceGetVlanID(const char *vlanifname, int *vlanid) {
int fd = socket(PF_PACKET, SOCK_DGRAM, 0);
if (fd < 0)
- return errno;
+ return -errno;
if (virStrcpyStatic(vlanargs.device1, vlanifname) == NULL) {
- rc = EINVAL;
+ rc = -EINVAL;
goto cleanup;
}
if (ioctl(fd, SIOCGIFVLAN, &vlanargs) != 0) {
- rc = errno;
+ rc = -errno;
goto cleanup;
}
@@ -393,7 +394,7 @@ ifaceGetVlanID(const char *vlanifname ATTRIBUTE_UNUSED,
ifaceError(VIR_ERR_INTERNAL_ERROR, "%s",
_("ifaceGetVlanID is not supported on non-linux platforms"));
- return ENOSYS;
+ return -ENOSYS;
}
#endif /* __linux__ */
@@ -404,7 +405,7 @@ ifaceGetVlanID(const char *vlanifname ATTRIBUTE_UNUSED,
*
* This function gets the @macaddr for a given interface @ifname.
*
- * Returns 0 in case of success or an errno code in case of failure.
+ * Returns 0 on success, -errno on failure.
*/
#ifdef __linux__
int
@@ -416,20 +417,20 @@ ifaceGetMacAddress(const char *ifname,
int rc = 0;
if (!ifname)
- return EINVAL;
+ return -EINVAL;
fd = socket(AF_INET, SOCK_STREAM, 0);
if (fd < 0)
- return errno;
+ return -errno;
memset(&ifr, 0, sizeof(struct ifreq));
if (virStrcpyStatic(ifr.ifr_name, ifname) == NULL) {
- rc = EINVAL;
+ rc = -EINVAL;
goto cleanup;
}
if (ioctl(fd, SIOCGIFHWADDR, (char *)&ifr) != 0) {
- rc = errno;
+ rc = -errno;
goto cleanup;
}
@@ -446,7 +447,7 @@ int
ifaceGetMacAddress(const char *ifname ATTRIBUTE_UNUSED,
unsigned char *macaddr ATTRIBUTE_UNUSED)
{
- return ENOSYS;
+ return -ENOSYS;
}
#endif /* __linux__ */
@@ -459,7 +460,7 @@ ifaceGetMacAddress(const char *ifname ATTRIBUTE_UNUSED,
* This function sets the @macaddr for a given interface @ifname. This
* gets rid of the kernel's automatically assigned random MAC.
*
- * Returns 0 in case of success or an errno code in case of failure.
+ * Returns 0 on success, -errno on failure.
*/
#ifdef __linux__
int
@@ -471,27 +472,27 @@ ifaceSetMacAddress(const char *ifname,
int rc = 0;
if (!ifname)
- return EINVAL;
+ return -EINVAL;
fd = socket(AF_INET, SOCK_STREAM, 0);
if (fd < 0)
- return errno;
+ return -errno;
memset(&ifr, 0, sizeof(struct ifreq));
if (virStrcpyStatic(ifr.ifr_name, ifname) == NULL) {
- rc = EINVAL;
+ rc = -EINVAL;
goto cleanup;
}
/* To fill ifr.ifr_hdaddr.sa_family field */
if (ioctl(fd, SIOCGIFHWADDR, &ifr) != 0) {
- rc = errno;
+ rc = -errno;
goto cleanup;
}
memcpy(ifr.ifr_hwaddr.sa_data, macaddr, VIR_MAC_BUFLEN);
- rc = ioctl(fd, SIOCSIFHWADDR, &ifr) == 0 ? 0 : errno;
+ rc = ioctl(fd, SIOCSIFHWADDR, &ifr) == 0 ? 0 : -errno;
cleanup:
VIR_FORCE_CLOSE(fd);
@@ -504,7 +505,7 @@ int
ifaceSetMacAddress(const char *ifname ATTRIBUTE_UNUSED,
const unsigned char *macaddr ATTRIBUTE_UNUSED)
{
- return ENOSYS;
+ return -ENOSYS;
}
#endif /* __linux__ */
@@ -546,7 +547,7 @@ ifaceMacvtapLinkAdd(const char *type,
struct nl_msg *nl_msg;
struct nlattr *linkinfo, *info_data;
- if (ifaceGetIndex(true, srcdev, &ifindex) != 0)
+ if (ifaceGetIndex(true, srcdev, &ifindex) < 0)
return -1;
*retry = 0;
@@ -969,8 +970,8 @@ ifaceGetNthParent(int ifindex, const char *ifname, unsigned int
nthParent,
*nth = 0;
- if (ifindex <= 0 && ifaceGetIndex(true, ifname, &ifindex) != 0)
- return 1;
+ if (ifindex <= 0 && ifaceGetIndex(true, ifname, &ifindex) < 0)
+ return -1;
while (!end && i <= nthParent) {
rc = ifaceMacvtapLinkDump(true, ifname, ifindex, tb, &recvbuf, NULL);
@@ -983,7 +984,7 @@ ifaceGetNthParent(int ifindex, const char *ifname, unsigned int
nthParent,
ifaceError(VIR_ERR_INTERNAL_ERROR, "%s",
_("buffer for root interface name is too small"));
VIR_FREE(recvbuf);
- return 1;
+ return -1;
}
*parent_ifindex = ifindex;
}
@@ -1033,7 +1034,7 @@ ifaceGetNthParent(int ifindex ATTRIBUTE_UNUSED,
* @linkdev: name of interface
* @stateDir: directory to store old MAC address
*
- * Returns 0 on success, -1 in case of fatal error, error code otherwise.
+ * Returns 0 on success, -errno on failure.
*
*/
int
@@ -1046,7 +1047,7 @@ ifaceReplaceMacAddress(const unsigned char *macaddress,
rc = ifaceGetMacAddress(linkdev, oldmac);
- if (rc) {
+ if (rc < 0) {
virReportSystemError(rc,
_("Getting MAC address from '%s' "
"to '%02x:%02x:%02x:%02x:%02x:%02x'
failed."),
@@ -1061,18 +1062,18 @@ ifaceReplaceMacAddress(const unsigned char *macaddress,
stateDir,
linkdev) < 0) {
virReportOOMError();
- return errno;
+ return -errno;
}
virFormatMacAddr(oldmac, macstr);
if (virFileWriteStr(path, macstr, O_CREAT|O_TRUNC|O_WRONLY) < 0) {
virReportSystemError(errno, _("Unable to preserve mac for %s"),
linkdev);
- return errno;
+ return -errno;
}
}
rc = ifaceSetMacAddress(linkdev, macaddress);
- if (rc) {
+ if (rc < 0) {
virReportSystemError(rc,
_("Setting MAC address on '%s' to "
"'%02x:%02x:%02x:%02x:%02x:%02x'
failed."),
@@ -1089,7 +1090,7 @@ ifaceReplaceMacAddress(const unsigned char *macaddress,
* @linkdev: name of interface
* @stateDir: directory containing old MAC address
*
- * Returns 0 on success, -1 in case of fatal error, error code otherwise.
+ * Returns 0 on success, -errno on failure.
*
*/
int
@@ -1106,11 +1107,11 @@ ifaceRestoreMacAddress(const char *linkdev,
stateDir,
linkdev) < 0) {
virReportOOMError();
- return -1;
+ return -errno;
}
if (virFileReadAll(path, VIR_MAC_STRING_BUFLEN, &macstr) < 0) {
- return errno;
+ return -errno;
}
if (virParseMacAddr(macstr, &oldmac[0]) != 0) {
@@ -1118,12 +1119,12 @@ ifaceRestoreMacAddress(const char *linkdev,
_("Cannot parse MAC address from '%s'"),
oldmacname);
VIR_FREE(macstr);
- return -1;
+ return -EINVAL;
}
/*reset mac and remove file-ignore results*/
rc = ifaceSetMacAddress(linkdev, oldmac);
- if (rc) {
+ if (rc < 0) {
virReportSystemError(rc,
_("Setting MAC address on '%s' to "
"'%02x:%02x:%02x:%02x:%02x:%02x'
failed."),
diff --git a/src/util/macvtap.c b/src/util/macvtap.c
index 7b97c6a..86e52fa 100644
--- a/src/util/macvtap.c
+++ b/src/util/macvtap.c
@@ -309,14 +309,14 @@ openMacvtapTap(const char *tgifname,
cr_ifname = tgifname;
rc = ifaceMacvtapLinkAdd(type, macaddress, 6, tgifname, linkdev,
macvtapMode, &do_retry);
- if (rc)
+ if (rc < 0)
return -1;
} else {
create_name:
retries = 5;
for (c = 0; c < 8192; c++) {
snprintf(ifname, sizeof(ifname), MACVTAP_NAME_PATTERN, c);
- if (ifaceGetIndex(false, ifname, &ifindex) == ENODEV) {
+ if (ifaceGetIndex(false, ifname, &ifindex) == -ENODEV) {
rc = ifaceMacvtapLinkAdd(type, macaddress, 6, ifname, linkdev,
macvtapMode, &do_retry);
if (rc == 0)
@@ -340,7 +340,7 @@ create_name:
}
rc = ifaceUp(cr_ifname);
- if (rc != 0) {
+ if (rc < 0) {
virReportSystemError(errno,
_("cannot 'up' interface %s -- another "
"macvtap device may be 'up' and have the same
"
@@ -838,7 +838,7 @@ getPhysdevAndVlan(const char *ifname, int *root_ifindex, char
*root_ifname,
if (nth == 0)
break;
if (*vlanid == -1) {
- if (ifaceGetVlanID(root_ifname, vlanid))
+ if (ifaceGetVlanID(root_ifname, vlanid) < 0)
*vlanid = -1;
}
@@ -997,7 +997,7 @@ doPortProfileOp8021Qbh(const char *ifname,
if (rc)
goto err_exit;
- if (ifaceGetIndex(true, physfndev, &ifindex) != 0) {
+ if (ifaceGetIndex(true, physfndev, &ifindex) < 0) {
rc = 1;
goto err_exit;
}
--
1.7.3.4
11:22 p.m.
New subject: [libvirt] [PATCH] util: make interface.c functions consistently return < 0 on error
On 07/21/2011 10:03 PM, Laine Stump wrote:
(This patch must be pushed before the "listenNetwork"
patches can be
pushed)
All of the functions in util/interface.c were returning 0 on success,
but some returned -1 on error, and some returned a positive value
(usually the value of errno, but sometimes just 1). Libvirt's standard
is to return< 0 on error (in the case of functions that need to
return errno, -errno is returned.
This patch modifies all functions in interface.c to consistently
return< 0 on error, and makes changes to callers of those functions
where necessary.
I looked through the source code, and think you got all of the callers
correctly, as well as all of interface.c. Hopefully I didn't miss anything.
ACK.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
8:30 a.m.
New subject: [libvirt] [PATCH] util: make interface.c functions consistently return < 0 on error
On 07/22/2011 12:22 AM, Eric Blake wrote:
On 07/21/2011 10:03 PM, Laine Stump wrote:
> (This patch must be pushed before the "listenNetwork" patches can be
> pushed)
>
> All of the functions in util/interface.c were returning 0 on success,
> but some returned -1 on error, and some returned a positive value
> (usually the value of errno, but sometimes just 1). Libvirt's standard
> is to return< 0 on error (in the case of functions that need to
> return errno, -errno is returned.
>
> This patch modifies all functions in interface.c to consistently
> return< 0 on error, and makes changes to callers of those functions
> where necessary.
I looked through the source code, and think you got all of the callers
correctly, as well as all of interface.c. Hopefully I didn't miss
anything.
Thanks, I just pushed it.
Of course now that I've seen Dan's idea for nicer XML, it will be awhile
before I push the "listenNetwork" series that brought this up (hmm, I'll
have to change my shorthand for it, since that attribute will disappear)
3:11 a.m.
New subject: [libvirt] [PATCHv2 2/4] network: provide internal API to return IP of a network
The new listenNetwork atribute needs to learn an IP address based on a
named network. This patch provides a function networkGetNetworkAddress
which provides that.
Some networks have an IP address explicitly in their configuration
(ie, those with a forward type of "none", "route", or
"nat"). For
those, we can just return the IP address from the config.
The rest will have a physical device associated with them (either via
<bridge name='...'/>, <forward ... dev='...'/>, or possibly
via a pool
of interfaces inside the network's <forward> element) and we will need
to ask the kernel for the current IP address of that device (via the
newly added ifaceGetIPAddress
If networkGetNetworkAddress encounters an error while trying to learn
the address for a network, it will return -1. In the case that libvirt
has been compiled without the network driver, a static inline version
of the function, which returns -2, will be used. This allows
differentiating between a failure of the network driver, and its
complete absence.
---
src/libvirt_network.syms | 1 +
src/network/bridge_driver.c | 100 +++++++++++++++++++++++++++++++++++++++++++
src/network/bridge_driver.h | 7 +++
3 files changed, 108 insertions(+), 0 deletions(-)
diff --git a/src/libvirt_network.syms b/src/libvirt_network.syms
index e402b5f..1fe8902 100644
--- a/src/libvirt_network.syms
+++ b/src/libvirt_network.syms
@@ -5,5 +5,6 @@
# bridge_driver.h
networkAllocateActualDevice;
networkBuildDhcpDaemonCommandLine;
+networkGetNetworkAddress;
networkNotifyActualDevice;
networkReleaseActualDevice;
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 15e0710..64ca9a8 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -55,6 +55,7 @@
#include "uuid.h"
#include "iptables.h"
#include "bridge.h"
+#include "interface.h"
#include "logging.h"
#include "dnsmasq.h"
#include "util/network.h"
@@ -3087,3 +3088,102 @@ cleanup:
iface->data.network.actual = NULL;
return ret;
}
+
+/*
+ * networkGetNetworkAddress:
+ * @netname: the name of a network
+ * @netaddr: string representation of IP address for that network.
+ *
+ * Attempt to return the IP (v4) address associated with the named
+ * network. If a libvirt virtual network, that will be provided in the
+ * configuration. For host bridge and direct (macvtap) networks, we
+ * must do an ioctl to learn the address.
+ *
+ * Note: This function returns the 1st IPv4 address it finds. It might
+ * be useful if it was more flexible, but the current use (getting a
+ * listen address for qemu's vnc/spice graphics server) can only use a
+ * single address anyway.
+ *
+ * Returns 0 on success, and puts a string (which must be free'd by
+ * the caller) into *netaddr. Returns -1 on failure
+ */
+int
+networkGetNetworkAddress(const char *netname, char **netaddr)
+{
+ int ret = -1;
+ struct network_driver *driver = driverState;
+ virNetworkObjPtr network = NULL;
+ virNetworkDefPtr netdef;
+ virNetworkIpDefPtr ipdef;
+ virSocketAddr addr;
+ virSocketAddrPtr addrptr = NULL;
+ char *devname = NULL;
+
+ *netaddr = NULL;
+ networkDriverLock(driver);
+ network = virNetworkFindByName(&driver->networks, netname);
+ networkDriverUnlock(driver);
+ if (!network) {
+ networkReportError(VIR_ERR_NO_NETWORK,
+ _("no network with matching name '%s'"),
+ netname);
+ goto cleanup;
+ }
+ netdef = network->def;
+
+ switch (netdef->forwardType) {
+ case VIR_NETWORK_FORWARD_NONE:
+ case VIR_NETWORK_FORWARD_NAT:
+ case VIR_NETWORK_FORWARD_ROUTE:
+ /* if there's an ipv4def, get it's address */
+ ipdef = virNetworkDefGetIpByIndex(netdef, AF_INET, 0);
+ if (!ipdef) {
+ networkReportError(VIR_ERR_INTERNAL_ERROR,
+ _("network '%s' doesn't have an IP
address"),
+ netdef->name);
+ break;
+ }
+ addrptr = &ipdef->address;
+ break;
+
+ case VIR_NETWORK_FORWARD_BRIDGE:
+ if ((devname = netdef->bridge))
+ break;
+ /*
+ * If netdef->bridge wasn't set, this is a direct-mode
+ * interface, so purposefully fall through to the next case
+ */
+ case VIR_NETWORK_FORWARD_PRIVATE:
+ case VIR_NETWORK_FORWARD_VEPA:
+ case VIR_NETWORK_FORWARD_PASSTHROUGH:
+ if ((netdef->nForwardIfs > 0) && netdef->forwardIfs)
+ devname = netdef->forwardIfs[0].dev;
+
+ if (!devname) {
+ networkReportError(VIR_ERR_INTERNAL_ERROR,
+ _("network '%s' has no associated interface
or bridge"),
+ netdef->name);
+ }
+ break;
+ }
+
+ if (devname) {
+ if (ifaceGetIPAddress(devname, &addr)) {
+ virReportSystemError(errno,
+ _("Failed to get IP address for '%s'
(network '%s')"),
+ devname, netdef->name);
+ } else {
+ addrptr = &addr;
+ }
+ }
+
+ if (addrptr) {
+ *netaddr = virSocketFormatAddr(addrptr);
+ ret = 0;
+ }
+
+cleanup:
+ if (network)
+ virNetworkObjUnlock(network);
+ return ret;
+}
diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h
index 710d862..930c526 100644
--- a/src/network/bridge_driver.h
+++ b/src/network/bridge_driver.h
@@ -40,6 +40,8 @@ int networkAllocateActualDevice(virDomainNetDefPtr iface);
int networkNotifyActualDevice(virDomainNetDefPtr iface);
int networkReleaseActualDevice(virDomainNetDefPtr iface);
+int networkGetNetworkAddress(const char *netname, char **netaddr);
+
int networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network,
virCommandPtr *cmdout, char *pidfile,
dnsmasqContext *dctx);
@@ -57,6 +59,11 @@ networkReleaseActualDevice(virDomainNetDefPtr iface ATTRIBUTE_UNUSED)
{ return 0; }
static inline int
+networkGetNetworkAddress(const char *netname ATTRIBUTE_UNUSED,
+ char **netaddr ATTRIBUTE_UNUSED)
+ { return -2; }
+
+static inline int
networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network ATTRIBUTE_UNUSED,
virCommandPtr *cmdout ATTRIBUTE_UNUSED,
char *pidfile ATTRIBUTE_UNUSED,
--
1.7.3.4
6:27 p.m.
New subject: [libvirt] [PATCHv2 2/4] network: provide internal API to return IP of a network
On 07/20/2011 02:11 AM, Laine Stump wrote:
The new listenNetwork atribute needs to learn an IP address based on
a
s/atribute/attribute/
named network. This patch provides a function
networkGetNetworkAddress
which provides that.
Some networks have an IP address explicitly in their configuration
(ie, those with a forward type of "none", "route", or
"nat"). For
those, we can just return the IP address from the config.
The rest will have a physical device associated with them (either via
<bridge name='...'/>,<forward ... dev='...'/>, or possibly
via a pool
of interfaces inside the network's<forward> element) and we will need
to ask the kernel for the current IP address of that device (via the
s/the current/a current/ - the kernel may know more than one, but we
only care about finding at least one (and as you pointed out in the
previous patch, you only get the first).
newly added ifaceGetIPAddress
Missing )
If networkGetNetworkAddress encounters an error while trying to learn
the address for a network, it will return -1. In the case that libvirt
has been compiled without the network driver, a static inline version
of the function, which returns -2, will be used. This allows
differentiating between a failure of the network driver, and its
complete absence.
---
src/libvirt_network.syms | 1 +
src/network/bridge_driver.c | 100 +++++++++++++++++++++++++++++++++++++++++++
src/network/bridge_driver.h | 7 +++
3 files changed, 108 insertions(+), 0 deletions(-)
+
+/*
+ * networkGetNetworkAddress:
+ * @netname: the name of a network
+ * @netaddr: string representation of IP address for that network.
+ *
+ * Attempt to return the IP (v4) address associated with the named
s/the IP/an IP/
+ * network. If a libvirt virtual network, that will be provided in
the
+ * configuration. For host bridge and direct (macvtap) networks, we
+ * must do an ioctl to learn the address.
+ *
+ * Note: This function returns the 1st IPv4 address it finds. It might
+ * be useful if it was more flexible, but the current use (getting a
+ * listen address for qemu's vnc/spice graphics server) can only use a
+ * single address anyway.
+ *
+ * Returns 0 on success, and puts a string (which must be free'd by
+ * the caller) into *netaddr. Returns -1 on failure
Might also be worth mentioning -2 on complete lack of support.
+ case VIR_NETWORK_FORWARD_ROUTE:
+ /* if there's an ipv4def, get it's address */
+ ipdef = virNetworkDefGetIpByIndex(netdef, AF_INET, 0);
+ if (!ipdef) {
+ networkReportError(VIR_ERR_INTERNAL_ERROR,
+ _("network '%s' doesn't have an IP
address"),
It might have an IPv6 address though, so touch up the error message:
s/IP/IPv4/
+ case VIR_NETWORK_FORWARD_BRIDGE:
+ if ((devname = netdef->bridge))
+ break;
+ /*
+ * If netdef->bridge wasn't set, this is a direct-mode
+ * interface, so purposefully fall through to the next case
+ */
+ case VIR_NETWORK_FORWARD_PRIVATE:
Coverity will probably gripe at this one. We may need to re-word this:
/* Fall through if netdef->bridge wasn't set, since this is a
direct-mode interface */
+++ b/src/network/bridge_driver.h
@@ -40,6 +40,8 @@ int networkAllocateActualDevice(virDomainNetDefPtr iface);
int networkNotifyActualDevice(virDomainNetDefPtr iface);
int networkReleaseActualDevice(virDomainNetDefPtr iface);
+int networkGetNetworkAddress(const char *netname, char **netaddr);
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
+
int networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network,
virCommandPtr *cmdout, char *pidfile,
dnsmasqContext *dctx);
@@ -57,6 +59,11 @@ networkReleaseActualDevice(virDomainNetDefPtr iface ATTRIBUTE_UNUSED)
{ return 0; }
static inline int
+networkGetNetworkAddress(const char *netname ATTRIBUTE_UNUSED,
+ char **netaddr ATTRIBUTE_UNUSED)
+ { return -2; }
# define networkGetNetworkAddress(netname, netaddr) (-2)
ACK with those nits fixed.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
3:11 a.m.
New subject: [libvirt] [PATCHv2 3/4] conf: add listenNetwork attribute to domain graphics element
Once it's plugged in, listenNetwork will be an optional replacement
for the "listen" attribute. While listen can be a host name or IP
address, listenNetwork names one of the networks managed by libvirt
(with virNetwork*()/visrh net-*).
---
docs/schemas/domain.rng | 33 ++++++++---
src/conf/domain_conf.c | 60 +++++++++++++++++--
src/conf/domain_conf.h | 3 +
.../qemuxml2argv-graphics-listenNetwork.xml | 30 ++++++++++
tests/qemuxml2xmltest.c | 1 +
5 files changed, 111 insertions(+), 16 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-graphics-listenNetwork.xml
diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng
index 07c63bd..42f3eb2 100644
--- a/docs/schemas/domain.rng
+++ b/docs/schemas/domain.rng
@@ -1241,9 +1241,14 @@
</attribute>
</optional>
<optional>
- <attribute name="listen">
- <ref name="addrIPorName"/>
- </attribute>
+ <choice>
+ <attribute name="listen">
+ <ref name="addrIPorName"/>
+ </attribute>
+ <attribute name="listenNetwork">
+ <text/>
+ </attribute>
+ </choice>
</optional>
</group>
<group>
@@ -1300,9 +1305,14 @@
</attribute>
</optional>
<optional>
- <attribute name="listen">
- <ref name="addrIPorName"/>
- </attribute>
+ <choice>
+ <attribute name="listen">
+ <ref name="addrIPorName"/>
+ </attribute>
+ <attribute name="listenNetwork">
+ <text/>
+ </attribute>
+ </choice>
</optional>
<optional>
<attribute name="passwd">
@@ -1461,9 +1471,14 @@
</attribute>
</optional>
<optional>
- <attribute name="listen">
- <ref name="addrIPorName"/>
- </attribute>
+ <choice>
+ <attribute name="listen">
+ <ref name="addrIPorName"/>
+ </attribute>
+ <attribute name="listenNetwork">
+ <text/>
+ </attribute>
+ </choice>
</optional>
</group>
<group>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 4c58633..035b743 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -633,6 +633,7 @@ void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def)
switch (def->type) {
case VIR_DOMAIN_GRAPHICS_TYPE_VNC:
VIR_FREE(def->data.vnc.listenAddr);
+ VIR_FREE(def->data.vnc.listenNetwork);
VIR_FREE(def->data.vnc.socket);
VIR_FREE(def->data.vnc.keymap);
virDomainGraphicsAuthDefClear(&def->data.vnc.auth);
@@ -645,6 +646,7 @@ void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def)
case VIR_DOMAIN_GRAPHICS_TYPE_RDP:
VIR_FREE(def->data.rdp.listenAddr);
+ VIR_FREE(def->data.rdp.listenNetwork);
break;
case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP:
@@ -653,6 +655,7 @@ void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def)
case VIR_DOMAIN_GRAPHICS_TYPE_SPICE:
VIR_FREE(def->data.spice.listenAddr);
+ VIR_FREE(def->data.spice.listenNetwork);
VIR_FREE(def->data.spice.keymap);
virDomainGraphicsAuthDefClear(&def->data.spice.auth);
break;
@@ -4061,14 +4064,23 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, unsigned int flags)
VIR_FREE(autoport);
}
- def->data.vnc.listenAddr = virXMLPropString(node, "listen");
def->data.vnc.socket = virXMLPropString(node, "socket");
def->data.vnc.keymap = virXMLPropString(node, "keymap");
- if (def->data.vnc.listenAddr &&
- !def->data.vnc.listenAddr[0])
+ def->data.vnc.listenAddr = virXMLPropString(node, "listen");
+ if (def->data.vnc.listenAddr && !def->data.vnc.listenAddr[0])
VIR_FREE(def->data.vnc.listenAddr);
+ def->data.vnc.listenNetwork = virXMLPropString(node,
"listenNetwork");
+ if (def->data.vnc.listenNetwork &&
!def->data.vnc.listenNetwork[0])
+ VIR_FREE(def->data.vnc.listenNetwork);
+
+ if (def->data.vnc.listenAddr && def->data.vnc.listenNetwork) {
+ virDomainReportError(VIR_ERR_INTERNAL_ERROR,
+ "%s", _("Both listen and listenNetwork
given in graphics element"));
+ goto error;
+ }
+
if (virDomainGraphicsAuthDefParseXML(node, &def->data.vnc.auth,
def->type) < 0)
goto error;
@@ -4138,6 +4150,17 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, unsigned int flags)
if (def->data.rdp.listenAddr &&
!def->data.rdp.listenAddr[0])
VIR_FREE(def->data.rdp.listenAddr);
+
+ def->data.rdp.listenNetwork = virXMLPropString(node,
"listenNetwork");
+ if (def->data.rdp.listenNetwork &&
!def->data.rdp.listenNetwork[0])
+ VIR_FREE(def->data.rdp.listenNetwork);
+
+ if (def->data.rdp.listenAddr && def->data.rdp.listenNetwork) {
+ virDomainReportError(VIR_ERR_INTERNAL_ERROR,
+ "%s", _("Both listen and listenNetwork
given in graphics element"));
+ goto error;
+ }
+
} else if (def->type == VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP) {
char *fullscreen = virXMLPropString(node, "fullscreen");
@@ -4199,13 +4222,21 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, unsigned int flags)
VIR_FREE(autoport);
}
- def->data.spice.listenAddr = virXMLPropString(node, "listen");
def->data.spice.keymap = virXMLPropString(node, "keymap");
-
- if (def->data.spice.listenAddr &&
- !def->data.spice.listenAddr[0])
+ def->data.spice.listenAddr = virXMLPropString(node, "listen");
+ if (def->data.spice.listenAddr && !def->data.spice.listenAddr[0])
VIR_FREE(def->data.spice.listenAddr);
+ def->data.spice.listenNetwork = virXMLPropString(node,
"listenNetwork");
+ if (def->data.spice.listenNetwork &&
!def->data.spice.listenNetwork[0])
+ VIR_FREE(def->data.spice.listenNetwork);
+
+ if (def->data.spice.listenAddr && def->data.spice.listenNetwork) {
+ virDomainReportError(VIR_ERR_INTERNAL_ERROR,
+ "%s", _("Both listen and listenNetwork
given in graphics element"));
+ goto error;
+ }
+
if (virDomainGraphicsAuthDefParseXML(node, &def->data.spice.auth,
def->type) < 0)
goto error;
@@ -9431,6 +9462,11 @@ virDomainGraphicsDefFormat(virBufferPtr buf,
if (def->data.vnc.listenAddr)
virBufferAsprintf(buf, " listen='%s'",
def->data.vnc.listenAddr);
+
+ if (def->data.vnc.listenNetwork) {
+ virBufferAsprintf(buf, " listenNetwork='%s'",
+ def->data.vnc.listenNetwork);
+ }
}
if (def->data.vnc.keymap)
@@ -9472,6 +9508,11 @@ virDomainGraphicsDefFormat(virBufferPtr buf,
if (def->data.rdp.listenAddr)
virBufferAsprintf(buf, " listen='%s'",
def->data.rdp.listenAddr);
+ if (def->data.rdp.listenNetwork) {
+ virBufferAsprintf(buf, " listenNetwork='%s'",
+ def->data.rdp.listenNetwork);
+ }
+
break;
case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP:
@@ -9500,6 +9541,11 @@ virDomainGraphicsDefFormat(virBufferPtr buf,
virBufferAsprintf(buf, " listen='%s'",
def->data.spice.listenAddr);
+ if (def->data.spice.listenNetwork) {
+ virBufferAsprintf(buf, " listenNetwork='%s'",
+ def->data.spice.listenNetwork);
+ }
+
if (def->data.spice.keymap)
virBufferEscapeString(buf, " keymap='%s'",
def->data.spice.keymap);
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 9e9db41..7f05084 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -768,6 +768,7 @@ struct _virDomainGraphicsDef {
int port;
unsigned int autoport :1;
char *listenAddr;
+ char *listenNetwork;
char *keymap;
char *socket;
virDomainGraphicsAuthDef auth;
@@ -780,6 +781,7 @@ struct _virDomainGraphicsDef {
struct {
int port;
char *listenAddr;
+ char *listenNetwork;
unsigned int autoport :1;
unsigned int replaceUser :1;
unsigned int multiUser :1;
@@ -792,6 +794,7 @@ struct _virDomainGraphicsDef {
int port;
int tlsPort;
char *listenAddr;
+ char *listenNetwork;
char *keymap;
virDomainGraphicsAuthDef auth;
unsigned int autoport :1;
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-listenNetwork.xml
b/tests/qemuxml2argvdata/qemuxml2argv-graphics-listenNetwork.xml
new file mode 100644
index 0000000..f7757f1
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-listenNetwork.xml
@@ -0,0 +1,30 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory>219100</memory>
+ <currentMemory>219100</currentMemory>
+ <vcpu>1</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu</emulator>
+ <disk type='block' device='disk'>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0'
unit='0'/>
+ </disk>
+ <controller type='ide' index='0'/>
+ <input type='mouse' bus='ps2'/>
+ <graphics type='vnc' port='5903' autoport='no'
listenNetwork='Bobsnetwork'/>
+ <video>
+ <model type='cirrus' vram='9216' heads='1'/>
+ </video>
+ <memballoon model='virtio'/>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 6b1fbf5..e8b5ece 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -139,6 +139,7 @@ mymain(void)
DO_TEST("disk-drive-cache-v1-wb");
DO_TEST("disk-drive-cache-v1-none");
DO_TEST("disk-scsi-device");
+ DO_TEST("graphics-listenNetwork");
DO_TEST("graphics-vnc");
DO_TEST("graphics-vnc-sasl");
DO_TEST("graphics-vnc-tls");
--
1.7.3.4
6:32 p.m.
New subject: [libvirt] [PATCHv2 3/4] conf: add listenNetwork attribute to domain graphics element
On 07/20/2011 02:11 AM, Laine Stump wrote:
Once it's plugged in, listenNetwork will be an optional
replacement
for the "listen" attribute. While listen can be a host name or IP
address, listenNetwork names one of the networks managed by libvirt
(with virNetwork*()/visrh net-*).
---
docs/schemas/domain.rng | 33 ++++++++---
src/conf/domain_conf.c | 60 +++++++++++++++++--
src/conf/domain_conf.h | 3 +
.../qemuxml2argv-graphics-listenNetwork.xml | 30 ++++++++++
tests/qemuxml2xmltest.c | 1 +
5 files changed, 111 insertions(+), 16 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-graphics-listenNetwork.xml
diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng
index 07c63bd..42f3eb2 100644
--- a/docs/schemas/domain.rng
+++ b/docs/schemas/domain.rng
@@ -1241,9 +1241,14 @@
</attribute>
</optional>
<optional>
-<attribute name="listen">
-<ref name="addrIPorName"/>
-</attribute>
+<choice>
+<attribute name="listen">
+<ref name="addrIPorName"/>
+</attribute>
+<attribute name="listenNetwork">
+<text/>
[stupid thunderbird whitespace corruption]
Shouldn't this be something more specific than <text/>? A <ref
name=.../> to something that matches valid network names might be more
appropriate. Then again, network.rng uses <text/> for the name element,
so I guess we're okay here.
@@ -1300,9 +1305,14 @@
</attribute>
</optional>
<optional>
-<attribute name="listen">
-<ref name="addrIPorName"/>
-</attribute>
+<choice>
+<attribute name="listen">
+<ref name="addrIPorName"/>
+</attribute>
+<attribute name="listenNetwork">
+<text/>
+</attribute>
+</choice>
We repeat this <choice> enough times; maybe it's worth factorizing into
a macro and using it by <ref> in all three call sites? Up to you - it's
not a show-stopper to the patch as-is.
ACK.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
10:32 p.m.
New subject: [libvirt] [PATCHv2 3/4] conf: add listenNetwork attribute to domain graphics element
On 07/21/2011 07:32 PM, Eric Blake wrote:
On 07/20/2011 02:11 AM, Laine Stump wrote:
> Once it's plugged in, listenNetwork will be an optional replacement
> for the "listen" attribute. While listen can be a host name or IP
> address, listenNetwork names one of the networks managed by libvirt
> (with virNetwork*()/visrh net-*).
> ---
> docs/schemas/domain.rng | 33 ++++++++---
> src/conf/domain_conf.c | 60
> +++++++++++++++++--
> src/conf/domain_conf.h | 3 +
> .../qemuxml2argv-graphics-listenNetwork.xml | 30 ++++++++++
> tests/qemuxml2xmltest.c | 1 +
> 5 files changed, 111 insertions(+), 16 deletions(-)
> create mode 100644
> tests/qemuxml2argvdata/qemuxml2argv-graphics-listenNetwork.xml
>
> diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng
> index 07c63bd..42f3eb2 100644
> --- a/docs/schemas/domain.rng
> +++ b/docs/schemas/domain.rng
> @@ -1241,9 +1241,14 @@
> </attribute>
> </optional>
> <optional>
> -<attribute name="listen">
> -<ref name="addrIPorName"/>
> -</attribute>
> +<choice>
> +<attribute name="listen">
> +<ref name="addrIPorName"/>
> +</attribute>
> +<attribute name="listenNetwork">
> +<text/>
[stupid thunderbird whitespace corruption]
Shouldn't this be something more specific than <text/>? A <ref
name=.../> to something that matches valid network names might be more
appropriate. Then again, network.rng uses <text/> for the name
element, so I guess we're okay here.
Right. We have to accept any valid network name, and currently
(according to the RNG) a network name can be anything.
> @@ -1300,9 +1305,14 @@
> </attribute>
> </optional>
> <optional>
> -<attribute name="listen">
> -<ref name="addrIPorName"/>
> -</attribute>
> +<choice>
> +<attribute name="listen">
> +<ref name="addrIPorName"/>
> +</attribute>
> +<attribute name="listenNetwork">
> +<text/>
> +</attribute>
> +</choice>
We repeat this <choice> enough times; maybe it's worth factorizing
into a macro and using it by <ref> in all three call sites? Up to you
- it's not a show-stopper to the patch as-is.
I think I actually prefer them separate, that makes it easier for
someone reading the rng to see just what attributes are in each element,
without needing to traipse up and down the file.
I'll push this one as is (after re-basing.)
3:11 a.m.
New subject: [libvirt] [PATCHv2 4/4] qemu: support the listenNetwork attribute in <graphics>
The domain XML now understands an attribute named "listenNetwork" in
the <graphics> element, and the network driver has an internal API
that will turn a network name into an IP address, so the final logical
step is to put the glue into the qemu driver such that when it is
starting up a domain, if it finds "listenNetwork" in the XML, it will
call the network driver to get an associated IP address, and tell qemu
to listen for vnc (or spice) on that address rather than the default
(localhost).
The motivation for this is that a large installation may want the
guests' VNC servers listening on physical interfaces rather than
localhost, so that users can connect directly from the outside; this
requires sending qemu the appropriate IP address to listen on. But
this address will of course be different for each host, and if a guest
might be migrated around from one host to another, it's important that
the guest's config not have any information embedded in it that is
specific to one particular host. listenNetwork can solve this problem
in the following manner:
1) on each host, define a libvirt network of the same name,
associated with some interface on that host (for example, a
simple macvtap network: <forward mode='bridge' dev='eth0'/>,
or
host bridge network: <forward mode='bridge'/> <bridge
name='br0'/>
2) in the <graphics> element of each guest's domain xml, tell vnc to
listen on the network name used in step 1:
<graphics type='vnc' autoport='yes'
listenNetwork='example-net'/>
(all the above also applies for graphics type='spice').
Since this is the commit that turns on the new functionality, I've
included the doc changes here.
---
docs/formatdomain.html.in | 22 ++++++++++++++---
src/qemu/qemu_command.c | 55 ++++++++++++++++++++++++++++++++++++++++-----
src/qemu/qemu_hotplug.c | 12 +++++++++
3 files changed, 79 insertions(+), 10 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 8f42ba9..2788190 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -1934,8 +1934,15 @@ qemu-kvm -net nic,model=? /dev/null
auto-allocated). The <code>autoport</code> attribute is
the new preferred syntax for indicating autoallocation of
the TCP port to use. The <code>listen</code> attribute is
- an IP address for the server to listen
- on. The <code>passwd</code> attribute provides a VNC
+ an IP address for the server to listen on.
+ Alternately, a <code>listenNetwork</code> attribute can be
+ specified instead<span class="since">Since
0.9.4</span>;
+ the named network will be looked up in the list of
+ networks managed by libvirt and, if found, the VNC server
+ will listen on the IP address associated with the physical
+ device used by that network (e.g. the bridge device or the
+ direct mode forward <code>dev</code>).
+ The <code>passwd</code> attribute provides a VNC
password in clear text. The <code>keymap</code> attribute
specifies the keymap to use. It is possible to set a limit
on the validity of the password be giving an
@@ -1959,8 +1966,15 @@ qemu-kvm -net nic,model=? /dev/null
port number. The <code>autoport</code> attribute is the
new preferred syntax for indicating autoallocation of
both port numbers. The <code>listen</code> attribute is
- an IP address for the server to listen
- on. The <code>passwd</code> attribute provides a SPICE
+ an IP address for the server to listen on.
+ Alternately, a <code>listenNetwork</code> attribute can be
+ specified instead<span class="since">Since
0.9.4</span>;
+ the named network will be looked up in the list of
+ networks managed by libvirt and, if found, the VNC server
+ will listen on the IP address associated with the physical
+ device used by that network (e.g. the bridge device or the
+ direct mode forward <code>dev</code>).
+ The <code>passwd</code> attribute provides a SPICE
password in clear text. The <code>keymap</code>
attribute specifies the keymap to use. It is possible to
set a limit on the validity of the password be giving an
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 0ae8d67..bccb984 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4116,16 +4116,40 @@ qemuBuildCommandLine(virConnectPtr conn,
def->graphics[0]->data.vnc.socket);
} else if (qemuCapsGet(qemuCaps, QEMU_CAPS_VNC_COLON)) {
- const char *addr = def->graphics[0]->data.vnc.listenAddr ?
- def->graphics[0]->data.vnc.listenAddr :
- driver->vncListen;
- bool escapeAddr = strchr(addr, ':') != NULL;
+ int ret;
+ char *addr;
+ bool freeAddr = false;
+ bool escapeAddr;
+
+ if (def->graphics[0]->data.vnc.listenAddr) {
+ addr = def->graphics[0]->data.vnc.listenAddr;
+ } else if (def->graphics[0]->data.vnc.listenNetwork) {
+ ret =
networkGetNetworkAddress(def->graphics[0]->data.vnc.listenNetwork,
+ &addr);
+ if (ret <= -2) {
+ qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ "%s", _("listenNetwork not supported,
network driver not present"));
+ }
+ if (!addr) {
+ qemuReportError(VIR_ERR_XML_ERROR,
+ _("listenNetwork '%s' had no usable
address"),
+ def->graphics[0]->data.vnc.listenNetwork);
+ goto error;
+ }
+ freeAddr = true;
+ } else {
+ addr = driver->vncListen;
+ }
+
+ escapeAddr = strchr(addr, ':') != NULL;
if (escapeAddr)
virBufferAsprintf(&opt, "[%s]", addr);
else
virBufferAdd(&opt, addr, -1);
virBufferAsprintf(&opt, ":%d",
def->graphics[0]->data.vnc.port - 5900);
+ if (freeAddr)
+ VIR_FREE(addr);
} else {
virBufferAsprintf(&opt, "%d",
@@ -4222,10 +4246,29 @@ qemuBuildCommandLine(virConnectPtr conn,
if (driver->spiceTLS && def->graphics[0]->data.spice.tlsPort !=
-1)
virBufferAsprintf(&opt, ",tls-port=%u",
def->graphics[0]->data.spice.tlsPort);
- if (def->graphics[0]->data.spice.listenAddr)
+ if (def->graphics[0]->data.spice.listenAddr) {
virBufferAsprintf(&opt, ",addr=%s",
def->graphics[0]->data.spice.listenAddr);
- else if (driver->spiceListen)
+ } else if (def->graphics[0]->data.spice.listenNetwork) {
+ int ret;
+ char *addr;
+
+ ret =
networkGetNetworkAddress(def->graphics[0]->data.spice.listenNetwork,
+ &addr);
+ if (ret <= -2) {
+ qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ "%s", _("listenNetwork not supported,
network driver not present"));
+ }
+ if (!addr) {
+ qemuReportError(VIR_ERR_XML_ERROR,
+ _("listenNetwork '%s' had no usable
address"),
+ def->graphics[0]->data.spice.listenNetwork);
+ goto error;
+ }
+ virBufferAsprintf(&opt, ",addr=%s", addr);
+ VIR_FREE(addr);
+ } else if (driver->spiceListen) {
virBufferAsprintf(&opt, ",addr=%s", driver->spiceListen);
+ }
/* In the password case we set it via monitor command, to avoid
* making it visible on CLI, so there's no use of password=XXX
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 74ce9be..09023b0 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1075,6 +1075,12 @@ qemuDomainChangeGraphics(struct qemud_driver *driver,
_("cannot change listen address setting on vnc
graphics"));
return -1;
}
+ if (STRNEQ_NULLABLE(olddev->data.vnc.listenNetwork,
+ dev->data.vnc.listenNetwork)) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("cannot change listen network setting on vnc
graphics"));
+ return -1;
+ }
if (STRNEQ_NULLABLE(olddev->data.vnc.keymap, dev->data.vnc.keymap)) {
qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("cannot change keymap setting on vnc
graphics"));
@@ -1124,6 +1130,12 @@ qemuDomainChangeGraphics(struct qemud_driver *driver,
_("cannot change listen address setting on spice
graphics"));
return -1;
}
+ if (STRNEQ_NULLABLE(olddev->data.spice.listenNetwork,
+ dev->data.spice.listenNetwork)) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("cannot change listen address setting on spice
graphics"));
+ return -1;
+ }
if (STRNEQ_NULLABLE(olddev->data.spice.keymap,
dev->data.spice.keymap)) {
qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
--
1.7.3.4
6:40 p.m.
New subject: [libvirt] [PATCHv2 4/4] qemu: support the listenNetwork attribute in <graphics>
On 07/20/2011 02:11 AM, Laine Stump wrote:
The domain XML now understands an attribute named
"listenNetwork" in
the<graphics> element, and the network driver has an internal API
that will turn a network name into an IP address, so the final logical
step is to put the glue into the qemu driver such that when it is
starting up a domain, if it finds "listenNetwork" in the XML, it will
call the network driver to get an associated IP address, and tell qemu
to listen for vnc (or spice) on that address rather than the default
(localhost).
Since this is the commit that turns on the new functionality, I've
included the doc changes here.
Yay. Did you ever finish the other doc changes you were promising?
} else if (qemuCapsGet(qemuCaps, QEMU_CAPS_VNC_COLON)) {
- const char *addr = def->graphics[0]->data.vnc.listenAddr ?
- def->graphics[0]->data.vnc.listenAddr :
- driver->vncListen;
- bool escapeAddr = strchr(addr, ':') != NULL;
+ int ret;
+ char *addr;
Uninitialized...
+ bool freeAddr = false;
+ bool escapeAddr;
+
+ if (def->graphics[0]->data.vnc.listenAddr) {
+ addr = def->graphics[0]->data.vnc.listenAddr;
+ } else if (def->graphics[0]->data.vnc.listenNetwork) {
+ ret =
networkGetNetworkAddress(def->graphics[0]->data.vnc.listenNetwork,
+&addr);
and if networkGetNetworkAddress is stubbed out, addr remains
uninitialized...
+ if (ret<= -2) {
+ qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ "%s", _("listenNetwork not supported,
network driver not present"));
+ }
+ if (!addr) {
Oops - jump depending on uninit data - valgrind will call you on that!
Not to mention that it looks odd to use ret <= -2, then !addr. I would
have expected ret <= -2, then ret < 0, for consistency. Simplest fix
would be to add the missing "goto error;" statement in the ret <= -2 clause.
+ } else if (def->graphics[0]->data.spice.listenNetwork)
{
+ int ret;
+ char *addr;
+
+ ret =
networkGetNetworkAddress(def->graphics[0]->data.spice.listenNetwork,
+&addr);
+ if (ret<= -2) {
+ qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ "%s", _("listenNetwork not supported,
network driver not present"));
+ }
+ if (!addr) {
Same problem with addr.
ACK with those fixed.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
10:58 p.m.
New subject: [libvirt] [PATCHv2 4/4] qemu: support the listenNetwork attribute in <graphics>
On 07/21/2011 07:40 PM, Eric Blake wrote:
On 07/20/2011 02:11 AM, Laine Stump wrote:
> The domain XML now understands an attribute named "listenNetwork" in
> the<graphics> element, and the network driver has an internal API
> that will turn a network name into an IP address, so the final logical
> step is to put the glue into the qemu driver such that when it is
> starting up a domain, if it finds "listenNetwork" in the XML, it will
> call the network driver to get an associated IP address, and tell qemu
> to listen for vnc (or spice) on that address rather than the default
> (localhost).
>
> Since this is the commit that turns on the new functionality, I've
> included the doc changes here.
Yay. Did you ever finish the other doc changes you were promising?
Not yet, but before the end of Friday.
> } else if (qemuCapsGet(qemuCaps, QEMU_CAPS_VNC_COLON)) {
> - const char *addr = def->graphics[0]->data.vnc.listenAddr ?
> - def->graphics[0]->data.vnc.listenAddr :
> - driver->vncListen;
> - bool escapeAddr = strchr(addr, ':') != NULL;
> + int ret;
> + char *addr;
Uninitialized...
> + bool freeAddr = false;
> + bool escapeAddr;
> +
> + if (def->graphics[0]->data.vnc.listenAddr) {
> + addr = def->graphics[0]->data.vnc.listenAddr;
> + } else if (def->graphics[0]->data.vnc.listenNetwork) {
> + ret =
> networkGetNetworkAddress(def->graphics[0]->data.vnc.listenNetwork,
> +&addr);
and if networkGetNetworkAddress is stubbed out, addr remains
uninitialized...
> + if (ret<= -2) {
> + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + "%s", _("listenNetwork not
> supported, network driver not present"));
> + }
> + if (!addr) {
Oops - jump depending on uninit data - valgrind will call you on that!
Not to mention that it looks odd to use ret <= -2, then !addr. I
would have expected ret <= -2, then ret < 0, for consistency.
Simplest fix would be to add the missing "goto error;" statement in
the ret <= -2 clause.
> + } else if (def->graphics[0]->data.spice.listenNetwork) {
> + int ret;
> + char *addr;
> +
> + ret =
> networkGetNetworkAddress(def->graphics[0]->data.spice.listenNetwork,
> +&addr);
> + if (ret<= -2) {
> + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + "%s", _("listenNetwork not
> supported, network driver not present"));
> + }
> + if (!addr) {
Same problem with addr.
ACK with those fixed.
Okay, I fixed those up, but am waiting to push until the new
prerequisite patch is pushed (changing the returns of functions in
util/interface.c to be < 0 on error).
5:37 a.m.
On Wed, Jul 20, 2011 at 04:11:11AM -0400, Laine Stump wrote:
This patch series resolves the following bug:
https://bugzilla.redhat.com/show_bug.cgi?id=703851
In short, it implements support for a new "listenNetwork" attribute in
the domain's <graphics> element. When listenNetwork is specified,
libvirt will look for a network of that name (from the networks that
can be created within libvirt), and derive a listen address from the
physical interface used to connect that network to the outside
world. (More details are given in each commit message).
I think my main thought on this is whether we are being forward
thinking enough. The problems with the current 'listen' attribute
- You can only list one address
- The address is tied to a specific machine
The 'listenNetwork' attribute would solve the second, but do
nothing to solve the first.
I think we ought to thus use a subelement
<graphics>
<listen type='network' network='foo'/>
</graphics>
<graphics>
<listen type='address' address='192.168.122.122'/>
<listen type='address' address='10.0.0.1'/>
<listen type='address' address='fec0:1:2::232'/>
</graphics>
Yes, I know QEMU does not support listening on multiple addresses
yet, but QEMU really needs to be fixed in that respect. This
shouldn't stop us from structuring the XML nicely now.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
8:23 a.m.
On 07/22/2011 04:37 AM, Daniel P. Berrange wrote:
The 'listenNetwork' attribute would solve the second, but do
nothing to solve the first.
I think we ought to thus use a subelement
<graphics>
<listen type='network' network='foo'/>
</graphics>
<graphics>
<listen type='address' address='192.168.122.122'/>
<listen type='address' address='10.0.0.1'/>
<listen type='address' address='fec0:1:2::232'/>
And instead of listenNetwork, this would be:
<listen type='network' name='netname'/>
</graphics>
Of course, for back-compat reasons, the first <listen> tag with an
address (and not a network name) should _also_ populate the listen=''
attribute of <graphics>. So, just like we did for networks, a single
listen address can be specified in one or both locations on input, but
is shown in both on output, and the new listen-network uses only the new
sub-element style.
I like the idea, and do think it's more flexible, if qemu ever learns to
support multiple interfaces.
Yes, I know QEMU does not support listening on multiple addresses
yet, but QEMU really needs to be fixed in that respect. This
shouldn't stop us from structuring the XML nicely now.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
8:29 a.m.
On 07/22/2011 07:23 AM, Eric Blake wrote:
On 07/22/2011 04:37 AM, Daniel P. Berrange wrote:
> The 'listenNetwork' attribute would solve the second, but do
> nothing to solve the first.
>
> I think we ought to thus use a subelement
>
> <graphics>
> <listen type='network' network='foo'/>
> </graphics>
Oops - I didn't read this...
>
> <graphics>
> <listen type='address' address='192.168.122.122'/>
> <listen type='address' address='10.0.0.1'/>
> <listen type='address' address='fec0:1:2::232'/>
And instead of listenNetwork, this would be:
<listen type='network' name='netname'/>
...and ended up proposing something slightly different :)
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4:39 a.m.
On 07/22/2011 06:37 AM, Daniel P. Berrange wrote:
On Wed, Jul 20, 2011 at 04:11:11AM -0400, Laine Stump wrote:
> This patch series resolves the following bug:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=703851
>
> In short, it implements support for a new "listenNetwork" attribute in
> the domain's<graphics> element. When listenNetwork is specified,
> libvirt will look for a network of that name (from the networks that
> can be created within libvirt), and derive a listen address from the
> physical interface used to connect that network to the outside
> world. (More details are given in each commit message).
I think my main thought on this is whether we are being forward
thinking enough. The problems with the current 'listen' attribute
- You can only list one address
- The address is tied to a specific machine
The 'listenNetwork' attribute would solve the second, but do
nothing to solve the first.
I think we ought to thus use a subelement
<graphics>
<listen type='network' network='foo'/>
</graphics>
<graphics>
<listen type='address' address='192.168.122.122'/>
<listen type='address' address='10.0.0.1'/>
<listen type='address' address='fec0:1:2::232'/>
</graphics>
Yes! I knew there had to be a better way to describe it, and this is it!
I just posted new patches that implement this (I also moved port,
tlsPort, and autoport into the <listen> element, since they are also
associated with listening, and we may want to use a different port for
each address in the future).
The parser and formatter both fully support multiple <listen> elements,
so when qemu is ready for that, we can easily plug them in.
Also, backward compatibility is setup similar to how I did it for the
old single <forward dev='xxx'/> vs. the new <forward> <interface
dev='xxx'> </forward> - When reading, both are accepted, but if both the
old and the new form are present, they must match. On output, both are
output to increase the chances of working "reasonably" with older clients.
Yes, I know QEMU does not support listening on multiple addresses
yet, but QEMU really needs to be fixed in that respect. This
shouldn't stop us from structuring the XML nicely now.
4869
days inactive
4874
days old
18 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Laine Stump