[libvirt] [PATCHv2 0/4] support the listenNetwork attribute in <graphics>
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!
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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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.
(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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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)
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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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.)
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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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).
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 :|
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 07/22/2011 06:37 AM, Daniel P. Berrange 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
On Wed, Jul 20, 2011 at 04:11:11AM -0400, Laine Stump wrote: 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.
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Laine Stump