[libvirt] PATCH 0/4: Openvz bridge support & related patches
This patch series is derived from Anton Protopopov /Evgeniy Sokolov bridge device patches. It first does some generic refactoring of MAC address handler in all drivers, then adds code to extract openvz version number, then does network config, and finally does filesystem config. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
This patch improves the MAC address handling. Currently our XML parser auto-generates a MAC addres using the KVM vendor prefix. This isn't much use for other drivers. This patch addresses this: - Stores each driver's vendor prefix in the capability object - Changes domain parser to use the per-driver vendor prefix for generating mac addresses - Adds more utility methods to util.c for parsing/generating/formatting MAC addresses - Updates each driver to record its vendor prefix for MAC address. NB, for LXC driver we're 'borrowing' KVM's vendor prefix. capabilities.c | 16 +++++++++++++++- capabilities.h | 11 +++++++++++ domain_conf.c | 34 +++++++--------------------------- domain_conf.h | 8 ++------ lxc_conf.c | 3 +++ lxc_driver.c | 4 +++- openvz_conf.c | 2 +- qemu_conf.c | 3 +++ qemu_driver.c | 6 ++++-- util.c | 24 +++++++++++++++++++++++- util.h | 12 +++++++++++- xen_internal.c | 3 +++ xend_internal.c | 8 ++++++-- xm_internal.c | 34 +++++++++++----------------------- 14 files changed, 103 insertions(+), 65 deletions(-) Daniel diff -r c928fca9ce2b src/capabilities.c --- a/src/capabilities.c Tue Oct 14 11:16:52 2008 +0100 +++ b/src/capabilities.c Tue Oct 14 12:24:44 2008 +0100 @@ -26,7 +26,7 @@ #include "capabilities.h" #include "buf.h" #include "memory.h" - +#include "util.h" /** * virCapabilitiesNew: @@ -647,3 +647,17 @@ virCapabilitiesFormatXML(virCapsPtr caps return virBufferContentAndReset(&xml); } + +extern void +virCapabilitiesSetMacPrefix(virCapsPtr caps, + unsigned char *prefix) +{ + memcpy(caps->macPrefix, prefix, sizeof(caps->macPrefix)); +} + +extern void +virCapabilitiesGenerateMac(virCapsPtr caps, + unsigned char *mac) +{ + virGenerateMacAddr(caps->macPrefix, mac); +} diff -r c928fca9ce2b src/capabilities.h --- a/src/capabilities.h Tue Oct 14 11:16:52 2008 +0100 +++ b/src/capabilities.h Tue Oct 14 12:24:13 2008 +0100 @@ -23,6 +23,9 @@ #ifndef __VIR_CAPABILITIES_H #define __VIR_CAPABILITIES_H + +#include "internal.h" +#include "util.h" typedef struct _virCapsGuestFeature virCapsGuestFeature; typedef virCapsGuestFeature *virCapsGuestFeaturePtr; @@ -95,6 +98,7 @@ struct _virCaps { virCapsHost host; int nguests; virCapsGuestPtr *guests; + unsigned char macPrefix[VIR_MAC_PREFIX_BUFLEN]; }; @@ -106,6 +110,13 @@ extern void extern void virCapabilitiesFree(virCapsPtr caps); +extern void +virCapabilitiesSetMacPrefix(virCapsPtr caps, + unsigned char *prefix); + +extern void +virCapabilitiesGenerateMac(virCapsPtr caps, + unsigned char *mac); extern int virCapabilitiesAddHostFeature(virCapsPtr caps, diff -r c928fca9ce2b src/domain_conf.c --- a/src/domain_conf.c Tue Oct 14 11:16:52 2008 +0100 +++ b/src/domain_conf.c Tue Oct 14 12:26:36 2008 +0100 @@ -762,23 +762,14 @@ cleanup: } -static void virDomainNetRandomMAC(virDomainNetDefPtr def) { - /* XXX there different vendor prefixes per hypervisor */ - def->mac[0] = 0x52; - def->mac[1] = 0x54; - def->mac[2] = 0x00; - def->mac[3] = 1 + (int)(256*(rand()/(RAND_MAX+1.0))); - def->mac[4] = 1 + (int)(256*(rand()/(RAND_MAX+1.0))); - def->mac[5] = 1 + (int)(256*(rand()/(RAND_MAX+1.0))); -} - /* Parse the XML definition for a network interface * @param node XML nodeset to parse for net definition * @return 0 on success, -1 on failure */ -virDomainNetDefPtr +static virDomainNetDefPtr virDomainNetDefParseXML(virConnectPtr conn, + virCapsPtr caps, xmlNodePtr node) { virDomainNetDefPtr def; xmlNodePtr cur; @@ -857,22 +848,9 @@ virDomainNetDefParseXML(virConnectPtr co } if (macaddr) { - unsigned int mac[6]; - sscanf((const char *)macaddr, "%02x:%02x:%02x:%02x:%02x:%02x", - (unsigned int*)&mac[0], - (unsigned int*)&mac[1], - (unsigned int*)&mac[2], - (unsigned int*)&mac[3], - (unsigned int*)&mac[4], - (unsigned int*)&mac[5]); - def->mac[0] = mac[0]; - def->mac[1] = mac[1]; - def->mac[2] = mac[2]; - def->mac[3] = mac[3]; - def->mac[4] = mac[4]; - def->mac[5] = mac[5]; + virParseMacAddr((const char *)macaddr, def->mac); } else { - virDomainNetRandomMAC(def); + virCapabilitiesGenerateMac(caps, def->mac); } switch (def->type) { @@ -1630,6 +1608,7 @@ static int virDomainLifecycleParseXML(vi virDomainDeviceDefPtr virDomainDeviceDefParse(virConnectPtr conn, + virCapsPtr caps, const virDomainDefPtr def, const char *xmlStr) { @@ -1666,7 +1645,7 @@ virDomainDeviceDefPtr virDomainDeviceDef goto error; } else if (xmlStrEqual(node->name, BAD_CAST "interface")) { dev->type = VIR_DOMAIN_DEVICE_NET; - if (!(dev->data.net = virDomainNetDefParseXML(conn, node))) + if (!(dev->data.net = virDomainNetDefParseXML(conn, caps, node))) goto error; } else if (xmlStrEqual(node->name, BAD_CAST "input")) { dev->type = VIR_DOMAIN_DEVICE_INPUT; @@ -1988,6 +1967,7 @@ static virDomainDefPtr virDomainDefParse goto no_memory; for (i = 0 ; i < n ; i++) { virDomainNetDefPtr net = virDomainNetDefParseXML(conn, + caps, nodes[i]); if (!net) goto error; diff -r c928fca9ce2b src/domain_conf.h --- a/src/domain_conf.h Tue Oct 14 11:16:52 2008 +0100 +++ b/src/domain_conf.h Tue Oct 14 12:26:56 2008 +0100 @@ -127,14 +127,12 @@ enum virDomainNetType { }; -#define VIR_DOMAIN_NET_MAC_SIZE 6 - /* Stores the virtual network interface configuration */ typedef struct _virDomainNetDef virDomainNetDef; typedef virDomainNetDef *virDomainNetDefPtr; struct _virDomainNetDef { int type; - unsigned char mac[VIR_DOMAIN_NET_MAC_SIZE]; + unsigned char mac[VIR_MAC_BUFLEN]; char *model; union { struct { @@ -513,6 +511,7 @@ void virDomainRemoveInactive(virDomainOb #ifndef PROXY virDomainDeviceDefPtr virDomainDeviceDefParse(virConnectPtr conn, + virCapsPtr caps, const virDomainDefPtr def, const char *xmlStr); virDomainDefPtr virDomainDefParseString(virConnectPtr conn, @@ -573,9 +572,6 @@ int virDiskNameToBusDeviceIndex(virDomai int *busIdx, int *devIdx); -virDomainNetDefPtr virDomainNetDefParseXML(virConnectPtr conn, - xmlNodePtr node); - const char *virDomainDefDefaultEmulator(virConnectPtr conn, virDomainDefPtr def, virCapsPtr caps); diff -r c928fca9ce2b src/lxc_conf.c --- a/src/lxc_conf.c Tue Oct 14 11:16:52 2008 +0100 +++ b/src/lxc_conf.c Tue Oct 14 12:33:39 2008 +0100 @@ -42,6 +42,9 @@ virCapsPtr lxcCapsInit(void) 0, 0)) == NULL) goto no_memory; + /* XXX shouldn't 'borrow' KVM's prefix */ + virCapabilitiesSetMacPrefix(caps, (unsigned char []){ 0x52, 0x54, 0x00 }); + if ((guest = virCapabilitiesAddGuest(caps, "exe", utsname.machine, diff -r c928fca9ce2b src/lxc_driver.c --- a/src/lxc_driver.c Tue Oct 14 11:16:52 2008 +0100 +++ b/src/lxc_driver.c Tue Oct 14 12:34:45 2008 +0100 @@ -1187,6 +1187,7 @@ static int lxcGetSchedulerParameters(vir int rc = 0; virCgroupPtr group; virDomainObjPtr domain; + unsigned long val; if (virCgroupHaveSupport() != 0) return 0; @@ -1208,7 +1209,8 @@ static int lxcGetSchedulerParameters(vir if (rc != 0) return rc; - rc = virCgroupGetCpuShares(group, (unsigned long *)¶ms[0].value.ul); + rc = virCgroupGetCpuShares(group, &val); + params[0].value.ul = val; strncpy(params[0].field, "cpu_shares", sizeof(params[0].field)); params[0].type = VIR_DOMAIN_SCHED_FIELD_ULLONG; diff -r c928fca9ce2b src/openvz_conf.c --- a/src/openvz_conf.c Tue Oct 14 11:16:52 2008 +0100 +++ b/src/openvz_conf.c Tue Oct 14 14:41:29 2008 +0100 @@ -108,7 +108,7 @@ int openvzCheckEmptyMac(const unsigned c int openvzCheckEmptyMac(const unsigned char *mac) { int i; - for (i = 0; i < VIR_DOMAIN_NET_MAC_SIZE; i++) + for (i = 0; i < VIR_MAC_BUFLEN; i++) if (mac[i] != 0x00) return 1; diff -r c928fca9ce2b src/qemu_conf.c --- a/src/qemu_conf.c Tue Oct 14 11:16:52 2008 +0100 +++ b/src/qemu_conf.c Tue Oct 14 12:32:22 2008 +0100 @@ -364,6 +364,9 @@ virCapsPtr qemudCapsInit(void) { if ((caps = virCapabilitiesNew(utsname.machine, 0, 0)) == NULL) goto no_memory; + + /* Using KVM's mac prefix for QEMU too */ + virCapabilitiesSetMacPrefix(caps, (unsigned char[]){ 0x52, 0x54, 0x00 }); if (qemudCapsInitNUMA(caps) < 0) goto no_memory; diff -r c928fca9ce2b src/qemu_driver.c --- a/src/qemu_driver.c Tue Oct 14 11:16:52 2008 +0100 +++ b/src/qemu_driver.c Tue Oct 14 12:33:23 2008 +0100 @@ -2389,7 +2389,7 @@ static int qemudDomainChangeEjectableMed { struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData; virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid); - virDomainDiskDefPtr origdisk, newdisk; + virDomainDiskDefPtr origdisk = NULL, newdisk; char *cmd, *reply, *safe_path; char *devname = NULL; unsigned int qemuCmdFlags; @@ -2631,7 +2631,9 @@ static int qemudDomainAttachDevice(virDo return -1; } - dev = virDomainDeviceDefParse(dom->conn, vm->def, xml); + dev = virDomainDeviceDefParse(dom->conn, + driver->caps, + vm->def, xml); if (dev == NULL) { return -1; } diff -r c928fca9ce2b src/util.c --- a/src/util.c Tue Oct 14 11:16:52 2008 +0100 +++ b/src/util.c Tue Oct 14 12:23:15 2008 +0100 @@ -983,7 +983,7 @@ virParseMacAddr(const char* str, unsigne int i; errno = 0; - for (i = 0; i < 6; i++) { + for (i = 0; i < VIR_MAC_BUFLEN; i++) { char *end_ptr; unsigned long result; @@ -1012,6 +1012,28 @@ virParseMacAddr(const char* str, unsigne return -1; } + +void virFormatMacAddr(const unsigned char *addr, + char *str) +{ + snprintf(str, VIR_MAC_STRING_BUFLEN, + "%02X:%02X:%02X:%02X:%02X:%02X", + addr[0], addr[1], addr[2], + addr[3], addr[4], addr[5]); + str[VIR_MAC_STRING_BUFLEN-1] = '\0'; +} + +void virGenerateMacAddr(const unsigned char *prefix, + unsigned char *addr) +{ + addr[0] = prefix[0]; + addr[1] = prefix[1]; + addr[2] = prefix[2]; + addr[3] = 1 + (int)(256*(rand()/(RAND_MAX+1.0))); + addr[4] = 1 + (int)(256*(rand()/(RAND_MAX+1.0))); + addr[5] = 1 + (int)(256*(rand()/(RAND_MAX+1.0))); +} + int virEnumFromString(const char *const*types, unsigned int ntypes, diff -r c928fca9ce2b src/util.h --- a/src/util.h Tue Oct 14 11:16:52 2008 +0100 +++ b/src/util.h Tue Oct 14 12:21:58 2008 +0100 @@ -1,3 +1,4 @@ + /* * utils.h: common, generic utility functions * @@ -113,7 +114,16 @@ void virSkipSpaces(const char **str); void virSkipSpaces(const char **str); int virParseNumber(const char **str); -int virParseMacAddr(const char* str, unsigned char *addr); +#define VIR_MAC_BUFLEN 6 +#define VIR_MAC_PREFIX_BUFLEN 3 +#define VIR_MAC_STRING_BUFLEN VIR_MAC_BUFLEN * 3 + +int virParseMacAddr(const char* str, + unsigned char *addr); +void virFormatMacAddr(const unsigned char *addr, + char *str); +void virGenerateMacAddr(const unsigned char *prefix, + unsigned char *addr); int virDiskNameToIndex(const char* str); diff -r c928fca9ce2b src/xen_internal.c --- a/src/xen_internal.c Tue Oct 14 11:16:52 2008 +0100 +++ b/src/xen_internal.c Tue Oct 14 12:27:38 2008 +0100 @@ -2132,6 +2132,9 @@ xenHypervisorBuildCapabilities(virConnec if ((caps = virCapabilitiesNew(hostmachine, 1, 1)) == NULL) goto no_memory; + + virCapabilitiesSetMacPrefix(caps, (unsigned char[]){ 0x00, 0x16, 0x3e }); + if (hvm_type && STRNEQ(hvm_type, "") && virCapabilitiesAddHostFeature(caps, hvm_type) < 0) goto no_memory; diff -r c928fca9ce2b src/xend_internal.c --- a/src/xend_internal.c Tue Oct 14 11:16:52 2008 +0100 +++ b/src/xend_internal.c Tue Oct 14 12:29:07 2008 +0100 @@ -3852,7 +3852,9 @@ xenDaemonAttachDevice(virDomainPtr domai NULL))) goto cleanup; - if (!(dev = virDomainDeviceDefParse(domain->conn, def, xml))) + if (!(dev = virDomainDeviceDefParse(domain->conn, + priv->caps, + def, xml))) goto cleanup; @@ -3940,7 +3942,9 @@ xenDaemonDetachDevice(virDomainPtr domai NULL))) goto cleanup; - if (!(dev = virDomainDeviceDefParse(domain->conn, def, xml))) + if (!(dev = virDomainDeviceDefParse(domain->conn, + priv->caps, + def, xml))) goto cleanup; if (virDomainXMLDevID(domain, dev, class, ref, sizeof(ref))) diff -r c928fca9ce2b src/xm_internal.c --- a/src/xm_internal.c Tue Oct 14 11:16:52 2008 +0100 +++ b/src/xm_internal.c Tue Oct 14 12:30:37 2008 +0100 @@ -2422,12 +2422,16 @@ xenXMDomainAttachDevice(virDomainPtr dom int ret = -1; virDomainDeviceDefPtr dev = NULL; virDomainDefPtr def; + xenUnifiedPrivatePtr priv; if ((!domain) || (!domain->conn) || (!domain->name) || (!xml)) { xenXMError((domain ? domain->conn : NULL), VIR_ERR_INVALID_ARG, __FUNCTION__); return -1; } + + priv = (xenUnifiedPrivatePtr) domain->conn->privateData; + if (domain->conn->flags & VIR_CONNECT_RO) return -1; if (domain->id != -1) @@ -2440,6 +2444,7 @@ xenXMDomainAttachDevice(virDomainPtr dom def = entry->def; if (!(dev = virDomainDeviceDefParse(domain->conn, + priv->caps, entry->def, xml))) return -1; @@ -2489,28 +2494,6 @@ xenXMDomainAttachDevice(virDomainPtr dom return ret; } - -/** - * xenXMAutoAssignMac: - * @mac: pointer to Mac String - * - * a mac is assigned automatically. - * - * Returns 0 in case of success, -1 in case of failure. - */ -char * -xenXMAutoAssignMac() { - char *buf; - - if (VIR_ALLOC_N(buf, 18) < 0) - return 0; - srand((unsigned)time(NULL)); - sprintf(buf, "00:16:3e:%02x:%02x:%02x" - ,1 + (int)(256*(rand()/(RAND_MAX+1.0))) - ,1 + (int)(256*(rand()/(RAND_MAX+1.0))) - ,1 + (int)(256*(rand()/(RAND_MAX+1.0)))); - return buf; -} /** * xenXMDomainDetachDevice: @@ -2529,12 +2512,16 @@ xenXMDomainDetachDevice(virDomainPtr dom virDomainDefPtr def; int ret = -1; int i; + xenUnifiedPrivatePtr priv; if ((!domain) || (!domain->conn) || (!domain->name) || (!xml)) { xenXMError((domain ? domain->conn : NULL), VIR_ERR_INVALID_ARG, __FUNCTION__); return -1; } + + priv = (xenUnifiedPrivatePtr) domain->conn->privateData; + if (domain->conn->flags & VIR_CONNECT_RO) return -1; if (domain->id != -1) @@ -2546,6 +2533,7 @@ xenXMDomainDetachDevice(virDomainPtr dom def = entry->def; if (!(dev = virDomainDeviceDefParse(domain->conn, + priv->caps, entry->def, xml))) return -1; @@ -2573,7 +2561,7 @@ xenXMDomainDetachDevice(virDomainPtr dom for (i = 0 ; i < def->nnets ; i++) { if (!memcmp(def->nets[i]->mac, dev->data.net->mac, - VIR_DOMAIN_NET_MAC_SIZE)) { + sizeof(def->nets[i]->mac))) { virDomainNetDefFree(def->nets[i]); if (i < (def->nnets - 1)) memmove(def->nets + i, -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
This patch improves the MAC address handling.
Currently our XML parser auto-generates a MAC addres using the KVM vendor prefix. This isn't much use for other drivers. This patch addresses this:
- Stores each driver's vendor prefix in the capability object - Changes domain parser to use the per-driver vendor prefix for generating mac addresses - Adds more utility methods to util.c for parsing/generating/formatting MAC addresses - Updates each driver to record its vendor prefix for MAC address.
This all looks fine, but I have a question about the moved code that makes up the new virGenerateMacAddr function:
+void virGenerateMacAddr(const unsigned char *prefix, + unsigned char *addr) +{ + addr[0] = prefix[0]; + addr[1] = prefix[1]; + addr[2] = prefix[2]; + addr[3] = 1 + (int)(256*(rand()/(RAND_MAX+1.0))); + addr[4] = 1 + (int)(256*(rand()/(RAND_MAX+1.0))); + addr[5] = 1 + (int)(256*(rand()/(RAND_MAX+1.0))); +}
I presume the intent is to generated numbers in 0..255, but that code generates them in 1..256 and relies on the truncate-to-8-bit-unsigned-char to map back to 0..255. Why are those "1 +" there? It doesn't seem to hurt, but doesn't make sense (to me), either. Removing them would not change the results. Also, unless there's a guarantee that the random number state is initialized elsewhere, it should be initialized here, like it was in the now-removed xenXMAutoAssignMac function. srand((unsigned)time(NULL)); Or maybe just initialize it once at start-up?
On Thu, Oct 16, 2008 at 12:37:36PM +0200, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
This patch improves the MAC address handling.
Currently our XML parser auto-generates a MAC addres using the KVM vendor prefix. This isn't much use for other drivers. This patch addresses this:
- Stores each driver's vendor prefix in the capability object - Changes domain parser to use the per-driver vendor prefix for generating mac addresses - Adds more utility methods to util.c for parsing/generating/formatting MAC addresses - Updates each driver to record its vendor prefix for MAC address.
This all looks fine, but I have a question about the moved code that makes up the new virGenerateMacAddr function:
+void virGenerateMacAddr(const unsigned char *prefix, + unsigned char *addr) +{ + addr[0] = prefix[0]; + addr[1] = prefix[1]; + addr[2] = prefix[2]; + addr[3] = 1 + (int)(256*(rand()/(RAND_MAX+1.0))); + addr[4] = 1 + (int)(256*(rand()/(RAND_MAX+1.0))); + addr[5] = 1 + (int)(256*(rand()/(RAND_MAX+1.0))); +}
I presume the intent is to generated numbers in 0..255, but that code generates them in 1..256 and relies on the truncate-to-8-bit-unsigned-char to map back to 0..255.
Why are those "1 +" there? It doesn't seem to hurt, but doesn't make sense (to me), either. Removing them would not change the results.
This was just moving code from elsewhere, not sure why it was like that in the first place. Seems sensible to just remove the '1 +' bit.
Also, unless there's a guarantee that the random number state is initialized elsewhere, it should be initialized here, like it was in the now-removed xenXMAutoAssignMac function.
srand((unsigned)time(NULL));
Or maybe just initialize it once at start-up?
Could just add a call to srand() in virInitialize() i guess, although is that a reasonable thing for a shared library to be doing, rather than leaving it upto the application ? Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote: ...
Also, unless there's a guarantee that the random number state is initialized elsewhere, it should be initialized here, like it was in the now-removed xenXMAutoAssignMac function.
srand((unsigned)time(NULL));
Or maybe just initialize it once at start-up?
Could just add a call to srand() in virInitialize() i guess, although is that a reasonable thing for a shared library to be doing, rather than leaving it upto the application ?
You're right that it's not reasonable to call srand from library code. One alternative is to use something like coreutils' gnulib-style randint module. I'm looking into whether we can relax it's GPL license to LGPLv2+. Using it or any other library-safe random_r-like interface would mean maintaining and reusing an internal-to-libvirt random state buffer.
On Tue, Oct 14, 2008 at 04:17:57PM +0100, Daniel P. Berrange wrote:
This patch improves the MAC address handling.
Currently our XML parser auto-generates a MAC addres using the KVM vendor prefix. This isn't much use for other drivers. This patch addresses this:
- Stores each driver's vendor prefix in the capability object - Changes domain parser to use the per-driver vendor prefix for generating mac addresses - Adds more utility methods to util.c for parsing/generating/formatting MAC addresses - Updates each driver to record its vendor prefix for MAC address.
NB, for LXC driver we're 'borrowing' KVM's vendor prefix.
Looks fine to me. Jim's raised an interesting point though, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
This patch implements the getVersion driver method for openvz to report the version number of vzctl. This is needed in the next patch to determine if we have builtin support for bridges Daniel diff -r 524f426a413d src/openvz_conf.c --- a/src/openvz_conf.c Tue Oct 14 14:41:47 2008 +0100 +++ b/src/openvz_conf.c Tue Oct 14 15:09:10 2008 +0100 @@ -41,6 +41,7 @@ #include <errno.h> #include <string.h> #include <sys/utsname.h> +#include <sys/wait.h> #include "openvz_conf.h" #include "uuid.h" @@ -63,6 +64,74 @@ strtoI(const char *str) return val; } + + +static int +openvzExtractVersionInfo(const char *cmd, int *retversion) +{ + const char *const vzarg[] = { cmd, "--help", NULL }; + const char *const vzenv[] = { "LC_ALL=C", NULL }; + pid_t child; + int newstdout = -1; + int ret = -1, status; + unsigned int major, minor, micro; + unsigned int version; + + if (retversion) + *retversion = 0; + + if (virExec(NULL, vzarg, vzenv, NULL, + &child, -1, &newstdout, NULL, VIR_EXEC_NONE) < 0) + return -1; + + char *help = NULL; + enum { MAX_HELP_OUTPUT_SIZE = 8192 }; + int len = virFileReadLimFD(newstdout, MAX_HELP_OUTPUT_SIZE, &help); + if (len < 0) + goto cleanup2; + + if (sscanf(help, "vzctl version %u.%u.%u", + &major, &minor, µ) != 3) { + goto cleanup2; + } + + version = (major * 1000 * 1000) + (minor * 1000) + micro; + + if (retversion) + *retversion = version; + + ret = 0; + +cleanup2: + VIR_FREE(help); + if (close(newstdout) < 0) + ret = -1; + +rewait: + if (waitpid(child, &status, 0) != child) { + if (errno == EINTR) + goto rewait; + ret = -1; + } + + return ret; +} + +int openvzExtractVersion(virConnectPtr conn, + struct openvz_driver *driver) +{ + if (driver->version > 0) + return 0; + + if (openvzExtractVersionInfo(VZCTL, &driver->version) < 0) { + openvzError(conn, VIR_ERR_INTERNAL_ERROR, + "%s", _("Cound not extract vzctl version")); + return -1; + } + + return 0; +} + virCapsPtr openvzCapsInit(void) { diff -r 524f426a413d src/openvz_conf.h --- a/src/openvz_conf.h Tue Oct 14 14:41:47 2008 +0100 +++ b/src/openvz_conf.h Tue Oct 14 15:10:48 2008 +0100 @@ -47,15 +47,18 @@ enum { OPENVZ_WARN, OPENVZ_ERR }; /* OpenVZ commands - Replace with wrapper scripts later? */ -#define VZLIST "vzlist" -#define VZCTL "vzctl" +#define VZLIST "/usr/sbin/vzlist" +#define VZCTL "/usr/sbin/vzctl" struct openvz_driver { virCapsPtr caps; virDomainObjList domains; + int version; }; int openvz_readline(int fd, char *ptr, int maxlen); +int openvzExtractVersion(virConnectPtr conn, + struct openvz_driver *driver); int openvzReadConfigParam(int vpsid ,const char * param, char *value, int maxlen); virCapsPtr openvzCapsInit(void); int openvzLoadDomains(struct openvz_driver *driver); diff -r 524f426a413d src/openvz_driver.c --- a/src/openvz_driver.c Tue Oct 14 14:41:47 2008 +0100 +++ b/src/openvz_driver.c Tue Oct 14 15:11:56 2008 +0100 @@ -172,6 +172,12 @@ static virDomainPtr openvzDomainLookupBy return dom; } +static int openvzGetVersion(virConnectPtr conn, unsigned long *version) { + struct openvz_driver *driver = (struct openvz_driver *)conn->privateData; + *version = driver->version; + return 0; +} + static char *openvzGetOSType(virDomainPtr dom) { struct openvz_driver *driver = (struct openvz_driver *)dom->conn->privateData; @@ -777,6 +783,9 @@ static virDrvOpenStatus openvzOpen(virCo if (openvzLoadDomains(driver) < 0) goto cleanup; + if (openvzExtractVersion(conn, driver) < 0) + goto cleanup; + conn->privateData = driver; return VIR_DRV_OPEN_SUCCESS; @@ -961,7 +970,7 @@ static virDriver openvzDriver = { openvzClose, /* close */ NULL, /* supports_feature */ openvzGetType, /* type */ - NULL, /* version */ + openvzGetVersion, /* version */ NULL, /* hostname */ NULL, /* uri */ openvzGetMaxVCPUs, /* getMaxVcpus */ -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
This patch implements the getVersion driver method for openvz to report the version number of vzctl. This is needed in the next patch to determine if we have builtin support for bridges ... diff -r 524f426a413d src/openvz_conf.c
ACK
On Tue, Oct 14, 2008 at 04:19:06PM +0100, Daniel P. Berrange wrote:
This patch implements the getVersion driver method for openvz to report the version number of vzctl. This is needed in the next patch to determine if we have builtin support for bridges
Looks fine except:
+static int +openvzExtractVersionInfo(const char *cmd, int *retversion) +{ [...] + enum { MAX_HELP_OUTPUT_SIZE = 8192 }; + int len = virFileReadLimFD(newstdout, MAX_HELP_OUTPUT_SIZE, &help);
Except that part which puzzles me. Why defining an enum for that. Also enum is one of the worse part of C, it has no clear storage size definition, and virFileReadLimFD takes an input int anyway... Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Mon, Oct 20, 2008 at 11:33:20AM +0200, Daniel Veillard wrote:
On Tue, Oct 14, 2008 at 04:19:06PM +0100, Daniel P. Berrange wrote:
This patch implements the getVersion driver method for openvz to report the version number of vzctl. This is needed in the next patch to determine if we have builtin support for bridges
Looks fine except:
+static int +openvzExtractVersionInfo(const char *cmd, int *retversion) +{ [...] + enum { MAX_HELP_OUTPUT_SIZE = 8192 }; + int len = virFileReadLimFD(newstdout, MAX_HELP_OUTPUT_SIZE, &help);
Except that part which puzzles me. Why defining an enum for that. Also enum is one of the worse part of C, it has no clear storage size definition, and virFileReadLimFD takes an input int anyway...
There's no particular reason for this - I just copied the equivalent code in the QEMU driver for extracting version info & this was the way it worked. This constant is only used once, so its not really doing much of use. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
This implements support for bridge configs in openvz following the rules set out in http://wiki.openvz.org/Virtual_Ethernet_device#Making_a_bridged_veth-device_... This simply requires that the admin has created /etc/vz/vznetctl.conf containing #!/bin/bash EXTERNAL_SCRIPT="/usr/sbin/vznetaddbr" For openvz <= 3.0.22, we have to manually re-write the NETIF line to add the bridge config parameter. For newer openvz we can simply pass the bridge name on the commnand line to --netif_add. Older openvz also requires that the admin install /usr/sbin/vznetaddbr since it is not available out of the box Daniel diff -r 2e218ae09a5d src/openvz_conf.c --- a/src/openvz_conf.c Tue Oct 14 15:14:35 2008 +0100 +++ b/src/openvz_conf.c Tue Oct 14 15:42:02 2008 +0100 @@ -51,7 +51,7 @@ static char *openvzLocateConfDir(void); static int openvzGetVPSUUID(int vpsid, char *uuidstr); -static int openvzLocateConfFile(int vpsid, char *conffile, int maxlen); +static int openvzLocateConfFile(int vpsid, char *conffile, int maxlen, const char *ext); static int openvzAssignUUIDs(void); int @@ -145,6 +145,8 @@ virCapsPtr openvzCapsInit(void) 0, 0)) == NULL) goto no_memory; + virCapabilitiesSetMacPrefix(caps, (unsigned char[]){ 0x52, 0x54, 0x00 }); + if ((guest = virCapabilitiesAddGuest(caps, "exe", utsname.machine, @@ -169,54 +171,6 @@ no_memory: return NULL; } - -/* function checks MAC address is empty - return 0 - empty - 1 - not -*/ -int openvzCheckEmptyMac(const unsigned char *mac) -{ - int i; - for (i = 0; i < VIR_MAC_BUFLEN; i++) - if (mac[i] != 0x00) - return 1; - - return 0; -} - -/* convert mac address to string - return pointer to string or NULL -*/ -char *openvzMacToString(const unsigned char *mac) -{ - char str[20]; - if (snprintf(str, 18, "%02X:%02X:%02X:%02X:%02X:%02X", - mac[0], mac[1], mac[2], - mac[3], mac[4], mac[5]) >= 18) - return NULL; - - return strdup(str); -} - -/*parse MAC from view: 00:18:51:8F:D9:F3 - return -1 - error - 0 - OK -*/ -static int openvzParseMac(const char *macaddr, unsigned char *mac) -{ - int ret; - ret = sscanf((const char *)macaddr, "%02X:%02X:%02X:%02X:%02X:%02X", - (unsigned int*)&mac[0], - (unsigned int*)&mac[1], - (unsigned int*)&mac[2], - (unsigned int*)&mac[3], - (unsigned int*)&mac[4], - (unsigned int*)&mac[5]) ; - if (ret == 6) - return 0; - - return -1; -} static int openvzReadNetworkConf(virConnectPtr conn, @@ -288,6 +242,9 @@ openvzReadNetworkConf(virConnectPtr conn while (*next != '\0' && *next != ',') next++; if (STRPREFIX(p, "ifname=")) { p += 7; + /* skip in libvirt */ + } else if (STRPREFIX(p, "host_ifname=")) { + p += 12; len = next - p; if (len > 16) { openvzError(conn, VIR_ERR_INTERNAL_ERROR, @@ -295,14 +252,25 @@ openvzReadNetworkConf(virConnectPtr conn goto error; } + if (VIR_ALLOC_N(net->ifname, len+1) < 0) + goto no_memory; + + strncpy(net->ifname, p, len); + net->ifname[len] = '\0'; + } else if (STRPREFIX(p, "bridge=")) { + p += 7; + len = next - p; + if (len > 16) { + openvzError(conn, VIR_ERR_INTERNAL_ERROR, + "%s", _("Too long bridge device name")); + goto error; + } + if (VIR_ALLOC_N(net->data.bridge.brname, len+1) < 0) goto no_memory; strncpy(net->data.bridge.brname, p, len); net->data.bridge.brname[len] = '\0'; - } else if (STRPREFIX(p, "host_ifname=")) { - p += 12; - //skip in libvirt } else if (STRPREFIX(p, "mac=")) { p += 4; len = next - p; @@ -313,14 +281,11 @@ openvzReadNetworkConf(virConnectPtr conn } strncpy(cpy_temp, p, len); cpy_temp[len] = '\0'; - if (openvzParseMac(cpy_temp, net->mac)<0) { + if (virParseMacAddr(cpy_temp, net->mac) < 0) { openvzError(conn, VIR_ERR_INTERNAL_ERROR, "%s", _("Wrong MAC address")); goto error; } - } else if (STRPREFIX(p, "host_mac=")) { - p += 9; - //skip in libvirt } p = ++next; } while (p < token + strlen(token)); @@ -450,6 +415,71 @@ int openvzLoadDomains(struct openvz_driv return -1; } + +int +openvzWriteConfigParam(int vpsid, const char *param, const char *value) +{ + char conf_file[PATH_MAX]; + char temp_file[PATH_MAX]; + char line[PATH_MAX] ; + int fd, temp_fd; + + if (openvzLocateConfFile(vpsid, conf_file, PATH_MAX, "conf")<0) + return -1; + if (openvzLocateConfFile(vpsid, temp_file, PATH_MAX, "tmp")<0) + return -1; + + fd = open(conf_file, O_RDONLY); + if (fd == -1) + return -1; + temp_fd = open(temp_file, O_WRONLY | O_CREAT | O_TRUNC, 0644); + if (temp_fd == -1) { + close(fd); + return -1; + } + + while(1) { + if (openvz_readline(fd, line, sizeof(line)) <= 0) + break; + + if (!STRPREFIX(line, param)) { + if (safewrite(temp_fd, line, strlen(line)) != + strlen(line)) + goto error; + } + } + + if (safewrite(temp_fd, param, strlen(param)) != + strlen(param)) + goto error; + if (safewrite(temp_fd, "=\"", 2) != 2) + goto error; + if (safewrite(temp_fd, value, strlen(value)) != + strlen(value)) + goto error; + if (safewrite(temp_fd, "\"\n", 2) != 2) + goto error; + + close(fd); + close(temp_fd); + fd = temp_fd = -1; + + if (rename(temp_file, conf_file) < 0) + goto error; + + return 0; + +error: + fprintf(stderr, "damn %s\n", strerror(errno)); + + if (fd != -1) + close(fd); + if (temp_fd != -1) + close(temp_fd); + unlink(temp_file); + return -1; +} + /* * Read parameter from container config * sample: 133, "OSTEMPLATE", value, 1024 @@ -467,7 +497,7 @@ openvzReadConfigParam(int vpsid ,const c char * sf, * token; char *saveptr = NULL; - if (openvzLocateConfFile(vpsid, conf_file, PATH_MAX)<0) + if (openvzLocateConfFile(vpsid, conf_file, PATH_MAX, "conf")<0) return -1; value[0] = 0; @@ -507,7 +537,7 @@ openvzReadConfigParam(int vpsid ,const c * 0 - OK */ static int -openvzLocateConfFile(int vpsid, char *conffile, int maxlen) +openvzLocateConfFile(int vpsid, char *conffile, int maxlen, const char *ext) { char * confdir; int ret = 0; @@ -516,7 +546,8 @@ openvzLocateConfFile(int vpsid, char *co if (confdir == NULL) return -1; - if (snprintf(conffile, maxlen, "%s/%d.conf", confdir, vpsid) >= maxlen) + if (snprintf(conffile, maxlen, "%s/%d.%s", + confdir, vpsid, ext ? ext : "conf") >= maxlen) ret = -1; VIR_FREE(confdir); @@ -573,7 +604,7 @@ openvzGetVPSUUID(int vpsid, char *uuidst char iden[1024]; int fd, ret; - if (openvzLocateConfFile(vpsid, conf_file, PATH_MAX)<0) + if (openvzLocateConfFile(vpsid, conf_file, PATH_MAX, "conf")<0) return -1; fd = open(conf_file, O_RDONLY); @@ -613,7 +644,7 @@ openvzSetDefinedUUID(int vpsid, unsigned if (uuid == NULL) return -1; - if (openvzLocateConfFile(vpsid, conf_file, PATH_MAX)<0) + if (openvzLocateConfFile(vpsid, conf_file, PATH_MAX, "conf")<0) return -1; if (openvzGetVPSUUID(vpsid, uuidstr)) @@ -681,4 +712,3 @@ static int openvzAssignUUIDs(void) VIR_FREE(conf_dir); return 0; } - diff -r 2e218ae09a5d src/openvz_conf.h --- a/src/openvz_conf.h Tue Oct 14 15:14:35 2008 +0100 +++ b/src/openvz_conf.h Tue Oct 14 15:17:02 2008 +0100 @@ -50,6 +50,8 @@ enum { OPENVZ_WARN, OPENVZ_ERR }; #define VZLIST "/usr/sbin/vzlist" #define VZCTL "/usr/sbin/vzctl" +#define VZCTL_BRIDGE_MIN_VERSION ((3 * 1000 * 1000) + (0 * 1000) + 22 + 1) + struct openvz_driver { virCapsPtr caps; virDomainObjList domains; @@ -60,12 +62,11 @@ int openvzExtractVersion(virConnectPtr c int openvzExtractVersion(virConnectPtr conn, struct openvz_driver *driver); int openvzReadConfigParam(int vpsid ,const char * param, char *value, int maxlen); +int openvzWriteConfigParam(int vpsid, const char *param, const char *value); virCapsPtr openvzCapsInit(void); int openvzLoadDomains(struct openvz_driver *driver); void openvzFreeDriver(struct openvz_driver *driver); int strtoI(const char *str); -int openvzCheckEmptyMac(const unsigned char *mac); -char *openvzMacToString(const unsigned char *mac); int openvzSetDefinedUUID(int vpsid, unsigned char *uuid); #endif /* OPENVZ_CONF_H */ diff -r 2e218ae09a5d src/openvz_driver.c --- a/src/openvz_driver.c Tue Oct 14 15:14:35 2008 +0100 +++ b/src/openvz_driver.c Tue Oct 14 15:43:36 2008 +0100 @@ -55,6 +55,7 @@ #include "openvz_conf.h" #include "nodeinfo.h" #include "memory.h" +#include "bridge.h" #define OPENVZ_MAX_ARG 28 #define CMDBUF_LEN 1488 @@ -329,13 +330,55 @@ static int openvzDomainReboot(virDomainP return 0; } +static char * +openvzGenerateVethName(int veid, char *dev_name_ve) +{ + char dev_name[32]; + int ifNo = 0; + + if (sscanf(dev_name_ve, "%*[^0-9]%d", &ifNo) != 1) + return NULL; + if (snprintf(dev_name, sizeof(dev_name), "veth%d.%d", veid, ifNo) < 7) + return NULL; + return strdup(dev_name); +} + +static char * +openvzGenerateContainerVethName(int veid) +{ + int ret; + char temp[1024]; + + /* try to get line "^NETIF=..." from config */ + if ( (ret = openvzReadConfigParam(veid, "NETIF", temp, sizeof(temp))) <= 0) { + snprintf(temp, sizeof(temp), "eth0"); + } else { + char *s; + int max = 0; + + /* get maximum interface number (actually, it is the last one) */ + for (s=strtok(temp, ";"); s; s=strtok(NULL, ";")) { + int x; + + if (sscanf(s, "ifname=eth%d", &x) != 1) return NULL; + if (x > max) max = x; + } + + /* set new name */ + snprintf(temp, sizeof(temp), "eth%d", max+1); + } + return strdup(temp); +} + static int openvzDomainSetNetwork(virConnectPtr conn, const char *vpsid, - virDomainNetDefPtr net) + virDomainNetDefPtr net, + virBufferPtr configBuf) { int rc = 0, narg; const char *prog[OPENVZ_MAX_ARG]; - char *mac = NULL; + char macaddr[VIR_MAC_STRING_BUFLEN]; + struct openvz_driver *driver = (struct openvz_driver *) conn->privateData; #define ADD_ARG_LIT(thisarg) \ do { \ @@ -367,21 +410,61 @@ openvzDomainSetNetwork(virConnectPtr con ADD_ARG_LIT(vpsid); } - if (openvzCheckEmptyMac(net->mac) > 0) - mac = openvzMacToString(net->mac); - - if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE && - net->data.bridge.brname != NULL) { - char opt[1024]; + virFormatMacAddr(net->mac, macaddr); + + if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE) { + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *opt; + char *dev_name_ve; + int veid = strtoI(vpsid); + //--netif_add ifname[,mac,host_ifname,host_mac] ADD_ARG_LIT("--netif_add") ; - strncpy(opt, net->data.bridge.brname, 256); - if (mac != NULL) { - strcat(opt, ","); - strcat(opt, mac); - } + + /* generate interface name in ve and copy it to options */ + dev_name_ve = openvzGenerateContainerVethName(veid); + if (dev_name_ve == NULL) { + openvzError(conn, VIR_ERR_INTERNAL_ERROR, + _("Could not generate eth name for container")); + rc = -1; + goto exit; + } + + /* if user doesn't specified host interface name, + * than we need to generate it */ + if (net->ifname == NULL) { + net->ifname = openvzGenerateVethName(veid, dev_name_ve); + if (net->ifname == NULL) { + openvzError(conn, VIR_ERR_INTERNAL_ERROR, + _("Could not generate veth name")); + rc = -1; + VIR_FREE(dev_name_ve); + goto exit; + } + } + + virBufferAdd(&buf, dev_name_ve, -1); /* Guest dev */ + virBufferVSprintf(&buf, ",%s", macaddr); /* Guest dev mac */ + virBufferVSprintf(&buf, ",%s", net->ifname); /* Host dev */ + virBufferVSprintf(&buf, ",%s", macaddr); /* Host dev mac */ + + if (driver->version >= VZCTL_BRIDGE_MIN_VERSION) { + virBufferVSprintf(&buf, ",%s", net->data.bridge.brname); /* Host bridge */ + } else { + virBufferVSprintf(configBuf, "ifname=%s", dev_name_ve); + virBufferVSprintf(configBuf, ",mac=%s", macaddr); /* Guest dev mac */ + virBufferVSprintf(configBuf, ",host_ifname=%s", net->ifname); /* Host dev */ + virBufferVSprintf(configBuf, ",host_mac=%s", macaddr); /* Host dev mac */ + virBufferVSprintf(configBuf, ",bridge=%s", net->data.bridge.brname); /* Host bridge */ + } + + VIR_FREE(dev_name_ve); + + if (!(opt = virBufferContentAndReset(&buf))) + goto no_memory; + ADD_ARG_LIT(opt) ; - }else if (net->type == VIR_DOMAIN_NET_TYPE_ETHERNET && + } else if (net->type == VIR_DOMAIN_NET_TYPE_ETHERNET && net->data.ethernet.ipaddr != NULL) { //--ipadd ip ADD_ARG_LIT("--ipadd") ; @@ -402,18 +485,57 @@ openvzDomainSetNetwork(virConnectPtr con exit: cmdExecFree(prog); - VIR_FREE(mac); return rc; no_memory: openvzError(conn, VIR_ERR_INTERNAL_ERROR, _("Could not put argument to %s"), VZCTL); cmdExecFree(prog); - VIR_FREE(mac); return -1; #undef ADD_ARG_LIT } + + +static int +openvzDomainSetNetworkConfig(virConnectPtr conn, + virDomainDefPtr def) +{ + unsigned int i; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *param; + struct openvz_driver *driver = (struct openvz_driver *) conn->privateData; + + for (i = 0 ; i < def->nnets ; i++) { + if (driver->version < VZCTL_BRIDGE_MIN_VERSION && i > 0) + virBufferAddLit(&buf, ";"); + + if (openvzDomainSetNetwork(conn, def->name, def->nets[i], &buf) < 0) { + openvzError(conn, VIR_ERR_INTERNAL_ERROR, + "%s", _("Could not configure network")); + goto exit; + } + } + + param = virBufferContentAndReset(&buf); + if (driver->version < VZCTL_BRIDGE_MIN_VERSION && def->nnets) { + if (openvzWriteConfigParam(strtoI(def->name), "NETIF", param) < 0) { + VIR_FREE(param); + openvzError(conn, VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot replace NETIF config")); + return -1; + } + } + + VIR_FREE(param); + return 0; + +exit: + param = virBufferContentAndReset(&buf); + VIR_FREE(param); + return -1; +} + static virDomainPtr openvzDomainDefineXML(virConnectPtr conn, const char *xml) @@ -422,7 +544,6 @@ openvzDomainDefineXML(virConnectPtr conn virDomainDefPtr vmdef = NULL; virDomainObjPtr vm = NULL; virDomainPtr dom = NULL; - int i; const char *prog[OPENVZ_MAX_ARG]; prog[0] = NULL; @@ -468,17 +589,12 @@ openvzDomainDefineXML(virConnectPtr conn goto exit; } + if (openvzDomainSetNetworkConfig(conn, vmdef) < 0) + goto exit; + dom = virGetDomain(conn, vm->def->name, vm->def->uuid); if (dom) dom->id = -1; - - for (i = 0 ; i < vmdef->nnets ; i++) { - if (openvzDomainSetNetwork(conn, vmdef->name, vmdef->nets[i]) < 0) { - openvzError(conn, VIR_ERR_INTERNAL_ERROR, - "%s", _("Could not configure network")); - goto exit; - } - } if (vmdef->vcpus > 0) { if (openvzDomainSetVcpus(dom, vmdef->vcpus) < 0) { @@ -500,7 +616,6 @@ openvzDomainCreateXML(virConnectPtr conn virDomainDefPtr vmdef = NULL; virDomainObjPtr vm = NULL; virDomainPtr dom = NULL; - int i; struct openvz_driver *driver = (struct openvz_driver *) conn->privateData; const char *progstart[] = {VZCTL, "--quiet", "start", NULL, NULL}; const char *progcreate[OPENVZ_MAX_ARG]; @@ -546,13 +661,8 @@ openvzDomainCreateXML(virConnectPtr conn goto exit; } - for (i = 0 ; i < vmdef->nnets ; i++) { - if (openvzDomainSetNetwork(conn, vmdef->name, vmdef->nets[i]) < 0) { - openvzError(conn, VIR_ERR_INTERNAL_ERROR, - "%s", _("Could not configure network")); - goto exit; - } - } + if (openvzDomainSetNetworkConfig(conn, vmdef) < 0) + goto exit; progstart[3] = vmdef->name; -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
This implements support for bridge configs in openvz following the rules set out in
http://wiki.openvz.org/Virtual_Ethernet_device#Making_a_bridged_veth-device_...
This simply requires that the admin has created /etc/vz/vznetctl.conf containing
#!/bin/bash EXTERNAL_SCRIPT="/usr/sbin/vznetaddbr"
For openvz <= 3.0.22, we have to manually re-write the NETIF line to add the bridge config parameter. It is alternative, but more flexible way. It require simple modification of vznetaddbr. Common scenario is to add VZHOSTBR="<bridge if>" to config. For newer openvz we can simply pass the bridge name on the commnand line to --netif_add.
Older openvz also requires that the admin install /usr/sbin/vznetaddbr since it is not available out of the box
Daniel
+ + while(1) { + if (openvz_readline(fd, line, sizeof(line)) <= 0) + break; + + if (!STRPREFIX(line, param)) { need to check for '='. Currently, if you will search for 'NETIF', you can find any string with such prefix. example 'NETIFOTHERPARAM' + if (safewrite(temp_fd, line, strlen(line)) != + strlen(line)) + goto error; + } + } + /*
+ + if (!(opt = virBufferContentAndReset(&buf))) + goto no_memory; + ADD_ARG_LIT(opt) ; Need to free opt - }else if (net->type == VIR_DOMAIN_NET_TYPE_ETHERNET && + } else if (net->type == VIR_DOMAIN_NET_TYPE_ETHERNET && net->data.ethernet.ipaddr != NULL) {
+static int +openvzDomainSetNetworkConfig(virConnectPtr conn, + virDomainDefPtr def) +{ + unsigned int i; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *param; + struct openvz_driver *driver = (struct openvz_driver *) conn->privateData; + + for (i = 0 ; i < def->nnets ; i++) { + if (driver->version < VZCTL_BRIDGE_MIN_VERSION && i > 0) + virBufferAddLit(&buf, ";"); Need to check that network is bridge. In other case we will have NETIF='<some params>;;;;' + + if (openvzDomainSetNetwork(conn, def->name, def->nets[i], &buf) < 0) { + openvzError(conn, VIR_ERR_INTERNAL_ERROR, + "%s", _("Could not configure network")); + goto exit; + } + } +
On Thu, Oct 16, 2008 at 07:52:42PM +0400, Evgeniy Sokolov wrote:
This implements support for bridge configs in openvz following the rules set out in
http://wiki.openvz.org/Virtual_Ethernet_device#Making_a_bridged_veth-device_...
This simply requires that the admin has created /etc/vz/vznetctl.conf containing
#!/bin/bash EXTERNAL_SCRIPT="/usr/sbin/vznetaddbr"
For openvz <= 3.0.22, we have to manually re-write the NETIF line to add the bridge config parameter. It is alternative, but more flexible way. It require simple modification of vznetaddbr. Common scenario is to add VZHOSTBR="<bridge if>" to config. For newer openvz we can simply pass the bridge name on the commnand line to --netif_add.
Older openvz also requires that the admin install /usr/sbin/vznetaddbr since it is not available out of the box
Daniel
+ + while(1) { + if (openvz_readline(fd, line, sizeof(line)) <= 0) + break; + + if (!STRPREFIX(line, param)) { need to check for '='. Currently, if you will search for 'NETIF', you can find any string with such prefix. example 'NETIFOTHERPARAM'
Ahh, good point, will fix that.
+ if (safewrite(temp_fd, line, strlen(line)) != + strlen(line)) + goto error; + } + } + /*
+ + if (!(opt = virBufferContentAndReset(&buf))) + goto no_memory; + ADD_ARG_LIT(opt) ; Need to free opt
Yes.
- }else if (net->type == VIR_DOMAIN_NET_TYPE_ETHERNET && + } else if (net->type == VIR_DOMAIN_NET_TYPE_ETHERNET && net->data.ethernet.ipaddr != NULL) {
+static int +openvzDomainSetNetworkConfig(virConnectPtr conn, + virDomainDefPtr def) +{ + unsigned int i; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *param; + struct openvz_driver *driver = (struct openvz_driver *) conn->privateData; + + for (i = 0 ; i < def->nnets ; i++) { + if (driver->version < VZCTL_BRIDGE_MIN_VERSION && i > 0) + virBufferAddLit(&buf, ";"); Need to check that network is bridge. In other case we will have NETIF='<some params>;;;;'
Opps, completely forgot that check
+ + if (openvzDomainSetNetwork(conn, def->name, def->nets[i], &buf) < 0) { + openvzError(conn, VIR_ERR_INTERNAL_ERROR, + "%s", _("Could not configure network")); + goto exit; + } + } +
Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
This implements support for bridge configs in openvz following the rules set out in
http://wiki.openvz.org/Virtual_Ethernet_device#Making_a_bridged_veth-device_...
This simply requires that the admin has created /etc/vz/vznetctl.conf containing
#!/bin/bash EXTERNAL_SCRIPT="/usr/sbin/vznetaddbr"
For openvz <= 3.0.22, we have to manually re-write the NETIF line to add the bridge config parameter. For newer openvz we can simply pass the bridge name on the commnand line to --netif_add.
Older openvz also requires that the admin install /usr/sbin/vznetaddbr since it is not available out of the box
Hi Dan, Looks fine on principle. A couple suggestions below. ...
+int +openvzWriteConfigParam(int vpsid, const char *param, const char *value) +{ + char conf_file[PATH_MAX]; + char temp_file[PATH_MAX]; + char line[PATH_MAX] ; + int fd, temp_fd; + + if (openvzLocateConfFile(vpsid, conf_file, PATH_MAX, "conf")<0) + return -1; + if (openvzLocateConfFile(vpsid, temp_file, PATH_MAX, "tmp")<0) + return -1; + + fd = open(conf_file, O_RDONLY); + if (fd == -1) + return -1; + temp_fd = open(temp_file, O_WRONLY | O_CREAT | O_TRUNC, 0644); + if (temp_fd == -1) { + close(fd); + return -1; + } + + while(1) { + if (openvz_readline(fd, line, sizeof(line)) <= 0) + break; + + if (!STRPREFIX(line, param)) { + if (safewrite(temp_fd, line, strlen(line)) != + strlen(line)) + goto error; + } + } + + if (safewrite(temp_fd, param, strlen(param)) != + strlen(param)) + goto error; + if (safewrite(temp_fd, "=\"", 2) != 2) + goto error; + if (safewrite(temp_fd, value, strlen(value)) != + strlen(value)) + goto error; + if (safewrite(temp_fd, "\"\n", 2) != 2) + goto error;
How about this instead, so the reader doesn't have to manually count string lengths and verify that the "VAR" in strlen(VAR) on the RHS matches the one on the LHS: if (safewrite(temp_fd, param, strlen(param)) < 0 || safewrite(temp_fd, "=\"", 2) < 0 || safewrite(temp_fd, value, strlen(value)) < 0 || safewrite(temp_fd, "\"\n", 2) < 0) goto error; That works because safewrite always returns negative upon failure. By the way, in ensuring that, I noticed that both safewrite and saferead have this dead code: if (r == 0) return nread; if (r == 0) return nwritten; that's because read and write never return 0 when they've been told to read or write N>0 bytes.
+ close(fd); + close(temp_fd);
Officially, you should always check for failure when you've opened the file descriptor for writing.
+ fd = temp_fd = -1; + + if (rename(temp_file, conf_file) < 0) + goto error; + + return 0; + +error: + fprintf(stderr, "damn %s\n", strerror(errno));
How about mentioning the file name, too: fprintf(stderr, "failed to write %s: %s\n", conf_file, strerror(errno));
+ if (fd != -1) + close(fd); + if (temp_fd != -1) + close(temp_fd); + unlink(temp_file); + return -1; +} ... diff -r 2e218ae09a5d src/openvz_driver.c --- a/src/openvz_driver.c Tue Oct 14 15:14:35 2008 +0100 +++ b/src/openvz_driver.c Tue Oct 14 15:43:36 2008 +0100 @@ -55,6 +55,7 @@ #include "openvz_conf.h" #include "nodeinfo.h" #include "memory.h" +#include "bridge.h"
#define OPENVZ_MAX_ARG 28 #define CMDBUF_LEN 1488 @@ -329,13 +330,55 @@ static int openvzDomainReboot(virDomainP return 0; }
+static char * +openvzGenerateVethName(int veid, char *dev_name_ve) +{ + char dev_name[32]; + int ifNo = 0; + + if (sscanf(dev_name_ve, "%*[^0-9]%d", &ifNo) != 1) + return NULL; + if (snprintf(dev_name, sizeof(dev_name), "veth%d.%d", veid, ifNo) < 7) + return NULL; + return strdup(dev_name); +} + +static char * +openvzGenerateContainerVethName(int veid) +{ + int ret; + char temp[1024]; + + /* try to get line "^NETIF=..." from config */ + if ( (ret = openvzReadConfigParam(veid, "NETIF", temp, sizeof(temp))) <= 0) { + snprintf(temp, sizeof(temp), "eth0"); + } else { + char *s; + int max = 0; + + /* get maximum interface number (actually, it is the last one) */ + for (s=strtok(temp, ";"); s; s=strtok(NULL, ";")) { + int x; + + if (sscanf(s, "ifname=eth%d", &x) != 1) return NULL; + if (x > max) max = x;
More consistent and imho, readable to put newline after each ")": if (sscanf(s, "ifname=eth%d", &x) != 1) return NULL; if (x > max) max = x;
+ } + + /* set new name */ + snprintf(temp, sizeof(temp), "eth%d", max+1); + } + return strdup(temp); +} + static int openvzDomainSetNetwork(virConnectPtr conn, const char *vpsid, - virDomainNetDefPtr net) + virDomainNetDefPtr net, + virBufferPtr configBuf) { int rc = 0, narg; const char *prog[OPENVZ_MAX_ARG]; - char *mac = NULL; + char macaddr[VIR_MAC_STRING_BUFLEN]; + struct openvz_driver *driver = (struct openvz_driver *) conn->privateData;
#define ADD_ARG_LIT(thisarg) \ do { \ @@ -367,21 +410,61 @@ openvzDomainSetNetwork(virConnectPtr con ADD_ARG_LIT(vpsid); }
- if (openvzCheckEmptyMac(net->mac) > 0) - mac = openvzMacToString(net->mac); - - if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE && - net->data.bridge.brname != NULL) { - char opt[1024]; + virFormatMacAddr(net->mac, macaddr); + + if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE) { + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *opt; + char *dev_name_ve; + int veid = strtoI(vpsid); + //--netif_add ifname[,mac,host_ifname,host_mac] ADD_ARG_LIT("--netif_add") ; - strncpy(opt, net->data.bridge.brname, 256); - if (mac != NULL) { - strcat(opt, ","); - strcat(opt, mac); - } + + /* generate interface name in ve and copy it to options */ + dev_name_ve = openvzGenerateContainerVethName(veid); + if (dev_name_ve == NULL) { + openvzError(conn, VIR_ERR_INTERNAL_ERROR, + _("Could not generate eth name for container")); + rc = -1; + goto exit; + } + + /* if user doesn't specified host interface name, + * than we need to generate it */ + if (net->ifname == NULL) { + net->ifname = openvzGenerateVethName(veid, dev_name_ve); + if (net->ifname == NULL) { + openvzError(conn, VIR_ERR_INTERNAL_ERROR, + _("Could not generate veth name")); + rc = -1; + VIR_FREE(dev_name_ve); + goto exit; + } + } + + virBufferAdd(&buf, dev_name_ve, -1); /* Guest dev */ + virBufferVSprintf(&buf, ",%s", macaddr); /* Guest dev mac */ + virBufferVSprintf(&buf, ",%s", net->ifname); /* Host dev */ + virBufferVSprintf(&buf, ",%s", macaddr); /* Host dev mac */ + + if (driver->version >= VZCTL_BRIDGE_MIN_VERSION) { + virBufferVSprintf(&buf, ",%s", net->data.bridge.brname); /* Host bridge */ + } else { + virBufferVSprintf(configBuf, "ifname=%s", dev_name_ve); + virBufferVSprintf(configBuf, ",mac=%s", macaddr); /* Guest dev mac */ + virBufferVSprintf(configBuf, ",host_ifname=%s", net->ifname); /* Host dev */ + virBufferVSprintf(configBuf, ",host_mac=%s", macaddr); /* Host dev mac */ + virBufferVSprintf(configBuf, ",bridge=%s", net->data.bridge.brname); /* Host bridge */ + } + + VIR_FREE(dev_name_ve); + + if (!(opt = virBufferContentAndReset(&buf))) + goto no_memory; + ADD_ARG_LIT(opt) ; - }else if (net->type == VIR_DOMAIN_NET_TYPE_ETHERNET && + } else if (net->type == VIR_DOMAIN_NET_TYPE_ETHERNET && net->data.ethernet.ipaddr != NULL) { //--ipadd ip ADD_ARG_LIT("--ipadd") ; @@ -402,18 +485,57 @@ openvzDomainSetNetwork(virConnectPtr con
exit: cmdExecFree(prog); - VIR_FREE(mac); return rc;
no_memory: openvzError(conn, VIR_ERR_INTERNAL_ERROR, _("Could not put argument to %s"), VZCTL); cmdExecFree(prog); - VIR_FREE(mac); return -1;
#undef ADD_ARG_LIT } + + +static int +openvzDomainSetNetworkConfig(virConnectPtr conn, + virDomainDefPtr def) +{ + unsigned int i; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *param; + struct openvz_driver *driver = (struct openvz_driver *) conn->privateData; + + for (i = 0 ; i < def->nnets ; i++) { + if (driver->version < VZCTL_BRIDGE_MIN_VERSION && i > 0) + virBufferAddLit(&buf, ";"); + + if (openvzDomainSetNetwork(conn, def->name, def->nets[i], &buf) < 0) { + openvzError(conn, VIR_ERR_INTERNAL_ERROR, + "%s", _("Could not configure network")); + goto exit; + } + } + + param = virBufferContentAndReset(&buf); + if (driver->version < VZCTL_BRIDGE_MIN_VERSION && def->nnets) { + if (openvzWriteConfigParam(strtoI(def->name), "NETIF", param) < 0) { + VIR_FREE(param); + openvzError(conn, VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot replace NETIF config")); + return -1; + } + } + + VIR_FREE(param); + return 0;
param can be NULL and then dereferenced via openvzWriteConfigParam. How about this instead: if (driver->version < VZCTL_BRIDGE_MIN_VERSION && def->nnets) { char *param = virBufferContentAndReset(&buf); if (openvzWriteConfigParam(strtoI(def->name), "NETIF", param) < 0) { VIR_FREE(param); openvzError(conn, VIR_ERR_INTERNAL_ERROR, "%s", _("cannot replace NETIF config")); return -1; } VIR_FREE(param); } return 0;
+exit: + param = virBufferContentAndReset(&buf); + VIR_FREE(param);
Then free the above directly.
+ return -1; +} +
On Fri, Oct 17, 2008 at 03:58:05PM +0200, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote: ...
+int +openvzWriteConfigParam(int vpsid, const char *param, const char *value) +{ + char conf_file[PATH_MAX]; + char temp_file[PATH_MAX]; + char line[PATH_MAX] ; + int fd, temp_fd; + + if (openvzLocateConfFile(vpsid, conf_file, PATH_MAX, "conf")<0) + return -1; + if (openvzLocateConfFile(vpsid, temp_file, PATH_MAX, "tmp")<0) + return -1; + + fd = open(conf_file, O_RDONLY); + if (fd == -1) + return -1; + temp_fd = open(temp_file, O_WRONLY | O_CREAT | O_TRUNC, 0644); + if (temp_fd == -1) { + close(fd); + return -1; + } + + while(1) { + if (openvz_readline(fd, line, sizeof(line)) <= 0) + break; + + if (!STRPREFIX(line, param)) { + if (safewrite(temp_fd, line, strlen(line)) != + strlen(line)) + goto error; + } + } + + if (safewrite(temp_fd, param, strlen(param)) != + strlen(param)) + goto error; + if (safewrite(temp_fd, "=\"", 2) != 2) + goto error; + if (safewrite(temp_fd, value, strlen(value)) != + strlen(value)) + goto error; + if (safewrite(temp_fd, "\"\n", 2) != 2) + goto error;
How about this instead, so the reader doesn't have to manually count string lengths and verify that the "VAR" in strlen(VAR) on the RHS matches the one on the LHS:
if (safewrite(temp_fd, param, strlen(param)) < 0 || safewrite(temp_fd, "=\"", 2) < 0 || safewrite(temp_fd, value, strlen(value)) < 0 || safewrite(temp_fd, "\"\n", 2) < 0) goto error;
Yeah, that's good.
+ close(fd); + close(temp_fd);
Officially, you should always check for failure when you've opened the file descriptor for writing.
Ok, will fix.
+ fd = temp_fd = -1; + + if (rename(temp_file, conf_file) < 0) + goto error; + + return 0; + +error: + fprintf(stderr, "damn %s\n", strerror(errno));
How about mentioning the file name, too:
fprintf(stderr, "failed to write %s: %s\n", conf_file, strerror(errno));
That is debugging code I should have removed - we should raise a proper libvirt error if applicable.
+ +static int +openvzDomainSetNetworkConfig(virConnectPtr conn, + virDomainDefPtr def) +{ + unsigned int i; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *param; + struct openvz_driver *driver = (struct openvz_driver *) conn->privateData; + + for (i = 0 ; i < def->nnets ; i++) { + if (driver->version < VZCTL_BRIDGE_MIN_VERSION && i > 0) + virBufferAddLit(&buf, ";"); + + if (openvzDomainSetNetwork(conn, def->name, def->nets[i], &buf) < 0) { + openvzError(conn, VIR_ERR_INTERNAL_ERROR, + "%s", _("Could not configure network")); + goto exit; + } + } + + param = virBufferContentAndReset(&buf); + if (driver->version < VZCTL_BRIDGE_MIN_VERSION && def->nnets) { + if (openvzWriteConfigParam(strtoI(def->name), "NETIF", param) < 0) { + VIR_FREE(param); + openvzError(conn, VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot replace NETIF config")); + return -1; + } + } + + VIR_FREE(param); + return 0;
param can be NULL and then dereferenced via openvzWriteConfigParam. How about this instead:
if (driver->version < VZCTL_BRIDGE_MIN_VERSION && def->nnets) { char *param = virBufferContentAndReset(&buf); if (openvzWriteConfigParam(strtoI(def->name), "NETIF", param) < 0) { VIR_FREE(param); openvzError(conn, VIR_ERR_INTERNAL_ERROR, "%s", _("cannot replace NETIF config")); return -1; } VIR_FREE(param); }
return 0;
+exit: + param = virBufferContentAndReset(&buf); + VIR_FREE(param);
Then free the above directly.
Yep, good idea. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
The root filesystem for an openvz guest is defined from a template name. We support this when creating a new guest, but never include this info when dumping the XML. Thsi patch addresses this problem by reading the OSTEMPLATE config parameter diff -r e0c166ce24bd src/openvz_conf.c --- a/src/openvz_conf.c Tue Oct 14 15:46:24 2008 +0100 +++ b/src/openvz_conf.c Tue Oct 14 15:49:41 2008 +0100 @@ -308,6 +308,47 @@ error: } +static int +openvzReadFSConf(virConnectPtr conn, + virDomainDefPtr def, + int veid) { + int ret; + virDomainFSDefPtr fs; + char temp[4096]; + + ret = openvzReadConfigParam(veid, "OSTEMPLATE", temp, sizeof(temp)); + if (ret < 0) { + openvzError(conn, VIR_ERR_INTERNAL_ERROR, + _("Cound not read 'OSTEMPLATE' from config for container %d"), + veid); + goto error; + } else if (ret > 0) { + if (VIR_ALLOC(fs) < 0) + goto no_memory; + + fs->type = VIR_DOMAIN_FS_TYPE_TEMPLATE; + fs->src = strdup(temp); + fs->dst = strdup("/"); + + if (fs->src == NULL || fs->dst == NULL) + goto no_memory; + + if (VIR_REALLOC_N(def->fss, def->nfss + 1) < 0) + goto no_memory; + def->fss[def->nfss++] = fs; + fs = NULL; + + } + + return 0; +no_memory: + openvzError(conn, VIR_ERR_NO_MEMORY, NULL); +error: + virDomainFSDefFree(fs); + return -1; +} + + /* Free all memory associated with a openvz_driver structure */ void openvzFreeDriver(struct openvz_driver *driver) @@ -393,6 +434,7 @@ int openvzLoadDomains(struct openvz_driv /* XXX load rest of VM config data .... */ openvzReadNetworkConf(NULL, dom->def, veid); + openvzReadFSConf(NULL, dom->def, veid); if (VIR_REALLOC_N(driver->domains.objs, driver->domains.count + 1) < 0) -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
The root filesystem for an openvz guest is defined from a template name. We support this when creating a new guest, but never include this info when dumping the XML. Thsi patch addresses this problem by reading the OSTEMPLATE config parameter
diff -r e0c166ce24bd src/openvz_conf.c --- a/src/openvz_conf.c Tue Oct 14 15:46:24 2008 +0100 +++ b/src/openvz_conf.c Tue Oct 14 15:49:41 2008 +0100 @@ -308,6 +308,47 @@ error: }
+static int +openvzReadFSConf(virConnectPtr conn, + virDomainDefPtr def, + int veid) { + int ret; + virDomainFSDefPtr fs;
This needs to be initialized here or in the 'if' block. Otherwise, it will be freed uninitialized upon read failure: virDomainFSDefPtr fs = NULL;
+ char temp[4096]; + + ret = openvzReadConfigParam(veid, "OSTEMPLATE", temp, sizeof(temp)); + if (ret < 0) { + openvzError(conn, VIR_ERR_INTERNAL_ERROR, + _("Cound not read 'OSTEMPLATE' from config for container %d"), + veid); + goto error; + } else if (ret > 0) { + if (VIR_ALLOC(fs) < 0) + goto no_memory; + + fs->type = VIR_DOMAIN_FS_TYPE_TEMPLATE; + fs->src = strdup(temp); + fs->dst = strdup("/"); + + if (fs->src == NULL || fs->dst == NULL) + goto no_memory; + + if (VIR_REALLOC_N(def->fss, def->nfss + 1) < 0) + goto no_memory; + def->fss[def->nfss++] = fs; + fs = NULL; + + } + + return 0; +no_memory: + openvzError(conn, VIR_ERR_NO_MEMORY, NULL); +error: + virDomainFSDefFree(fs); + return -1; +} + + /* Free all memory associated with a openvz_driver structure */ void openvzFreeDriver(struct openvz_driver *driver) @@ -393,6 +434,7 @@ int openvzLoadDomains(struct openvz_driv /* XXX load rest of VM config data .... */
openvzReadNetworkConf(NULL, dom->def, veid); + openvzReadFSConf(NULL, dom->def, veid);
if (VIR_REALLOC_N(driver->domains.objs, driver->domains.count + 1) < 0)
On Thu, Oct 16, 2008 at 11:28:40AM +0200, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
The root filesystem for an openvz guest is defined from a template name. We support this when creating a new guest, but never include this info when dumping the XML. Thsi patch addresses this problem by reading the OSTEMPLATE config parameter
diff -r e0c166ce24bd src/openvz_conf.c --- a/src/openvz_conf.c Tue Oct 14 15:46:24 2008 +0100 +++ b/src/openvz_conf.c Tue Oct 14 15:49:41 2008 +0100 @@ -308,6 +308,47 @@ error: }
+static int +openvzReadFSConf(virConnectPtr conn, + virDomainDefPtr def, + int veid) { + int ret; + virDomainFSDefPtr fs;
This needs to be initialized here or in the 'if' block. Otherwise, it will be freed uninitialized upon read failure:
virDomainFSDefPtr fs = NULL;
Thanks, will fix that and then commit this. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Tue, Oct 14, 2008 at 04:22:35PM +0100, Daniel P. Berrange wrote:
The root filesystem for an openvz guest is defined from a template name. We support this when creating a new guest, but never include this info when dumping the XML. Thsi patch addresses this problem by reading the OSTEMPLATE config parameter
Except the fs initialization to NULL pointed by Jim looks fine, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (4)
-
Daniel P. Berrange -
Daniel Veillard -
Evgeniy Sokolov -
Jim Meyering