[libvirt] [PATCH] Fix the coding style
Fix the if ... else coding style, and indentions problem.
---
src/libvirt.c | 107 ++--
1 files changed, 57 insertions(+), 50 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c
index f509cc7..1304853 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -1199,18 +1199,19 @@ do_open(const char *name,
goto failed;
}
-VIR_DEBUG(trying driver %d (%s) ...,
- i, virDriverTab[i]-name);
+VIR_DEBUG(trying driver %d (%s) ..., i, virDriverTab[i]-name);
res = virDriverTab[i]-open(ret, auth, flags);
VIR_DEBUG(driver %d %s returned %s,
- i, virDriverTab[i]-name,
- res == VIR_DRV_OPEN_SUCCESS ? SUCCESS :
- (res == VIR_DRV_OPEN_DECLINED ? DECLINED :
- (res == VIR_DRV_OPEN_ERROR ? ERROR : unknown status)));
-if (res == VIR_DRV_OPEN_ERROR) goto failed;
-else if (res == VIR_DRV_OPEN_SUCCESS) {
+ i, virDriverTab[i]-name,
+ res == VIR_DRV_OPEN_SUCCESS ? SUCCESS :
+ (res == VIR_DRV_OPEN_DECLINED ? DECLINED :
+ (res == VIR_DRV_OPEN_ERROR ? ERROR : unknown status)));
+
+if (res == VIR_DRV_OPEN_SUCCESS) {
ret-driver = virDriverTab[i];
break;
+} else if (res == VIR_DRV_OPEN_ERROR) {
+goto failed;
}
}
@@ -1225,30 +1226,32 @@ do_open(const char *name,
for (i = 0; i virNetworkDriverTabCount; i++) {
res = virNetworkDriverTab[i]-open(ret, auth, flags);
VIR_DEBUG(network driver %d %s returned %s,
- i, virNetworkDriverTab[i]-name,
- res == VIR_DRV_OPEN_SUCCESS ? SUCCESS :
- (res == VIR_DRV_OPEN_DECLINED ? DECLINED :
- (res == VIR_DRV_OPEN_ERROR ? ERROR : unknown status)));
-if (res == VIR_DRV_OPEN_ERROR) {
-break;
-} else if (res == VIR_DRV_OPEN_SUCCESS) {
+ i, virNetworkDriverTab[i]-name,
+ res == VIR_DRV_OPEN_SUCCESS ? SUCCESS :
+ (res == VIR_DRV_OPEN_DECLINED ? DECLINED :
+ (res == VIR_DRV_OPEN_ERROR ? ERROR : unknown status)));
+
+if (res == VIR_DRV_OPEN_SUCCESS) {
ret-networkDriver = virNetworkDriverTab[i];
break;
+} else if (res == VIR_DRV_OPEN_ERROR) {
+break;
}
}
for (i = 0; i virInterfaceDriverTabCount; i++) {
res = virInterfaceDriverTab[i]-open(ret, auth, flags);
VIR_DEBUG(interface driver %d %s returned %s,
- i, virInterfaceDriverTab[i]-name,
- res == VIR_DRV_OPEN_SUCCESS ? SUCCESS :
- (res == VIR_DRV_OPEN_DECLINED ? DECLINED :
- (res == VIR_DRV_OPEN_ERROR ? ERROR : unknown status)));
-if (res == VIR_DRV_OPEN_ERROR) {
-break;
-} else if (res == VIR_DRV_OPEN_SUCCESS) {
+ i, virInterfaceDriverTab[i]-name,
+ res == VIR_DRV_OPEN_SUCCESS ? SUCCESS :
+ (res == VIR_DRV_OPEN_DECLINED ? DECLINED :
+ (res == VIR_DRV_OPEN_ERROR ? ERROR : unknown status)));
+
+if (res == VIR_DRV_OPEN_SUCCESS) {
ret-interfaceDriver = virInterfaceDriverTab[i];
break;
+} else if (res == VIR_DRV_OPEN_ERROR) {
+break;
}
}
@@ -1256,15 +1259,16 @@ do_open(const char *name,
for (i = 0; i virStorageDriverTabCount; i++) {
res = virStorageDriverTab[i]-open(ret, auth, flags);
VIR_DEBUG(storage driver %d %s returned %s,
- i, virStorageDriverTab[i]-name,
- res == VIR_DRV_OPEN_SUCCESS ? SUCCESS :
- (res == VIR_DRV_OPEN_DECLINED ? DECLINED :
- (res == VIR_DRV_OPEN_ERROR ? ERROR : unknown status)));
-if (res == VIR_DRV_OPEN_ERROR) {
-break;
- } else if (res == VIR_DRV_OPEN_SUCCESS) {
+ i, virStorageDriverTab[i]-name,
+ res == VIR_DRV_OPEN_SUCCESS ? SUCCESS :
+ (res == VIR_DRV_OPEN_DECLINED ? DECLINED :
+ (res == VIR_DRV_OPEN_ERROR ? ERROR : unknown status)));
+
+if (res == VIR_DRV_OPEN_SUCCESS) {
ret-storageDriver = virStorageDriverTab[i];
break;
+} else if (res == VIR_DRV_OPEN_ERROR) {
+break;
}
}
@@ -1272,15 +1276,16 @@ do_open(const char *name,
for (i = 0; i virDeviceMonitorTabCount; i++) {
res = virDeviceMonitorTab[i]-open(ret, auth, flags);
VIR_DEBUG(node driver %d %s returned %s,
- i, virDeviceMonitorTab[i]-name,
- res == VIR_DRV_OPEN_SUCCESS ? SUCCESS :
- (res == VIR_DRV_OPEN_DECLINED ? DECLINED :
- (res == VIR_DRV_OPEN_ERROR ? ERROR : unknown status)));
-if (res == VIR_DRV_OPEN_ERROR) {
-break;
-} else if (res ==
Re: [libvirt] [PATCH] virsh: don't crash if shutdown --mode not provided
On Fri, Nov 30, 2012 at 02:56:08PM -0700, Eric Blake wrote: virStringSplit requires a non-NULL input, but commit cef78ed forgot to follow the rule. * tools/virsh-domain.c (cmdReboot, cmdShutdown): Avoid NULL deref. --- Pushing under the build-breaker rule. tools/virsh-domain.c | 12 ++-- 1 files changed, 6 insertions(+), 6 deletions(-) ACK, thanks for finding this. I guess I forgot to re-run the test suite after the v2 of my series changing virsh. 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fix the coding style
On Mon, Dec 03, 2012 at 04:41:45PM +0800, Osier Yang wrote: Fix the if ... else coding style, and indentions problem. --- src/libvirt.c | 107 ++-- 1 files changed, 57 insertions(+), 50 deletions(-) ACK, this does look nicer now. 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] conf: prevent crash with no uuid in cephx auth secret
On 11/29/12 17:36, Ján Tomko wrote:
Also remove the pointless check for NULL in auth.cephx.secret.uuid,
since this is a static array.
---
src/conf/storage_conf.c |8 +++-
1 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 3fdc5b6..99c2e52 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -458,7 +458,7 @@ virStoragePoolDefParseAuthCephx(xmlXPathContextPtr ctxt,
return -1;
}
-if (virUUIDParse(uuid, auth-secret.uuid) 0) {
+if (uuid virUUIDParse(uuid, auth-secret.uuid) 0) {
Given the XML schema that was discussed in the previous version of this
patch, this hunk is OK, but it doesn't mark the secret that it doesn't
have an UUID. The old check that was present wasn't effective enough.
With this patch ceph secrets that have just the usage argument will have
an UUID full of zeroes.
virReportError(VIR_ERR_XML_ERROR,
%s, _(invalid auth secret uuid));
return -1;
@@ -979,10 +979,8 @@ virStoragePoolSourceFormat(virBufferPtr buf,
src-auth.cephx.username);
virBufferAsprintf(buf, %s, secret);
-if (src-auth.cephx.secret.uuid != NULL) {
-virUUIDFormat(src-auth.cephx.secret.uuid, uuid);
-virBufferAsprintf(buf, uuid='%s', uuid);
-}
+virUUIDFormat(src-auth.cephx.secret.uuid, uuid);
+virBufferAsprintf(buf, uuid='%s', uuid);
And it will be formated as such here.
if (src-auth.cephx.secret.usage != NULL) {
virBufferAsprintf(buf, usage='%s',
src-auth.cephx.secret.usage);
And in virStorageBackendRBDOpenRADOSConn() in
src/storage/storage_backend_rbd.c:
if (pool-def-source.auth.cephx.secret.uuid != NULL) {
virUUIDFormat(pool-def-source.auth.cephx.secret.uuid, secretUuid);
VIR_DEBUG(Looking up secret by UUID: %s, secretUuid);
secret = virSecretLookupByUUIDString(conn, secretUuid);
}
if (pool-def-source.auth.cephx.secret.usage != NULL) {
VIR_DEBUG(Looking up secret by usage: %s,
pool-def-source.auth.cephx.secret.usage);
secret = virSecretLookupByUsage(conn, VIR_SECRET_USAGE_TYPE_CEPH,
pool-def-source.auth.cephx.secret.usage);
}
If such a secret exists, it will be found by the UUID string at first
and then overwritten when looked up by usage, this would be OK, if we
wouldn't have to free the secret value afterwards. This causes a memleak.
As a fix, the options (if the schema actually is ok and UUID can be left
out) that come into my mind are:
1) add a new (bool) field to the secret value holding if UUID was provided
2) converting UUID to a pointer and marking the missing value with NULL
as it was before.
Option 1 is probably better as you don't have to keep in mind to free
the UUID array afterwards.
Peter
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2.1 4/4] v1: disable dnsmasq's RA service; use radvd
THIS PATCH IS WITHDRAWN!! If any of you are following Dnsmasq-discuss mailing list, you may be aware of the efforts to identify and correct the problem. The patch is no longer necessary because the problem has been discovered and a fix tested. The final fix by Simon Kelley will be included in dnsmasq 2.64. If you want to see the details, visit Dnsmasq-discuss. I do seem to have a bit of reverse midas touch as far as using dnsmasq's IPv6 services. I certainly found some problems. Fortunately, all the problems were fixable and will be in dnsmasq 2.64. Gene -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fix the coding style
On 2012年12月03日 19:10, Daniel P. Berrange wrote: On Mon, Dec 03, 2012 at 04:41:45PM +0800, Osier Yang wrote: Fix the if ... else coding style, and indentions problem. --- src/libvirt.c | 107 ++-- 1 files changed, 57 insertions(+), 50 deletions(-) ACK, this does look nicer now. Daniel Thanks, pushed. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 1/3] v1: allow guest to guest IPv6 without gateway definition
On 12/02/2012 12:39 PM, Laine Stump wrote: On 11/30/2012 10:01 AM, Gene Czarcinski wrote: This patch adds the capability for virtual guests to do IPv6 communication via a virtual network interface with no IPv6 (gateway) addresses specified. This capability currently exists for IPv4. This patch allows creation of a completely isolated IPv6 network. Note that virtual guests cannot communication with the virtualization host via this interface. Also note that: net.ipv6.conf.interface_name.disable_ipv6 = 1 Also not that starting libvirtd has set the following: net.bridge.bridge-nf-call-arptables = 1 net.bridge.bridge-nf-call-ip6tables = 1 net.bridge.bridge-nf-call-iptables = 1 although /etc/syslog.conf has them all set to 0. Yeah :-(, it's a rather unfortunate chain of events that leads to this, caused by the following: 1) the bridge module in the kernel has 1 compiled in as the default, so that's what is set when the module is first loaded, 2) the settings in sysctl.conf are set during startup of either the NetworkManager or network service (depending on which you've chosen). This is done after any bridges in the system config are started, so if you have a bridge defined there, the bridge module will be loaded, then sysctl -p will be run, thus making sure the setting from /etc/sysctl.conf is enacted. If the system has no bridges defined, sysctl -p will fall on deaf ears, since those tunables don't exist yet. 3) sometime later libvirt creates a bridge device with an ioctl(). Unknown to libvirt, this causes the bridge module to autoload, and it of course starts up with the compiled-in default settings of 1. For many reasons it's not desirable for libvirt to run sysctl -p (potential side effects on other completely unrelated subsystems) or even manually set those few items (maybe the admin really *does* want that policy). Note: If it is desired to control this behavior by having something like ipv6='yes' on the network statement, then this should also be done for ipv4. It's always nicer to not have to add another knob, and I was hoping that we could get by on this one without it. There were a few potential issues that we've discussed previously that could make it necessary to not turn this on by default: 1) unintentionally opening up a communications line to the host via the IPv6 link local address - as long as disable_ipv6 is set for the interface, this isn't an issue. I did not need to do anything. If no IPv6 addresses are specified then this network: network namenogw/name uuid7a3b7497-1ec7-8aef-6d5c-38dff9109e93/uuid bridge name='virbr19' stp='on' delay='0' / mac address='52:54:00:08:10:43'/ /network results in: # sysctl -a | grep ipv6 | grep virbr19 | grep disable net.ipv6.conf.virbr19.disable_ipv6 = 1 net.ipv6.conf.virbr19-nic.disable_ipv6 = 0 2) some installations don't have ip6tables - my opinion on this one is that ip6tables is part of the iptables package, and people shouldn't be building/installing just part of a package anyway. Mmm. However, there a point here. This is the only time when not specifying an IPv6 address results in something done with ip6tables. Maybe there should be a test added to make sure that it exists and is executable (such as that done for dnsmasq). 3) Even if ip6tables is available on the system, the additional commands may fail on some systems - this is our Waterloo :-( I finally just now tried disabling the ipv6 module on a RHEL6 system, then running ip6tables -S. I was greeted by the following error message and a non-0 exit code: ip6tables v1.4.7: can't initialize ip6tables table `filter': Address family not supported by protocol Perhaps ip6tables or your kernel needs to be upgraded. [root@rhel6-guest laine]# echo $? 3 This means that the addition of this patch as-is would result in libvirt failing to start any network on a system that had disabled IPv6, and my guess is there are still a lot of those around. So we're unfortunately going to have to add a network ipv6='yes' to enable this; if you don't have the time, I'll add that tonight and repost. Actually, since the dnsmasq extra RA to the real NIC bug has been found, a fix tested, and the promise by Simon Kelley that it will be in 2.64 final, I do have the time ;) (I disagree that an ipv4 attribute is also needed. the rules to allow ipv4 have been there by default since the networks were added to libvirt, so we don't want them disabled by default, and up to now nobody has ever asked for them to be manually disabled, so there apparently isn't a need for it. If it ever does come up, we can add the attribute then, but I'd rather not bloat the grammar just for the sake of symmetry.) I like the symmetry of the ipv4 parameter but it is certainly not worth any argument. So the default is ipv6='no' and network ipv6='yes' will need to be specified for the code added by this patch to work. Gene Other than that, the patch looks fine. ---
[libvirt] [PATCH] maint: Misc whitespace cleanups
---
src/conf/storage_conf.c | 2 +-
src/util/virtypedparam.c | 6 ++
tools/virsh-domain.c | 3 +--
3 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 3fdc5b6..2d72d06 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -441,7 +441,7 @@ virStoragePoolDefParseAuthChap(xmlXPathContextPtr ctxt,
static int
virStoragePoolDefParseAuthCephx(xmlXPathContextPtr ctxt,
- virStoragePoolAuthCephxPtr auth) {
+virStoragePoolAuthCephxPtr auth) {
char *uuid = NULL;
auth-username = virXPathString(string(./auth/@username), ctxt);
if (auth-username == NULL) {
diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c
index 0b7a268..7f0a44b 100644
--- a/src/util/virtypedparam.c
+++ b/src/util/virtypedparam.c
@@ -250,11 +250,9 @@ virTypedParameterAssignFromStr(virTypedParameterPtr param,
const char *name,
}
break;
case VIR_TYPED_PARAM_BOOLEAN:
-if (STRCASEEQ(val, true) ||
-STREQ(val, 1)) {
+if (STRCASEEQ(val, true) || STREQ(val, 1)) {
param-value.b = true;
-} else if (STRCASEEQ(val, false) ||
- STREQ(val, 0)) {
+} else if (STRCASEEQ(val, false) || STREQ(val, 0)) {
param-value.b = false;
} else {
virReportError(VIR_ERR_INVALID_ARG,
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index f12777c..73ebba9 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -3576,8 +3576,7 @@ cmdSchedinfo(vshControl *ctl, const vshCmd *cmd)
/* Print SchedulerType */
schedulertype = virDomainGetSchedulerType(dom, nparams);
if (schedulertype != NULL) {
-vshPrint(ctl, %-15s: %s\n, _(Scheduler),
- schedulertype);
+vshPrint(ctl, %-15s: %s\n, _(Scheduler), schedulertype);
VIR_FREE(schedulertype);
} else {
vshPrint(ctl, %-15s: %s\n, _(Scheduler), _(Unknown));
--
1.8.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] virsh: Report errors if arguments of the schedinfo command are incorrect
Libvirt's helper API's when called directly don't raise the error so
that virsh remembers it. Subsequent calls to libvirt API's might reset
the error.
In case of schedinfo virDomainFree() in the cleanup section resets the
error when virTypedParameterAssignFromStr() fails.
This patch adds function vshSaveLibvirtError() that can be called after
calling libvirt helper APIs to ensure the error is remembered.
---
tools/virsh-domain.c | 8 ++--
tools/virsh.c| 9 +
tools/virsh.h| 1 +
3 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 73ebba9..1f7aff7 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -3512,8 +3512,10 @@ cmdSchedInfoUpdate(vshControl *ctl, const vshCmd *cmd,
if (virTypedParameterAssign((params[nparams++]),
param-field,
param-type,
-val) 0)
+val) 0) {
+vshSaveLibvirtError();
goto cleanup;
+}
continue;
}
@@ -3523,8 +3525,10 @@ cmdSchedInfoUpdate(vshControl *ctl, const vshCmd *cmd,
if (virTypedParameterAssignFromStr((params[nparams++]),
param-field,
param-type,
- set_val) 0)
+ set_val) 0) {
+vshSaveLibvirtError();
goto cleanup;
+}
continue;
}
diff --git a/tools/virsh.c b/tools/virsh.c
index dea3f82..322f778 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -240,6 +240,15 @@ virshErrorHandler(void *unused ATTRIBUTE_UNUSED,
virErrorPtr error)
virDefaultErrorFunc(error);
}
+/* Store a libvirt error that is from a helper API that doesn't raise errors
+ * so it doesn't get overwritten */
+void
+vshSaveLibvirtError(void)
+{
+virFreeError(last_error);
+last_error = virSaveLastError();
+}
+
/*
* Reset libvirt error on graceful fallback paths
*/
diff --git a/tools/virsh.h b/tools/virsh.h
index ba44f42..6913ed1 100644
--- a/tools/virsh.h
+++ b/tools/virsh.h
@@ -346,6 +346,7 @@ struct _vshCtrlData {
extern virErrorPtr last_error;
void vshReportError(vshControl *ctl);
void vshResetLibvirtError(void);
+void vshSaveLibvirtError(void);
/* allocation wrappers */
void *_vshMalloc(vshControl *ctl, size_t sz, const char *filename, int line);
--
1.8.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] maint: Misc whitespace cleanups
On 2012年12月03日 21:46, Peter Krempa wrote:
---
src/conf/storage_conf.c | 2 +-
src/util/virtypedparam.c | 6 ++
tools/virsh-domain.c | 3 +--
3 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 3fdc5b6..2d72d06 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -441,7 +441,7 @@ virStoragePoolDefParseAuthChap(xmlXPathContextPtr ctxt,
static int
virStoragePoolDefParseAuthCephx(xmlXPathContextPtr ctxt,
- virStoragePoolAuthCephxPtr auth) {
+virStoragePoolAuthCephxPtr auth) {
char *uuid = NULL;
auth-username = virXPathString(string(./auth/@username), ctxt);
if (auth-username == NULL) {
diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c
index 0b7a268..7f0a44b 100644
--- a/src/util/virtypedparam.c
+++ b/src/util/virtypedparam.c
@@ -250,11 +250,9 @@ virTypedParameterAssignFromStr(virTypedParameterPtr param,
const char *name,
}
break;
case VIR_TYPED_PARAM_BOOLEAN:
-if (STRCASEEQ(val, true) ||
-STREQ(val, 1)) {
+if (STRCASEEQ(val, true) || STREQ(val, 1)) {
param-value.b = true;
-} else if (STRCASEEQ(val, false) ||
- STREQ(val, 0)) {
+} else if (STRCASEEQ(val, false) || STREQ(val, 0)) {
param-value.b = false;
} else {
virReportError(VIR_ERR_INVALID_ARG,
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index f12777c..73ebba9 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -3576,8 +3576,7 @@ cmdSchedinfo(vshControl *ctl, const vshCmd *cmd)
/* Print SchedulerType */
schedulertype = virDomainGetSchedulerType(dom,nparams);
if (schedulertype != NULL) {
-vshPrint(ctl, %-15s: %s\n, _(Scheduler),
- schedulertype);
+vshPrint(ctl, %-15s: %s\n, _(Scheduler), schedulertype);
VIR_FREE(schedulertype);
} else {
vshPrint(ctl, %-15s: %s\n, _(Scheduler), _(Unknown));
ACK
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv3] conf: prevent crash with no uuid in cephx auth secret
On 12/03/12 13:35, Ján Tomko wrote: Fix the null pointer access when UUID is not specified. Introduce a bool 'uuidUsable' to virStoragePoolAuthCephx that indicates if uuid was specified or not and use it instead of the pointless comparison of the static UUID array to NULL. Add an error message if both uuid and usage are specified. Fixes: Error: FORWARD_NULL (CWE-476): libvirt-0.10.2/src/conf/storage_conf.c:461: var_deref_model: Passing null pointer uuid to function virUUIDParse(char const *, unsigned char *), which dereferences it. (The dereference is assumed on the basis of the 'nonnull' parameter attribute.) Error: NO_EFFECT (CWE-398): libvirt-0.10.2/src/conf/storage_conf.c:979: array_null: Comparing an array to null is not useful: src-auth.cephx.secret.uuid != NULL. --- src/conf/storage_conf.c | 20 +++- src/conf/storage_conf.h |1 + src/storage/storage_backend_rbd.c |6 ++ 3 files changed, 18 insertions(+), 9 deletions(-) Now it looks OK to me. ACK. Peter -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv3] conf: prevent crash with no uuid in cephx auth secret
On 12/03/12 15:04, Peter Krempa wrote: On 12/03/12 13:35, Ján Tomko wrote: Fix the null pointer access when UUID is not specified. Introduce a bool 'uuidUsable' to virStoragePoolAuthCephx that indicates if uuid was specified or not and use it instead of the pointless comparison of the static UUID array to NULL. Add an error message if both uuid and usage are specified. Fixes: Error: FORWARD_NULL (CWE-476): libvirt-0.10.2/src/conf/storage_conf.c:461: var_deref_model: Passing null pointer uuid to function virUUIDParse(char const *, unsigned char *), which dereferences it. (The dereference is assumed on the basis of the 'nonnull' parameter attribute.) Error: NO_EFFECT (CWE-398): libvirt-0.10.2/src/conf/storage_conf.c:979: array_null: Comparing an array to null is not useful: src-auth.cephx.secret.uuid != NULL. --- src/conf/storage_conf.c | 20 +++- src/conf/storage_conf.h |1 + src/storage/storage_backend_rbd.c |6 ++ 3 files changed, 18 insertions(+), 9 deletions(-) Now it looks OK to me. ACK. Peter And pushed. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] maint: Misc whitespace cleanups
On 12/03/12 14:59, Osier Yang wrote: On 2012年12月03日 21:46, Peter Krempa wrote: --- src/conf/storage_conf.c | 2 +- src/util/virtypedparam.c | 6 ++ tools/virsh-domain.c | 3 +-- 3 files changed, 4 insertions(+), 7 deletions(-) ... ACK Thanks, pushed. Peter -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virsh: Report errors if arguments of the schedinfo command are incorrect
On Mon, Dec 03, 2012 at 14:46:51 +0100, Peter Krempa wrote:
Libvirt's helper API's when called directly don't raise the error so
that virsh remembers it. Subsequent calls to libvirt API's might reset
the error.
In case of schedinfo virDomainFree() in the cleanup section resets the
error when virTypedParameterAssignFromStr() fails.
This patch adds function vshSaveLibvirtError() that can be called after
calling libvirt helper APIs to ensure the error is remembered.
---
tools/virsh-domain.c | 8 ++--
tools/virsh.c| 9 +
tools/virsh.h| 1 +
3 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 73ebba9..1f7aff7 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -3512,8 +3512,10 @@ cmdSchedInfoUpdate(vshControl *ctl, const vshCmd *cmd,
if (virTypedParameterAssign((params[nparams++]),
param-field,
param-type,
-val) 0)
+val) 0) {
+vshSaveLibvirtError();
goto cleanup;
+}
continue;
}
Looks like there must be a lot of similar issues in other areas of virsh.
ACK,
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] virSecurity hook for hugepages?
Hi, Currently the hugepages support can automatically detect the hugepages mount, but it doesn't update the security information. At least for apparmor we need to be able to add permission for the domain to access the hugetlbfs mount path. There are a few ways this could be done, 1. add a virSecuritySetSecurityHugepages or virSecuritySetSecurityHugepagesFD hook which is called perhaps at qemudStartup 2. optionally add the qemu_driver-hugepage_path to the xml output, at least for the internal format (which is passed to virt-aa-helper). The concern I have with this is that it brings up the issue of what to do when defining a domain which has such an entry. 3. reproduce the logic in virt-aa-helper for detecting the hugepages mount path. Not preferred obviously. My guess would be that (1) would be preferred, but I wanted to ask here first and see if there are other suggestions. thanks, -serge -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: refactor iptables command construction into multiple steps
On Thu, 22 Nov 2012 15:02:18 +0100 Natanael Copa nc...@alpinelinux.org wrote: Instead of creating an iptables command in one shot, do it in steps so we can add conditional options like physdev and protocol. This removes code duplication while keeping existing behaviour. Signed-off-by: Natanael Copa nc...@alpinelinux.org --- This started with me wanting to add support for setting the public ip source address when network mode='nat' and there are multiple public ip addresses on the external interface. On IRC we talked about adding an option in the xml like this: network forward mode='nat' publicaddr='n.n.n.n'/ /network Which would make iptables use '-j SNAT --to-source n.n.n.n' instead of '-j MASQUERADE'. I have a working patch for the above and it appears to work. I wonder if we want go for 'publicaddr' as the attribute name? -nc -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 01/11] bandwidth: add new 'floor' attribute
On 30.11.2012 13:14, Laine Stump wrote:
On 11/19/2012 11:51 AM, Michal Privoznik wrote:
This is however supported only on domain interfaces with
type='network'. Moreover, target network needs to have at least
inbound QoS set. This is required by hierarchical traffic shaping.
From now on, the required attribute for inbound/ is either 'average'
(old) or 'floor' (new). This new attribute can be used just for
interfaces type of network (interface type='network'/) currently.
---
docs/formatdomain.html.in| 20 --
docs/schemas/networkcommon.rng |5 +++
src/conf/domain_conf.c |6 +++-
src/conf/netdev_bandwidth_conf.c | 54
+-
src/conf/netdev_bandwidth_conf.h |3 +-
src/conf/network_conf.c |4 +-
src/util/virnetdevbandwidth.h|1 +
7 files changed, 78 insertions(+), 15 deletions(-)
diff --git a/src/conf/netdev_bandwidth_conf.c
b/src/conf/netdev_bandwidth_conf.c
index 261718f..38c5a5b 100644
--- a/src/conf/netdev_bandwidth_conf.c
+++ b/src/conf/netdev_bandwidth_conf.c
@@ -26,6 +26,7 @@
#include virterror_internal.h
#include util.h
#include memory.h
+#include domain_conf.h
#define VIR_FROM_THIS VIR_FROM_NONE
@@ -36,6 +37,7 @@ virNetDevBandwidthParseRate(xmlNodePtr node,
virNetDevBandwidthRatePtr rate)
char *average = NULL;
char *peak = NULL;
char *burst = NULL;
+char *floor = NULL;
if (!node || !rate) {
virReportError(VIR_ERR_INVALID_ARG, %s,
@@ -46,6 +48,7 @@ virNetDevBandwidthParseRate(xmlNodePtr node,
virNetDevBandwidthRatePtr rate)
average = virXMLPropString(node, average);
peak = virXMLPropString(node, peak);
burst = virXMLPropString(node, burst);
+floor = virXMLPropString(node, floor);
if (average) {
if (virStrToLong_ull(average, NULL, 10, rate-average) 0) {
@@ -54,9 +57,15 @@ virNetDevBandwidthParseRate(xmlNodePtr node,
virNetDevBandwidthRatePtr rate)
average);
goto cleanup;
}
-} else {
+} else if (!floor) {
virReportError(VIR_ERR_XML_DETAIL, %s,
- _(Missing mandatory average attribute));
+ _(Missing mandatory average or floor attributes));
+goto cleanup;
+}
+
+if ((peak || burst) !average) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
+ _('peak' and 'burst' require 'average' attribute));
goto cleanup;
}
I didn't understand if floor and average were mutually exclusive. If
that's the case, then you need to check for it here. Otherwise, I just
need to understand better :-)
They are not mutually exclusive. The interface (of correct type as
you've pointed out above) can has both 'floor' and 'average'. The first
is applied on bridge the second directly on the interface.
I admit that this QoS magic is kind of black box. But if you ever saw a
picture of flow of a packet within linux kernel, where are which
{ip,eb}tables rules applied, then you know why is that. See [1] for
instance.
ACK with the minor things fixed (the bit about having a different log
message for network vs. interface is optional; or maybe you can think of
a single message that works better for both cases).
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
1: http://blog.propter.net/wp-content/uploads/2012/02/PacketFlow.png
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 10/11] bandwidth: Attach sfq to leaf node
On 30.11.2012 22:00, Laine Stump wrote: On 11/19/2012 11:51 AM, Michal Privoznik wrote: SFQ is qdisc which doesn't really shape any traffic but 'just' re-arrange packets in sending buffer so no stream starve. The goal is to ensure fairness. There is basically only one configuration parameter (perturb) which is set to advised value of 10. If this is a part of adding floor, then it should be merged into the earlier patch (2/11 I think?). If it is a good thing to have independent of floor, then I think it should be put in right at the beginning of the series (without the reference to hierarchical_class) so that if somebody wants to backport just that patch, it will be easier (then in 2/11 you would add the reference to hierarchical_class). Otherwise ACK. Okay, I'll move it in the front. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 01/11] bandwidth: add new 'floor' attribute
On 30.11.2012 13:58, Laine Stump wrote: On 11/19/2012 11:51 AM, Michal Privoznik wrote: This is however supported only on domain interfaces with type='network'. Moreover, target network needs to have at least inbound QoS set. This is required by hierarchical traffic shaping. From now on, the required attribute for inbound/ is either 'average' (old) or 'floor' (new). This new attribute can be used just for interfaces type of network (interface type='network'/) currently. Another thing that I just thought of - this patch needs to update virNetdevBandwidthEqual to check floor, otherwise qemuNetChange won't properly detect when bandwidth is changed (btw, a small patch to do the right thing when that happens (similar to the patch I recently sent to support changing filters) would be really nice :-) Okay, I'll save it for a separate patch. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 03/11] bandwidth: Create (un)plug functions
On 30.11.2012 18:53, Laine Stump wrote:
On 11/19/2012 11:51 AM, Michal Privoznik wrote:
These set bridge part of QoS when bringing domain's interface up.
Long story short, if there's a 'floor' set, a new QoS class is created.
ClassID MUST be unique within the bridge and should be kept for
unplug phase.
---
po/POTFILES.in|1 +
src/util/virnetdevbandwidth.c | 178
+
src/util/virnetdevbandwidth.h | 14 +++
3 files changed, 193 insertions(+), 0 deletions(-)
diff --git a/po/POTFILES.in b/po/POTFILES.in
index 9768528..f51e2c7 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -154,6 +154,7 @@ src/util/virhash.c
src/util/virkeyfile.c
src/util/virlockspace.c
src/util/virnetdev.c
+src/util/virnetdevbandwidth.c
src/util/virnetdevbridge.c
src/util/virnetdevmacvlan.c
src/util/virnetdevopenvswitch.c
diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c
index fcc6b56..9597dcd 100644
--- a/src/util/virnetdevbandwidth.c
+++ b/src/util/virnetdevbandwidth.c
@@ -285,3 +285,181 @@ virNetDevBandwidthEqual(virNetDevBandwidthPtr a,
return true;
}
+
+/*
+ * virNetDevBandwidthPlug:
+ * @brname: name of the bridge
+ * @net_bandwidth: QoS settings on @brname
+ * @ifname: interface being plugged into @brname
+ * @ifmac: MAC of @ifname
+ * @bandwidth: QoS settings for @ifname
+ * @id: unique ID (MUST be greater than 2)
* If it must be 2, then you need to check for that at the top of the
function.
+ *
+ * Set bridge part of interface QoS settings, e.g. guaranteed
+ * bandwidth. @id is an unique ID (among @brname) from which
+ * other identifiers for class, qdisc and filter are derived.
+ * However, two classes were already set up (by
+ * virNetDevBandwidthSet). That's why this @id MUST be greater
+ * than 2. You may want to keep passed @id, as it is used later
+ * by virNetDevBandwidthUnplug.
+ *
+ * Returns:
+ * 1 if there is nothing to set
+ * 0 if QoS set successfully
+ * -1 otherwise.
+ */
+int
+virNetDevBandwidthPlug(const char *brname,
+ virNetDevBandwidthPtr net_bandwidth,
+ const char *ifname,
+ const virMacAddrPtr ifmac_ptr,
+ virNetDevBandwidthPtr bandwidth,
+ unsigned int id)
+{
+int ret = -1;
+virCommandPtr cmd = NULL;
+char *class_id = NULL;
+char *qdisc_id = NULL;
+char *filter_id = NULL;
+char *floor = NULL;
+char *ceil = NULL;
+unsigned char ifmac[VIR_MAC_BUFLEN];
+char *mac[2];
+
+if (!bandwidth || !bandwidth-in || !bandwidth-in-floor) {
+/* nothing to set */
+return 1;
+}
+
+if (!net_bandwidth || !net_bandwidth-in) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _(Bridge '%s' has no QoS set, therefore
+ unable to set 'floor' on '%s'),
+ brname, ifname);
+return -1;
+}
+
+virMacAddrGetRaw(ifmac_ptr, ifmac);
+
+if (virAsprintf(class_id, 1:%x, id) 0 ||
+virAsprintf(qdisc_id, %x:, id) 0 ||
+virAsprintf(filter_id, %u, id) 0 ||
+virAsprintf(mac[0], 0x%02x%02x%02x%02x, ifmac[2],
+ifmac[3], ifmac[4], ifmac[5]) 0 ||
+virAsprintf(mac[1], 0x%02x%02x, ifmac[0], ifmac[1]) 0 ||
+virAsprintf(floor, %llukbps, bandwidth-in-floor) 0 ||
+virAsprintf(ceil, %llukbps, net_bandwidth-in-peak ?
+net_bandwidth-in-peak :
+net_bandwidth-in-average) 0) {
+virReportOOMError();
+goto cleanup;
+}
+
+cmd = virCommandNew(TC);
+virCommandAddArgList(cmd, class, add, dev, brname, parent,
1:1,
+ classid, class_id, htb, rate, floor,
+ ceil, ceil, NULL);
+
+if (virCommandRun(cmd, NULL) 0)
+goto cleanup;
+
+virCommandFree(cmd);
+cmd = virCommandNew(TC);
+virCommandAddArgList(cmd, qdisc, add, dev, brname, parent,
+ class_id, handle, qdisc_id, sfq, perturb,
+ 10, NULL);
+
+if (virCommandRun(cmd, NULL) 0)
+goto cleanup;
+
+virCommandFree(cmd);
+cmd = virCommandNew(TC);
+/* Okay, this not nice. But since libvirt does not know anything about
+ * interface IP address(es), and tc fw filter simply refuse to use
ebtables
+ * marks, we need to use u32 selector to match MAC address.
+ * If libvirt will ever know something, remove this FIXME
Heck, maybe not even then - don't forget about IPv6, along with the fact
that I'm not convinced the host can ever know all the IP addresses used
by an untrusted guest with 100% certainty (unless we filter for them I
suppose).
+ */
+virCommandAddArgList(cmd, filter, add, dev, brname, protocol,
ip,
+
Re: [libvirt] [PATCH v1 09/11] network: Allow class ID to be reused
On 30.11.2012 21:55, Laine Stump wrote: On 11/19/2012 11:51 AM, Michal Privoznik wrote: With current implementation, class ID is just incremented. This can lead to its exhaustion as tc accepts only 16 bits long identifiers. Hence, it's better if we allow class ID to be reused. To keep track which IDs are free and which are taken virBitmap is used. This requires network status file to change a bit: from class_id next=5/ to class_id bitmap=0-4/. But since the previous format hasn't been released, it doesn't really matter. Heh. Well, there you have it. :-) You've already implemented what I suggested in the review of 5/10. But rather than introducing one implementation that we need to review, then almost immediately replacing it with something else, why not just implement it this way to begin with? Because I think 5/10 is big enough already :) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 06/11] network: Create real network status files
On 30.11.2012 20:04, Laine Stump wrote: On 11/19/2012 11:51 AM, Michal Privoznik wrote: Currently, we are only keeping a inactive XML configuration in status dir. This is no longer enough as we need to keep this class_id attribute so we don't overwrite old entries when the daemon restarts. Aha! So you're looking at solving the problem in a different way - save everything to the status file rather than recomputing it as you restart. While I like the idea of having the network status file hold this information, I think its reliability is suspect. What if a guest's process is terminated while libvirtd isn't running? Or what if libvirtd exits unexpectedly after the commands to setup bandwidth have been executed, but before the new network state file has been written (or vice versa, depending on the code). Also, networks aren't properly shut down when the host is being shutdown (there's no equivalent to the libvirt-guests service, although at least one person a month or two ago requested it). If guest is being shut down, the networkReleaseActualDevice() is called, isn't it? And this should update the floor_sum. Even if libvirtd is restarted and qemu process dies meanwhile, qemuProcessStop is called and this calls the networkReleaseActualDevice() so I think we are safe here. If everybody agrees that saving this info to a file and re-reading it on start up is reliable, though, then we might as well do the same thing with the device pool (although it's currently a bit different - the inuse count is stored in the virNetworkDef rather than Obj, and is reported during net-dumpxml) With new code we can update this. However, since there has already been a libvirt release *a* libvirt release? :-) That 'a' belongs to 'release'. It doesn't matter which release it was. Yeah, now that I re-read it again it's a mess :) which has just network/ as root element, and we want to keep things compatible, detect that loaded status file is older one, and don't scream about it. --- src/conf/network_conf.c | 199 ++- src/conf/network_conf.h |2 + src/libvirt_private.syms|1 + src/network/bridge_driver.c |9 +-- 4 files changed, 165 insertions(+), 46 deletions(-) ACK. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 05/11] bandwidth: Create network (un)plug functions
On 30.11.2012 19:43, Laine Stump wrote:
On 11/19/2012 11:51 AM, Michal Privoznik wrote:
Network should be notified if we plug in or unplug an
interface, so it can perform some action, e.g. set/unset
network part of QoS.
---
src/Makefile.am |7 ++-
src/conf/domain_conf.h |1 +
src/conf/network_conf.c |1 +
src/conf/network_conf.h |3 +
src/libvirt_network.syms|8 ++
src/network/bridge_driver.c | 165
+++
src/network/bridge_driver.h | 10 ++-
src/qemu/qemu_command.c | 32 ++---
src/qemu/qemu_process.c | 12 +++
9 files changed, 225 insertions(+), 14 deletions(-)
create mode 100644 src/libvirt_network.syms
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
index 3e46304..8a8d1e4 100644
--- a/src/conf/network_conf.h
+++ b/src/conf/network_conf.h
@@ -221,6 +221,9 @@ struct _virNetworkObj {
virNetworkDefPtr def; /* The current definition */
virNetworkDefPtr newDef; /* New definition to activate at shutdown */
+
+unsigned int class_id; /* next class ID for QoS */
Okay, so this is just a monotonically increasing counter so that each
interface gets a new and different class_id. Maybe you should call it
nextFreeClassID or something like that, so that everyone understands
it's not a class id used by the network. Or you might want to do this
with a bitmap so you can re-use id's of interfaces that are
disconnected. (can virBitmaps being dynamically expanded?)
+unsigned long long floor_sum; /* sum of all 'floor'-s of attached NICs
*/
And you keep track of the sum of all floors here.
So, what happens if libvirtd is restarted? It looks like they both go
back to their initial values.
We think alike :) In the next patches I am fixing this. class_id becomes
a virBitmap and among with floor_sum gets stored into network status
XML. I've split it into several patches because changes are big (in
number of lines at least) and I think this way it's easier to review.
And what about hotplug?
This is a similar problem to the pool of interfaces used by
macvtap/hostdev modes - a network with an interface pool needs to have
the inuse counters of each interface refreshed whenever libvirtd
restarts. So the necessary hooks are already in place:
networkAllocateActualDevice - called any time an interface with
type='network' is initially
allocated and connected.
networkNotifyActualDevice - called for each type='network' interface
of each active domain
any time libvirtd restarts
networkReleaseActualDevice - called for every type='network'
interface as it is being
disconnected and destroyed.
These are called for *all* type='network' interfaces, not just those
that need to allocate a physical netdev from a pool.
Rather than adding your own new hooks (and figuring out all the places
they need to be called), I think it would be better to use the existing
hooks (perhaps calling a reduced/renamed version of your functions,
which can possibly be moved over to ). That will have two advantages:
1) It will assure that the bandwidth functions are called at all the
right times, including hotplug/unplug, and libvirtd restart
2) It will continue the process of consolidating all network-related
functionality need for these three events into 3 functions which may
some day be moved into their own daemom with a public (within libvirt
anyway) API accessible via RPC (thus allowing non-privileged libvirts to
have full networking functionality).
Okay, renamed to networkPlugBandwidth and networkUnplugBandwidth and
inserted into networkAllocateActualDevice and networkReleaseActualDevice
respectively.
Note that you will probably want to save the interface class_id in the
iface-actual (as a matter of fact, in
iface-data.network.actual-bandwidth, which can be retrieved with
virDomainNetGetActualBandwidth()) so that it's saved properly in the
interface's state without polluting the interface's config. Then it will
be read back from the state file during the restart and available during
networkNotifyActualDevice() of each interface. I guess you can then
re-set the network-class_id by checking interface-class_id and setting
network-class_id = MAX(network-class_id, iface-class_id + 1);
for each encountered interface (or add it to a bitmap, if a) bitmaps can
be enlarged dynamically or b) we can determine a reasonable maximum
limit on number of active domains). At the same time, you'll recompute
the network-floor_sum iteratively as the network is called with a
notify for each interface.
...
To summarize: I think this needs to be refactored to be integrated into
the network*ActualDevice() functions so that the bookkeeping is properly
handled in all cases, including hotplug and libvirtd restart.
--
libvir-list mailing list
libvir-list@redhat.com
Re: [libvirt] [PATCH v1 08/11] network: Update status file on (un)plug
On 30.11.2012 21:00, Laine Stump wrote: On 11/19/2012 11:51 AM, Michal Privoznik wrote: Status file keeps track of class_id and floor_sum. It's better to keep it updated in case libvirtd is killed. I'm not sure why you're doing this type of iterative improvement in separate patches. Since you would want this functionality in any working version of the code, and you haven't already pushed the earlier versions, why not just do it in the original patch that introduces these functions? Likewise, doing part of the functionality, then a bit of infrastructure to allow the new functionality to work better, and then another patch to improve the new functionality makes it a bit of a treasure hunt to review; I like to make my patchsets so that the first patches contain all the improvements/changes to existing infrastructure that will be needed, then the following patches introduce the new code, fully functioning from the beginning. Yeah, these two patches can be joined together. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 02/11] bandwidth: Create hierarchical shaping classes
On 30.11.2012 17:52, Laine Stump wrote:
On 11/19/2012 11:51 AM, Michal Privoznik wrote:
These classes can borrow unused bandwidth. Basically,
only egress qdsics can have classes, therefore we can
do this kind of traffic shaping only on host's outgoing,
that is domain's incoming traffic.
---
src/lxc/lxc_process.c |3 ++-
src/network/bridge_driver.c |3 ++-
src/qemu/qemu_command.c |3 ++-
src/qemu/qemu_driver.c|2 +-
src/util/virnetdevbandwidth.c | 33 +++--
src/util/virnetdevbandwidth.h |4 +++-
src/util/virnetdevmacvlan.c |2 +-
7 files changed, 38 insertions(+), 12 deletions(-)
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index 079bc3a..91ce2d3 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -335,7 +335,8 @@ static int
virLXCProcessSetupInterfaceBridged(virConnectPtr conn,
goto cleanup;
if (virNetDevBandwidthSet(net-ifname,
- virDomainNetGetActualBandwidth(net)) 0) {
+ virDomainNetGetActualBandwidth(net),
+ false) 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_(cannot set bandwidth limits on %s),
net-ifname);
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index c153d36..256b601 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -2240,7 +2240,8 @@ networkStartNetworkVirtual(struct network_driver
*driver,
VIR_FORCE_CLOSE(tapfd);
}
-if (virNetDevBandwidthSet(network-def-bridge,
network-def-bandwidth) 0) {
+if (virNetDevBandwidthSet(network-def-bridge,
+ network-def-bandwidth, true) 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_(cannot set bandwidth limits on %s),
network-def-bridge);
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 22bb209..c1f8680 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -292,7 +292,8 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
if (tapfd = 0
virNetDevBandwidthSet(net-ifname,
- virDomainNetGetActualBandwidth(net)) 0) {
+ virDomainNetGetActualBandwidth(net),
+ false) 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_(cannot set bandwidth limits on %s),
net-ifname);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 595c452..9ae0c1b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8960,7 +8960,7 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom,
sizeof(*newBandwidth-out));
}
-if (virNetDevBandwidthSet(net-ifname, newBandwidth) 0) {
+if (virNetDevBandwidthSet(net-ifname, newBandwidth, false) 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_(cannot set bandwidth limits on %s),
device);
diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c
index bddb788..fcc6b56 100644
--- a/src/util/virnetdevbandwidth.c
+++ b/src/util/virnetdevbandwidth.c
@@ -45,17 +45,21 @@ virNetDevBandwidthFree(virNetDevBandwidthPtr def)
* virNetDevBandwidthSet:
* @ifname: on which interface
* @bandwidth: rates to set (may be NULL)
+ * @hierarchical_class: whether to create hierarchical class
*
* This function enables QoS on specified interface
* and set given traffic limits for both, incoming
* and outgoing traffic. Any previous setting get
- * overwritten.
+ * overwritten. If @hierarchical_class is TRUE, create
+ * hierarchical class. It is used to guarantee minimal
+ * throughput ('floor' attribute in NIC).
*
* Return 0 on success, -1 otherwise.
*/
int
virNetDevBandwidthSet(const char *ifname,
- virNetDevBandwidthPtr bandwidth)
+ virNetDevBandwidthPtr bandwidth,
+ bool hierarchical_class)
{
int ret = -1;
virCommandPtr cmd = NULL;
@@ -71,7 +75,7 @@ virNetDevBandwidthSet(const char *ifname,
virNetDevBandwidthClear(ifname);
-if (bandwidth-in) {
+if (bandwidth-in bandwidth-in-average) {
if (virAsprintf(average, %llukbps, bandwidth-in-average) 0)
goto cleanup;
if (bandwidth-in-peak
@@ -83,15 +87,32 @@ virNetDevBandwidthSet(const char *ifname,
cmd = virCommandNew(TC);
virCommandAddArgList(cmd, qdisc, add, dev, ifname, root,
- handle, 1:, htb, default, 1, NULL);
+ handle, 1:, htb, default,
+ hierarchical_class ? 2 : 1, NULL);
Don't you really only need to do
Re: [libvirt] [PATCH v1 04/11] bandwidth: Create rate update function
On 30.11.2012 18:56, Laine Stump wrote:
On 11/19/2012 11:51 AM, Michal Privoznik wrote:
This will be used whenever a NIC with guaranteed throughput is to
be plugged into a bridge. It will adjust the average throughput of
non guaranteed NICs (classid 1:2) to meet new requirements.
---
src/util/virnetdevbandwidth.c | 49
+
src/util/virnetdevbandwidth.h |6 +
2 files changed, 55 insertions(+), 0 deletions(-)
diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c
index 9597dcd..3abe7e2 100644
--- a/src/util/virnetdevbandwidth.c
+++ b/src/util/virnetdevbandwidth.c
@@ -463,3 +463,52 @@ cleanup:
virCommandFree(cmd);
return ret;
}
+
+/**
+ * virNetDevBandwidthUpdateRate:
+ * @ifname: interface name
+ * @classid: ID of class to update
+ * @new_rate: new rate
+ *
+ * This function updates the 'rate' attribute of HTB class.
+ * It can be used whenever a new interface is plugged to a
+ * bridge to adjust average throughput of non guaranteed
+ * NICs.
So it's changing the average for that NIC to something other than what
was configured? Is a record of this kept anywhere? (or maybe it's not
necessary).
Right, but it will be used only for changing the average of the first
child class on the bridge, that is 1:2 which is the class where all not
guaranteed traffic is sent. And yeah - I keep track of it in the next
patches - it's called floor_sum.
+ *
+ * Returns 0 on success, -1 otherwise.
+ */
+int
+virNetDevBandwidthUpdateRate(const char *ifname,
+ const char *class_id,
+ virNetDevBandwidthPtr bandwidth,
+ unsigned long long new_rate)
+{
+int ret = -1;
+virCommandPtr cmd = NULL;
+char *rate = NULL;
+char *ceil = NULL;
+
+if (virAsprintf(rate, %llukbps, new_rate) 0 ||
+virAsprintf(ceil, %llukbps, bandwidth-in-peak ?
+bandwidth-in-peak :
+bandwidth-in-average) 0) {
+virReportOOMError();
+goto cleanup;
+}
+
+cmd = virCommandNew(TC);
+virCommandAddArgList(cmd, class, change, dev, ifname,
+ classid, class_id, htb, rate, rate,
+ ceil, ceil, NULL);
+
+if (virCommandRun(cmd, NULL) 0)
+goto cleanup;
+
+ret = 0;
+
+cleanup:
+virCommandFree(cmd);
+VIR_FREE(rate);
+VIR_FREE(ceil);
+return ret;
+}
diff --git a/src/util/virnetdevbandwidth.h b/src/util/virnetdevbandwidth.h
index 19eb418..a5d595e 100644
--- a/src/util/virnetdevbandwidth.h
+++ b/src/util/virnetdevbandwidth.h
@@ -67,4 +67,10 @@ int virNetDevBandwidthUnplug(const char *brname,
unsigned int id)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
+int virNetDevBandwidthUpdateRate(const char *ifname,
+ const char *class_id,
+ virNetDevBandwidthPtr bandwidth,
+ unsigned long long new_rate)
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
+ATTRIBUTE_RETURN_CHECK;
#endif /* __VIR_NETDEV_BANDWIDTH_H__ */
ACK.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv3 3/3] v7.9: put dnsmasq parameters into conf-file
This patch changes how parameters are passed to dnsmasq. Instead of
being on the command line, the parameters are put into a file (one
parameter per line) and a commandline --conf-file= specifies the
location of the file. The file is located in the same directory as
the leases file.
This also adds the dnsmasq parameter interface=net-name
Putting the dnsmasq parameters into a configuration file
allows them to be examined and more easily understood than
examining the command lines displayed by ps ax. This is
especially true when a number of networks have been started.
I suspect that when the use of dnsmasq was originally done,
the command line was simple but has gotten more complicated
over time and will likely become even more complicated in the
future.
I believe that if use of dnsmasq was done today with
the current requirements for dnsmasq, a configuration file
would have been used. Many (most?) daemons use configuration
files as oppose to large number of command line parameters.
One potential addition to dnsmasq parameters is to specify
the reverse-lookup queries which are not to be passed on up
the chain. For IPv4, such queries are rather simple and
take the form:
ipv4_address.in-addr.arpa
but ipv6 queries involve a lot longer specification. The
IPv6 query will be of the form:
ipv6_address.ip6.arpa
where the above query expands to a 44 character string which is
specified by --local=/ipv6_address.ip6.arpa/ which is
certainly a lot of clutter to add to the command line.
---
src/network/bridge_driver.c| 190 +
src/network/bridge_driver.h| 7 +-
tests/networkxml2argvdata/dhcp6-nat-network.argv | 29 ++--
tests/networkxml2argvdata/dhcp6-network.argv | 29 ++--
.../dhcp6host-routed-network.argv | 25 ++-
tests/networkxml2argvdata/isolated-network.argv| 31 ++--
.../networkxml2argvdata/nat-network-dns-hosts.argv | 19 +--
.../nat-network-dns-srv-record-minimal.argv| 37 ++--
.../nat-network-dns-srv-record.argv| 27 ++-
.../nat-network-dns-txt-record.argv| 26 +--
tests/networkxml2argvdata/nat-network.argv | 29 ++--
tests/networkxml2argvdata/netboot-network.argv | 37 ++--
.../networkxml2argvdata/netboot-proxy-network.argv | 33 ++--
tests/networkxml2argvdata/routed-network.argv | 15 +-
tests/networkxml2argvtest.c| 40 +
15 files changed, 286 insertions(+), 288 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index c07d61a..2ccc695 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -142,6 +142,16 @@ networkDnsmasqLeaseFileNameFunc
networkDnsmasqLeaseFileName =
networkDnsmasqLeaseFileNameDefault;
static char *
+networkDnsmasqConfigFileName(const char *netname)
+{
+char *conffile;
+
+ignore_value(virAsprintf(conffile, DNSMASQ_STATE_DIR /%s.conf,
+ netname));
+return conffile;
+}
+
+static char *
networkRadvdPidfileBasename(const char *netname)
{
/* this is simple but we want to be sure it's consistently done */
@@ -168,6 +178,7 @@ networkRemoveInactive(struct network_driver *driver,
{
char *leasefile = NULL;
char *radvdconfigfile = NULL;
+char *configfile = NULL;
char *radvdpidbase = NULL;
dnsmasqContext *dctx = NULL;
virNetworkDefPtr def = virNetworkObjGetPersistentDef(net);
@@ -187,9 +198,13 @@ networkRemoveInactive(struct network_driver *driver,
if (!(radvdpidbase = networkRadvdPidfileBasename(def-name)))
goto no_memory;
+if (!(configfile = networkDnsmasqConfigFileName(def-name)))
+goto no_memory;
+
/* dnsmasq */
dnsmasqDelete(dctx);
unlink(leasefile);
+unlink(configfile);
/* radvd */
unlink(radvdconfigfile);
@@ -202,6 +217,7 @@ networkRemoveInactive(struct network_driver *driver,
cleanup:
VIR_FREE(leasefile);
+VIR_FREE(configfile);
VIR_FREE(radvdconfigfile);
VIR_FREE(radvdpidbase);
dnsmasqContextFree(dctx);
@@ -635,12 +651,13 @@ networkBuildDnsmasqHostsList(dnsmasqContext *dctx,
static int
-networkBuildDnsmasqArgv(virNetworkObjPtr network,
+networkDnsmasqConfContents(virNetworkObjPtr network,
const char *pidfile,
-virCommandPtr cmd,
+char **configstr,
dnsmasqContext *dctx,
dnsmasqCapsPtr caps ATTRIBUTE_UNUSED)
{
+virBuffer configbuf = VIR_BUFFER_INITIALIZER;
int r, ret = -1;
int nbleases = 0;
int ii;
@@ -651,46 +668,43 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
virNetworkIpDefPtr tmpipdef, ipdef, ipv4def, ipv6def;
bool dhcp4flag, dhcp6flag, ipv6SLAAC;
+*configstr = NULL;
+
/*
- * NB, be careful about syntax for dnsmasq options in long format.
- *
- * If the flag has a
[libvirt] [PATCHv3 0/3] IPv6 enhancements; put dnsmasq parameters in conf-file
Rebased 3 December 2012 These three patch files are packaged together because they serially depend on each other. This version includes updates to add the ipv6='yes' for enabling the guest-to-guest communications. The DHCPv6 support checks dnsmasq's version and requires a minimum of 2.64. Also, using dnsmasq for providing the RA service is checked against the dnsmasq version and is currently 2.64. There are separate checks for DHCPv6 and Router Advertising support by dnsmasq. As with IPv4, IPv6 DHCP is only one subnetwork on an interface. Additionally, if other IPv6 addresses are defined, a warning message is issued since the Router Advertisement service will support only state-full (DHCP) or state-less (SLAAC) addressing on a network interface (not both). Thus, the additional subnetworks will need to be manually configured to properly function. If dnsmasq provides the RA service, it also points to itself as a RDNSS (Recursive DNS Server) as part of the information is supplies. If IPv6 DHCP is not being run, then SLAAC addressing is supported for any IPv6 addresses specified. Gene Czarcinski (3): v2.0: allow guest to guest IPv6 without gateway definition v8.2 add support for DHCPv6 v7.9: put dnsmasq parameters into conf-file docs/formatnetwork.html.in | 136 - docs/schemas/network.rng | 22 +- src/conf/network_conf.c| 108 ++-- src/conf/network_conf.h| 5 + src/network/bridge_driver.c| 594 ++--- src/network/bridge_driver.h| 7 +- src/util/dnsmasq.c | 9 +- tests/networkxml2argvdata/dhcp6-nat-network.argv | 14 + tests/networkxml2argvdata/dhcp6-nat-network.xml| 24 + tests/networkxml2argvdata/dhcp6-network.argv | 14 + tests/networkxml2argvdata/dhcp6-network.xml| 14 + .../dhcp6host-routed-network.argv | 12 + .../dhcp6host-routed-network.xml | 19 + tests/networkxml2argvdata/isolated-network.argv| 25 +- .../networkxml2argvdata/nat-network-dns-hosts.argv | 14 +- .../nat-network-dns-srv-record-minimal.argv| 34 +- .../nat-network-dns-srv-record.argv| 24 +- .../nat-network-dns-txt-record.argv| 22 +- tests/networkxml2argvdata/nat-network.argv | 22 +- tests/networkxml2argvdata/netboot-network.argv | 28 +- .../networkxml2argvdata/netboot-proxy-network.argv | 26 +- tests/networkxml2argvdata/routed-network.argv | 11 +- tests/networkxml2argvtest.c| 47 +- 23 files changed, 876 insertions(+), 355 deletions(-) create mode 100644 tests/networkxml2argvdata/dhcp6-nat-network.argv create mode 100644 tests/networkxml2argvdata/dhcp6-nat-network.xml create mode 100644 tests/networkxml2argvdata/dhcp6-network.argv create mode 100644 tests/networkxml2argvdata/dhcp6-network.xml create mode 100644 tests/networkxml2argvdata/dhcp6host-routed-network.argv create mode 100644 tests/networkxml2argvdata/dhcp6host-routed-network.xml -- 1.7.11.7 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv3 1/3] v2.0: allow guest to guest IPv6 without gateway definition
This patch adds the capability for virtual guests to do IPv6
communication via a virtual network interface with no IPv6
(gateway) addresses specified. This capability currently
exists for IPv4.
This patch allows creation of a completely isolated IPv6 network.
Note that virtual guests cannot communication with the virtualization
host via this interface. Also note that:
net.ipv6.conf.interface_name.disable_ipv6 = 1
Also not that starting libvirtd has set the following:
net.bridge.bridge-nf-call-arptables = 1
net.bridge.bridge-nf-call-ip6tables = 1
net.bridge.bridge-nf-call-iptables = 1
although /etc/syslog.conf has them all set to 0.
To control this behavior so that it is not enabled by default, the parameter
ipv6='yes' on the network statement has been added.
Documentation related to this patch has been updated.
The network schema has also been updated.
---
docs/formatnetwork.html.in | 28 +++-
docs/schemas/network.rng| 10 ++
src/conf/network_conf.c | 8
src/conf/network_conf.h | 5 +
src/network/bridge_driver.c | 25 -
5 files changed, 70 insertions(+), 6 deletions(-)
diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
index 49206dd..a3a5ced 100644
--- a/docs/formatnetwork.html.in
+++ b/docs/formatnetwork.html.in
@@ -33,7 +33,7 @@
/p
pre
- lt;networkgt;
+ lt;network ipv6='yes'gt;
lt;namegt;defaultlt;/namegt;
lt;uuidgt;3e3fce45-4f53-4fa7-bb32-11f34168b82blt;/uuidgt;
.../pre
@@ -52,6 +52,12 @@
The format must be RFC 4122 compliant, eg
code3e3fce45-4f53-4fa7-bb32-11f34168b82b/code.
If omitted when defining/creating a new network, a random
UUID is generated. span class=sinceSince 0.3.0/span/dd
+ dtcodeipv6='yes'/code/dt
+ ddThe new, optional parameter codeipv6='yes'/code enables
+a network definition with no IPv6 gateway addresses specified
+to have guest-to-guest communications. For further information,
+see the example below for the example with no gateway addresses.
+span class=sinceSince 1.0.1/span/dd
/dl
h3a name=elementsConnectConnectivity/a/h3
@@ -773,5 +779,25 @@
lt;/forwardgt;
lt;/networkgt;/pre
+h3a name=examplesNoGatewayNetwork config with no gateway
addresses/a/h3
+
+p
+A valid network definition can contain no IPv4 or IPv6 addresses. Such a
definition
+can be used for a very private or very isolated network since it will
not be
+possible to communicate with the virtualization host via this network.
However,
+this virtual network interface can be used for communication between
virtual guest
+systems. This works for IPv4 and span class=since(Since 1.0.1)/span
IPv6.
+However, the new ipv6='yes' must be added for guest-to-guest IPv6
+communication.
+/p
+
+pre
+ lt;network ipv6='yes'gt;
+lt;namegt;nogwlt;/namegt;
+lt;uuidgt;7a3b7497-1ec7-8aef-6d5c-38dff9109e93lt;/uuidgt;
+lt;bridge name=virbr2 stp=on delay=0 /gt;
+lt;mac address='00:16:3E:5D:C7:9E'/gt;
+ lt;/networkgt;/pre
+
/body
/html
diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng
index 4abfd91..0d67f7f 100644
--- a/docs/schemas/network.rng
+++ b/docs/schemas/network.rng
@@ -17,6 +17,16 @@
data type=unsignedInt/
/attribute
/optional
+ !-- Enables IPv6 guest-to-guest communications on a network
+ with no gateways addresses specified --
+ optional
+attribute name=ipv6
+ choice
+ valueyes/value
+ valueno/value
+ /choice
+ /attribute
+ /optional
interleave
!-- The name of the network, used to refer to it through the API
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 6ce2e63..3f9e13c 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -1264,6 +1264,12 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
def-uuid_specified = true;
}
+/* check if definitions with no IPv6 gateway addresses is to
+ * allow guest-to-guest communications.
+ */
+if (virXPathBoolean(boolean(./@ipv6), ctxt) == 1)
+def-ipv6nogw = true;
+
/* Parse network domain information */
def-domain = virXPathString(string(./domain[1]/@name), ctxt);
@@ -1839,6 +1845,8 @@ char *virNetworkDefFormat(const virNetworkDefPtr def,
unsigned int flags)
if (!(flags VIR_NETWORK_XML_INACTIVE) (def-connections 0)) {
virBufferAsprintf(buf, connections='%d', def-connections);
}
+if (def-ipv6nogw)
+virBufferAddLit(buf, ipv6='yes');
virBufferAddLit(buf, \n);
virBufferAdjustIndent(buf, 2);
virBufferEscapeString(buf, name%s/name\n, def-name);
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
index 3e46304..949b3d2 100644
--- a/src/conf/network_conf.h
+++
[libvirt] RFC: Coding style: require alphabetical header file sorting
Thus far we only have one rule about header files, #include config.h must be first in all .c files. I'm wondering if it is time to introduce some new rules - All system headers must be grouped to preceed all local headers - All system headers must be sorted within their group - All local headers must be sorted within their group This will require updating pretty much every single source and header file in the tree. Of course it will need a new syntax check rule to validate this too. Since fixing them is a serious amount of work, I was wondering about people's opinions on this ? The goal is just standardization to make code a little more readable. 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] RFC: Coding style: require alphabetical header file sorting
Daniel P. Berrange berra...@redhat.com writes: Thus far we only have one rule about header files, #include config.h must be first in all .c files. I'm wondering if it is time to introduce some new rules - All system headers must be grouped to preceed all local headers - All system headers must be sorted within their group - All local headers must be sorted within their group This will require updating pretty much every single source and header file in the tree. Of course it will need a new syntax check rule to validate this too. Since fixing them is a serious amount of work, I was wondering about people's opinions on this ? The goal is just standardization to make code a little more readable. Daniel I've recently been trying to find my way around the codebase for the first time, +1 to this. In fact just this morning I was reading the hacking guidelines trying to find this, because I expected it to be there. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] conf: fix virDomainNetGetActualDirect*() and BridgeName()
This resolves:
https://bugzilla.redhat.com/show_bug.cgi?id=881480
These three functions:
virDomainNetGetActualBridgeName
virDomainNetGetActualDirectDev
virDomainNetGetActualDirectMode
return attributes that are in a union whose contents are interpreted
differently depending on the actual-type and so they should only
return non-0 when actual-type is 'bridge' (in the first case) or
'direct' (in the other two cases, but I had neglected to do that, so
...DirectDev() was returning bridge.brname (which happens to share the
same spot in the union with direct.linkdev) if actual-type was
'bridge', and ...BridgeName was returning direct.linkdev when
actual-type was 'direct'.
How does this involve Bug 881480 (which was about the inability to
switch between two networks that both have forward mode='bridge'/
bridge name='xxx'/? Whenever the return value of
virDomainNetGetActualDirectDev() for the new and old network
definitions doesn't match, qemuDomainChangeNet() requires a complete
reconnect of the device, which qemu currently doesn't
support. ...DirectDev() *should* have been returning NULL for old and
new, but was instead returning the old and new bridge names, which
differ.
(The other two functions weren't causing any behavioral problems in
virDomainChangeNet(), but their problem and fix was identical, so I
included them in this same patch).
were supposed to only return non-0 values when
---
src/conf/domain_conf.c | 33 ++---
1 file changed, 18 insertions(+), 15 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 814859a..4ffb39d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15130,11 +15130,12 @@ virDomainNetGetActualBridgeName(virDomainNetDefPtr
iface)
{
if (iface-type == VIR_DOMAIN_NET_TYPE_BRIDGE)
return iface-data.bridge.brname;
-if (iface-type != VIR_DOMAIN_NET_TYPE_NETWORK)
-return NULL;
-if (!iface-data.network.actual)
-return NULL;
-return iface-data.network.actual-data.bridge.brname;
+if (iface-type == VIR_DOMAIN_NET_TYPE_NETWORK
+iface-data.network.actual
+iface-data.network.actual-type == VIR_DOMAIN_NET_TYPE_BRIDGE) {
+return iface-data.network.actual-data.bridge.brname;
+}
+return NULL;
}
const char *
@@ -15142,11 +15143,12 @@ virDomainNetGetActualDirectDev(virDomainNetDefPtr
iface)
{
if (iface-type == VIR_DOMAIN_NET_TYPE_DIRECT)
return iface-data.direct.linkdev;
-if (iface-type != VIR_DOMAIN_NET_TYPE_NETWORK)
-return NULL;
-if (!iface-data.network.actual)
-return NULL;
-return iface-data.network.actual-data.direct.linkdev;
+if (iface-type == VIR_DOMAIN_NET_TYPE_NETWORK
+iface-data.network.actual
+iface-data.network.actual-type == VIR_DOMAIN_NET_TYPE_DIRECT) {
+return iface-data.network.actual-data.direct.linkdev;
+}
+return NULL;
}
int
@@ -15154,11 +15156,12 @@ virDomainNetGetActualDirectMode(virDomainNetDefPtr
iface)
{
if (iface-type == VIR_DOMAIN_NET_TYPE_DIRECT)
return iface-data.direct.mode;
-if (iface-type != VIR_DOMAIN_NET_TYPE_NETWORK)
-return 0;
-if (!iface-data.network.actual)
-return 0;
-return iface-data.network.actual-data.direct.mode;
+if (iface-type == VIR_DOMAIN_NET_TYPE_NETWORK
+iface-data.network.actual
+iface-data.network.actual-type == VIR_DOMAIN_NET_TYPE_DIRECT) {
+return iface-data.network.actual-data.direct.mode;
+}
+return 0;
}
virDomainHostdevDefPtr
--
1.7.11.7
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: fix virDomainNetGetActualDirect*() and BridgeName()
This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=881480 These three functions: virDomainNetGetActualBridgeName virDomainNetGetActualDirectDev virDomainNetGetActualDirectMode return attributes that are in a union whose contents are interpreted differently depending on the actual-type and so they should only return non-0 when actual-type is 'bridge' (in the first case) or 'direct' (in the other two cases, but I had neglected to do that, (The other two functions weren't causing any behavioral problems in virDomainChangeNet(), but their problem and fix was identical, so I included them in this same patch). were supposed to only return non-0 values when Spurious line in the commit message. --- src/conf/domain_conf.c | 33 ++--- 1 file changed, 18 insertions(+), 15 deletions(-) ACK. -- Eric Blake ebl...@redhat.com+1-919-301-3266 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: fix virDomainNetGetActualDirect*() and BridgeName()
On 12/03/2012 01:10 PM, Eric Blake wrote: This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=881480 These three functions: virDomainNetGetActualBridgeName virDomainNetGetActualDirectDev virDomainNetGetActualDirectMode return attributes that are in a union whose contents are interpreted differently depending on the actual-type and so they should only return non-0 when actual-type is 'bridge' (in the first case) or 'direct' (in the other two cases, but I had neglected to do that, (The other two functions weren't causing any behavioral problems in virDomainChangeNet(), but their problem and fix was identical, so I included them in this same patch). were supposed to only return non-0 values when Spurious line in the commit message. Oops. Forgot to delete that. I removed it before I pushed. Thanks! -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 0/2] qemu: support live update of an interface's filter
On 11/29/2012 01:30 PM, Laine Stump wrote: These two patches enable making a live change to the nwfilter of a guest's interface via virDomainUpdateDeviceFlags (virsh update-device). Differences from V1: 1) add patch from Stefan Berger to do a proper comparison of the values stored in the filterparams hashtable. 2) simplify virNWFilterHashTableEqual to use Stefan's new function, and remove a couple of pointless comparisons based on Stefan's review. I've pushed this now. Thanks to Stefan and Guido for help in writing and testing the patch, and to Michal for reviewing it. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] libvirt [PATCHv2 2/2] Add iSCSI backend storage driver for ESX.
2012/11/10 Ata E Husain Bohra ata.hus...@hotmail.com:
The patch adds the backend driver to support iSCSI format storage pools
and volumes for ESX host. The mapping of ESX iSCSI specifics to Libvirt
is as follows:
1. ESX static iSCSI target -- Libvirt Storage Pools
2. ESX iSCSI LUNs -- Libvirt Storage Volumes.
The above understanding is based on http://libvirt.org/storage.html.
The operation supported on iSCSI pools includes:
1. List storage pools volumes.
2. Get xml descriptor operaion on pools volumes.
3. Lookup operation on pools volumes by name, uuid and path (if applicable).
iSCSI pools does not support operations such as: Create / remove pools
and volumes.
---
src/Makefile.am |1 +
src/esx/esx_storage_backend_iscsi.c | 807
+++
src/esx/esx_storage_backend_iscsi.h | 29 ++
src/esx/esx_storage_driver.c|8 +-
src/esx/esx_vi.c| 332 ++
src/esx/esx_vi.h| 18 +
src/esx/esx_vi_generator.input | 302 +
src/esx/esx_vi_generator.py | 19 +
8 files changed, 1515 insertions(+), 1 deletion(-)
create mode 100644 src/esx/esx_storage_backend_iscsi.c
create mode 100644 src/esx/esx_storage_backend_iscsi.h
--- /dev/null
+++ b/src/esx/esx_storage_backend_iscsi.c
+static int
+esxStorageBackendISCSIPoolListStorageVolumes(virStoragePoolPtr pool,
+ char **const names,
+ int maxnames)
+{
+int count = 0;
+esxPrivate *priv = pool-conn-storagePrivateData;
+esxVI_HostScsiTopologyLun *hostScsiTopologyLunList = NULL;
+const esxVI_HostScsiTopologyLun *hostScsiTopologyLun = NULL;
+esxVI_ScsiLun *scsiLunList = NULL;
+const esxVI_ScsiLun *scsiLun = NULL;
+bool success = false;
+int i = 0;
+
+if (esxVI_LookupHostScsiTopologyLunListByTargetName(
+ priv-primary, pool-name, hostScsiTopologyLunList) 0) {
+goto cleanup;
+}
+
+if (hostScsiTopologyLunList == NULL) {
+/* iSCSI adapter may not be enabled on ESX host */
+return 0;
+}
+
+if (esxVI_LookupScsiLunList(priv-primary, scsiLunList) 0) {
+goto cleanup;
+}
+
+/* O^2 but still faster than hash given N is not that large */
This comment is wrong here, because a hash wouldn't help here. In
order to get the storage volume list this 2-dimensional list need to
be iterated. There is no way to do this faster, except adding a cache
but then you get the problem of ehen to invalidate it. So this 2 level
for loop is totally fine and the comment should be removed, because it
is misleading.
+for (scsiLun = scsiLunList; scsiLun != NULL count maxnames;
+ scsiLun = scsiLun-_next) {
+for (hostScsiTopologyLun = hostScsiTopologyLunList;
+ hostScsiTopologyLun != NULL count maxnames;
+ hostScsiTopologyLun = hostScsiTopologyLun-_next) {
+if (STREQ(hostScsiTopologyLun-scsiLun, scsiLun-key)) {
+names[count] = strdup(scsiLun-deviceName);
+
+if (names[count] == NULL) {
+virReportOOMError();
+goto cleanup;
+}
+++count;
+}
+}
+}
+
+success = true;
+
+ cleanup:
+if (! success) {
+for (i = 0; i count; ++i) {
+VIR_FREE(names[i]);
+}
+count = -1;
+}
+
+esxVI_HostScsiTopologyLun_Free(hostScsiTopologyLunList);
+esxVI_ScsiLun_Free(scsiLunList);
+
+return count;
+}
+static int
+esxStorageBackendISCSIPoolRefresh(virStoragePoolPtr pool,
+ unsigned int flags)
+{
+int result = -1;
+esxPrivate *priv = pool-conn-storagePrivateData;
+esxVI_ManagedObjectReference *hostStorageSystem = NULL;
+esxVI_HostInternetScsiHba *hostInternetScsiHba = NULL;
+esxVI_ObjectContent *hostSystem = NULL;
+esxVI_String *propertyNameList = NULL;
+
+virCheckFlags(0, -1);
+
+if (esxVI_String_AppendValueToList(propertyNameList,
+ configManager.storageSystem\0) 0 ||
+esxVI_LookupHostSystemProperties(priv-primary,
+propertyNameList, hostSystem) 0 ||
+esxVI_GetManagedObjectReference(hostSystem,
+configManager.storageSystem, hostStorageSystem,
+esxVI_Occurrence_RequiredItem) 0 ||
No need to look up the hostStorageSystem here. It is already available
here: ctx-hostSystem-configManager-storageSystem
+esxVI_LookupHostInternetScsiHba(
+priv-primary, hostInternetScsiHba) 0) {
+goto cleanup;
+}
+
+ /**
+ * ESX does not allow rescan on a particular target,
+ * rescan all the static targets
+ */
+if (esxVI_RescanHba(priv-primary, hostStorageSystem,
+
[libvirt] [PATCH] build: fix incremental autogen.sh when no AUTHORS is present
Commit 71d1256 tried to fix a problem where rebasing an old
branch on top of newer libvirt.git resulted in automake failing
because of a missing AUTHORS file. However, while the fix
worked for an incremental 'make', it did not work for someone
that directly reran './autogen.sh'. Reported by Laine Stump.
* autogen.sh (autoreconf): Check for same conditions as cfg.mk.
* cfg.mk (_update_required): Add comments.
---
autogen.sh |3 ++-
cfg.mk |2 ++
2 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/autogen.sh b/autogen.sh
index 126b272..d4957f1 100755
--- a/autogen.sh
+++ b/autogen.sh
@@ -69,14 +69,15 @@ if test -d .git; then
*:set) ;;
*-dirty*)
echo error: gnulib submodule is dirty, please investigate 21
echo set env-var CLEAN_SUBMODULE to discard gnulib changes 21
exit 1 ;;
esac
+# Keep this test in sync with cfg.mk:_update_required
if test $t = $(cat $curr_status 2/dev/null) \
- test -f po/Makevars; then
+ test -f po/Makevars test -f AUTHORS; then
# good, it's up to date, all we need is autoreconf
autoreconf -if
else
if test ${CLEAN_SUBMODULE+set}; then
echo cleaning up submodules...
git submodule foreach 'git clean -dfqx git reset --hard'
diff --git a/cfg.mk b/cfg.mk
index ec4ab1c..c4ae195 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -688,12 +688,14 @@ ifeq (0,$(MAKELEVEL))
# in column 1 and does not print a git describe-style string after the
# submodule name. Contrast these:
# -b653eda3ac4864de205419d9f41eec267cb89eeb .gnulib
# b653eda3ac4864de205419d9f41eec267cb89eeb .gnulib (v0.0-2286-gb653eda)
# $ cat .git-module-status
# b653eda3ac4864de205419d9f41eec267cb89eeb
+ #
+ # Keep this logic in sync with autogen.sh.
_submodule_hash = sed 's/^[ +-]//;s/ .*//'
_update_required := $(shell \
cd '$(srcdir)'; \
test -d .git || { echo 0; exit; }; \
test -f po/Makevars || { echo 1; exit; };
\
test -f AUTHORS || { echo 1; exit; };\
--
1.7.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] build: fix incremental autogen.sh when no AUTHORS is present
On 12/03/2012 03:14 PM, Eric Blake wrote:
Commit 71d1256 tried to fix a problem where rebasing an old
branch on top of newer libvirt.git resulted in automake failing
because of a missing AUTHORS file. However, while the fix
worked for an incremental 'make', it did not work for someone
that directly reran './autogen.sh'. Reported by Laine Stump.
* autogen.sh (autoreconf): Check for same conditions as cfg.mk.
* cfg.mk (_update_required): Add comments.
---
autogen.sh |3 ++-
cfg.mk |2 ++
2 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/autogen.sh b/autogen.sh
index 126b272..d4957f1 100755
--- a/autogen.sh
+++ b/autogen.sh
@@ -69,14 +69,15 @@ if test -d .git; then
*:set) ;;
*-dirty*)
echo error: gnulib submodule is dirty, please investigate 21
echo set env-var CLEAN_SUBMODULE to discard gnulib changes 21
exit 1 ;;
esac
+# Keep this test in sync with cfg.mk:_update_required
if test $t = $(cat $curr_status 2/dev/null) \
- test -f po/Makevars; then
+ test -f po/Makevars test -f AUTHORS; then
# good, it's up to date, all we need is autoreconf
autoreconf -if
else
if test ${CLEAN_SUBMODULE+set}; then
echo cleaning up submodules...
git submodule foreach 'git clean -dfqx git reset --hard'
diff --git a/cfg.mk b/cfg.mk
index ec4ab1c..c4ae195 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -688,12 +688,14 @@ ifeq (0,$(MAKELEVEL))
# in column 1 and does not print a git describe-style string after the
# submodule name. Contrast these:
# -b653eda3ac4864de205419d9f41eec267cb89eeb .gnulib
# b653eda3ac4864de205419d9f41eec267cb89eeb .gnulib (v0.0-2286-gb653eda)
# $ cat .git-module-status
# b653eda3ac4864de205419d9f41eec267cb89eeb
+ #
+ # Keep this logic in sync with autogen.sh.
_submodule_hash = sed 's/^[ +-]//;s/ .*//'
_update_required := $(shell
\
cd '$(srcdir)';
\
test -d .git || { echo 0; exit; }; \
test -f po/Makevars || { echo 1; exit; };
\
test -f AUTHORS || { echo 1; exit; }; \
My local tree isn't in a state that I can conveniently test it right
now, but it looks correct to me. ACK.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv3 1/3] v2.0: allow guest to guest IPv6 without gateway definition
On 12/03/2012 11:13 AM, Gene Czarcinski wrote:
This patch adds the capability for virtual guests to do IPv6
communication via a virtual network interface with no IPv6
(gateway) addresses specified. This capability currently
exists for IPv4.
This patch allows creation of a completely isolated IPv6 network.
Note that virtual guests cannot communication with the virtualization
host via this interface. Also note that:
net.ipv6.conf.interface_name.disable_ipv6 = 1
Also not that starting libvirtd has set the following:
net.bridge.bridge-nf-call-arptables = 1
net.bridge.bridge-nf-call-ip6tables = 1
net.bridge.bridge-nf-call-iptables = 1
although /etc/syslog.conf has them all set to 0.
To control this behavior so that it is not enabled by default, the parameter
ipv6='yes' on the network statement has been added.
Documentation related to this patch has been updated.
The network schema has also been updated.
---
docs/formatnetwork.html.in | 28 +++-
docs/schemas/network.rng| 10 ++
src/conf/network_conf.c | 8
src/conf/network_conf.h | 5 +
src/network/bridge_driver.c | 25 -
5 files changed, 70 insertions(+), 6 deletions(-)
diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
index 49206dd..a3a5ced 100644
--- a/docs/formatnetwork.html.in
+++ b/docs/formatnetwork.html.in
@@ -33,7 +33,7 @@
/p
pre
- lt;networkgt;
+ lt;network ipv6='yes'gt;
lt;namegt;defaultlt;/namegt;
lt;uuidgt;3e3fce45-4f53-4fa7-bb32-11f34168b82blt;/uuidgt;
.../pre
@@ -52,6 +52,12 @@
The format must be RFC 4122 compliant, eg
code3e3fce45-4f53-4fa7-bb32-11f34168b82b/code.
If omitted when defining/creating a new network, a random
UUID is generated. span class=sinceSince 0.3.0/span/dd
+ dtcodeipv6='yes'/code/dt
+ ddThe new, optional parameter codeipv6='yes'/code enables
+a network definition with no IPv6 gateway addresses specified
+to have guest-to-guest communications. For further information,
+see the example below for the example with no gateway addresses.
+span class=sinceSince 1.0.1/span/dd
/dl
h3a name=elementsConnectConnectivity/a/h3
@@ -773,5 +779,25 @@
lt;/forwardgt;
lt;/networkgt;/pre
+h3a name=examplesNoGatewayNetwork config with no gateway
addresses/a/h3
+
+p
+A valid network definition can contain no IPv4 or IPv6 addresses. Such
a definition
+can be used for a very private or very isolated network since it
will not be
+possible to communicate with the virtualization host via this network.
However,
+this virtual network interface can be used for communication between
virtual guest
+systems. This works for IPv4 and span class=since(Since
1.0.1)/span IPv6.
+However, the new ipv6='yes' must be added for guest-to-guest IPv6
+communication.
+/p
I reformatted this paragraph to fit within 80 columns.
+
+pre
+ lt;network ipv6='yes'gt;
+lt;namegt;nogwlt;/namegt;
+lt;uuidgt;7a3b7497-1ec7-8aef-6d5c-38dff9109e93lt;/uuidgt;
+lt;bridge name=virbr2 stp=on delay=0 /gt;
+lt;mac address='00:16:3E:5D:C7:9E'/gt;
+ lt;/networkgt;/pre
+
/body
/html
diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng
index 4abfd91..0d67f7f 100644
--- a/docs/schemas/network.rng
+++ b/docs/schemas/network.rng
@@ -17,6 +17,16 @@
data type=unsignedInt/
/attribute
/optional
+ !-- Enables IPv6 guest-to-guest communications on a network
+ with no gateways addresses specified --
+ optional
+attribute name=ipv6
+ choice
+ valueyes/value
+ valueno/value
+ /choice
+ /attribute
+ /optional
interleave
!-- The name of the network, used to refer to it through the API
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 6ce2e63..3f9e13c 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -1264,6 +1264,12 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
def-uuid_specified = true;
}
+/* check if definitions with no IPv6 gateway addresses is to
+ * allow guest-to-guest communications.
+ */
+if (virXPathBoolean(boolean(./@ipv6), ctxt) == 1)
+def-ipv6nogw = true;
I don't think virXPathBoolean does what you think it does (although I
haven't figured out exactly what it *does* do, I've noticed that 1) none
of the rest of libvirt uses it for yes/no attributes, and 2) in the
libxml2 source code, when an XPATH_BOOLEAN is formatted into a string,
it becomes True or False. I'm changing the above lines to the following:
ipv6nogwStr = virXPathString(string(./@ipv6), ctxt);
if (ipv6nogwStr) {
if
Re: [libvirt] [PATCH v1 09/11] network: Allow class ID to be reused
On 12/03/2012 10:55 AM, Michal Privoznik wrote: On 30.11.2012 21:55, Laine Stump wrote: On 11/19/2012 11:51 AM, Michal Privoznik wrote: With current implementation, class ID is just incremented. This can lead to its exhaustion as tc accepts only 16 bits long identifiers. Hence, it's better if we allow class ID to be reused. To keep track which IDs are free and which are taken virBitmap is used. This requires network status file to change a bit: from class_id next=5/ to class_id bitmap=0-4/. But since the previous format hasn't been released, it doesn't really matter. Heh. Well, there you have it. :-) You've already implemented what I suggested in the review of 5/10. But rather than introducing one implementation that we need to review, then almost immediately replacing it with something else, why not just implement it this way to begin with? Because I think 5/10 is big enough already :) I actually don't mind large patches, as long as they're not mixing up a bunch of unrelated stuff. (And I don't mind split patches *too* much as long as a later patch doesn't undo too much stuff that was just put in with an earlier patch.) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 03/11] bandwidth: Create (un)plug functions
On 12/03/2012 10:55 AM, Michal Privoznik wrote:
On 30.11.2012 18:53, Laine Stump wrote:
As I'm looking at all these uses of id's, I'm wondering if there's
any danger of namespace conflict with other users of tc. (This isn't
any critique, just curiousity).
No, class ID is specific within an NIC. That is, classID of 3 on eth0
doesn't interfere with class on vnet0. In fact, they have nothing in
common. What we could interfere with is - if user sets something
afterwards on fully libvirt managed interface. But I guess we don't
support this, right? It's all or nothing approach to me.
Correct. I was only wondering if the use of the same class_id on a
different interface would cause interference. If it doesn't then nothing
to worry about :-)
+{
+int ret = -1;
+int cmd_ret = 0;
+virCommandPtr cmd = NULL;
+char *class_id = NULL;
+char *qdisc_id = NULL;
+char *filter_id = NULL;
+
+if (virAsprintf(class_id, 1:%x, id) 0 ||
+virAsprintf(qdisc_id, %x:, id) 0 ||
+virAsprintf(filter_id, %u, id) 0) {
+virReportOOMError();
+goto cleanup;
+}
+
+cmd = virCommandNew(TC);
+virCommandAddArgList(cmd, qdisc, del, dev, brname,
+ handle, qdisc_id, NULL);
+
+/* Don't threat tc errors as fatal, but
+ * try to remove as much as possible */
What's your definition of fatal? In this case, if tc fails you return
-1, not 0.
No, the return value of tc is stored into cmd_ret. Since we are not
passing a NULL here, virCommandRun should fail if something went really
wrong - e.g. OOM, or a pipe couldn't be created, and so on.
Right. I missed the presence of cmd_ret.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 06/11] network: Create real network status files
On 12/03/2012 10:55 AM, Michal Privoznik wrote: On 30.11.2012 20:04, Laine Stump wrote: On 11/19/2012 11:51 AM, Michal Privoznik wrote: Currently, we are only keeping a inactive XML configuration in status dir. This is no longer enough as we need to keep this class_id attribute so we don't overwrite old entries when the daemon restarts. Aha! So you're looking at solving the problem in a different way - save everything to the status file rather than recomputing it as you restart. While I like the idea of having the network status file hold this information, I think its reliability is suspect. What if a guest's process is terminated while libvirtd isn't running? Or what if libvirtd exits unexpectedly after the commands to setup bandwidth have been executed, but before the new network state file has been written (or vice versa, depending on the code). Also, networks aren't properly shut down when the host is being shutdown (there's no equivalent to the libvirt-guests service, although at least one person a month or two ago requested it). If guest is being shut down, the networkReleaseActualDevice() is called, isn't it? And this should update the floor_sum. Even if libvirtd is restarted and qemu process dies meanwhile, qemuProcessStop is called and this calls the networkReleaseActualDevice() so I think we are safe here. If you've tested this and it works properly, then okay. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: nicer error message if live disk snapshot unsupported
Without this patch, attempts to create a disk snapshot when qemu
is too old results in a cryptic message:
virsh # snapshot-create 23 --disk-only
error: operation failed: Failed to take snapshot: unknown command:
'snapshot_blkdev'
Now it reports:
virsh # snapshot-create 23 --disk-only
error: unsupported configuration: live disk snapshot not supported with this
QEMU binary
All versions of qemu that support live disk snapshot also support
QMP (basically upstream qemu 1.1 and later, and backport to RHEL 6.2).
* src/qemu/qemu_capabilities.h (QEMU_CAPS_DISK_SNAPSHOT): New
capability.
* src/qemu/qemu_capabilities.c (qemuCaps): Track it.
(qemuCapsProbeQMPCommands): Set it.
* src/qemu/qemu_driver.c (qemuDomainSnapshotCreateDiskActive): Use
it.
* src/qemu/qemu_monitor.c (qemuMonitorDiskSnapshot): Simplify.
* src/qemu/qemu_monitor_json.c (qemuMonitorJSONDiskSnapshot):
Likewise.
* src/qemu/qemu_monitor_text.h (qemuMonitorTextDiskSnapshot):
Delete.
* src/qemu/qemu_monitor_text.c (qemuMonitorTextDiskSnapshot):
Likewise.
---
src/qemu/qemu_capabilities.c |3 +++
src/qemu/qemu_capabilities.h |1 +
src/qemu/qemu_driver.c |5 +
src/qemu/qemu_monitor.c | 13 -
src/qemu/qemu_monitor_json.c |6 --
src/qemu/qemu_monitor_text.c | 37 -
src/qemu/qemu_monitor_text.h |4
7 files changed, 13 insertions(+), 56 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 6e34cdf..668935e 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -193,6 +193,7 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST,
drive-mirror, /* 115 */
usb-redir.bootindex,
usb-host.bootindex,
+ blockdev-snapshot-sync,
);
struct _qemuCaps {
@@ -1948,6 +1949,8 @@ qemuCapsProbeQMPCommands(qemuCapsPtr caps,
qemuCapsSet(caps, QEMU_CAPS_VNC);
else if (STREQ(name, drive-mirror))
qemuCapsSet(caps, QEMU_CAPS_DRIVE_MIRROR);
+else if (STREQ(name, blockdev-snapshot-sync))
+qemuCapsSet(caps, QEMU_CAPS_DISK_SNAPSHOT);
VIR_FREE(name);
}
VIR_FREE(commands);
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index f420c43..3da8672 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -155,6 +155,7 @@ enum qemuCapsFlags {
QEMU_CAPS_DRIVE_MIRROR = 115, /* drive-mirror monitor command */
QEMU_CAPS_USB_REDIR_BOOTINDEX = 116, /* usb-redir.bootindex */
QEMU_CAPS_USB_HOST_BOOTINDEX = 117, /* usb-host.bootindex */
+QEMU_CAPS_DISK_SNAPSHOT = 118, /* blockdev-snapshot-sync command */
QEMU_CAPS_LAST, /* this must always be the last item */
};
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8e838cd..fbacd8b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11230,6 +11230,11 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr
driver,
virReportOOMError();
goto cleanup;
}
+} else if (!qemuCapsGet(priv-caps, QEMU_CAPS_DISK_SNAPSHOT)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
+ _(live disk snapshot not supported with this
+ QEMU binary));
+goto cleanup;
}
/* No way to roll back if first disk succeeds but later disks
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index f85bb76..543f714 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2769,17 +2769,12 @@ qemuMonitorDiskSnapshot(qemuMonitorPtr mon,
virJSONValuePtr actions,
return -1;
}
-if (mon-json) {
+if (mon-json)
ret = qemuMonitorJSONDiskSnapshot(mon, actions, device, file, format,
reuse);
-} else {
-if (actions || STRNEQ(format, qcow2) || reuse) {
-virReportError(VIR_ERR_INVALID_ARG, %s,
- _(text monitor lacks several snapshot features));
-return -1;
-}
-ret = qemuMonitorTextDiskSnapshot(mon, device, file);
-}
+else
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
+ _(disk snapshot requires JSON monitor));
return ret;
}
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 9b6702a..0cd66b6 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -2986,12 +2986,6 @@ qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon,
virJSONValuePtr actions,
if ((ret = qemuMonitorJSONCommand(mon, cmd, reply)) 0)
goto cleanup;
-if (qemuMonitorJSONHasError(reply, CommandNotFound)) {
-VIR_DEBUG(blockdev-snapshot-sync command not found, trying HMP);
-ret = qemuMonitorTextDiskSnapshot(mon, device, file);
-goto cleanup;
-}
-
ret =
Re: [libvirt] [PATCH] build: fix incremental autogen.sh when no AUTHORS is present
On 12/03/2012 03:14 PM, Eric Blake wrote: Commit 71d1256 tried to fix a problem where rebasing an old branch on top of newer libvirt.git resulted in automake failing because of a missing AUTHORS file. However, while the fix worked for an incremental 'make', it did not work for someone that directly reran './autogen.sh'. Reported by Laine Stump. * autogen.sh (autoreconf): Check for same conditions as cfg.mk. * cfg.mk (_update_required): Add comments. --- autogen.sh |3 ++- cfg.mk |2 ++ 2 files changed, 4 insertions(+), 1 deletions(-) I'm not sure how if it impacts this particular change, but why don't we switch to AM_INIT_AUTOMAKE([foreign]) ? Since AUTHORS or ChangeLog are no longer static, it seems we are just causing ourselves pain by trying to work around auto* insisting those files exist. Then again I don't know what benefits non-foreign gives us... - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] build: fix incremental autogen.sh when no AUTHORS is present
Commit 71d1256 tried to fix a problem where rebasing an old branch on top of newer libvirt.git resulted in automake failing because of a missing AUTHORS file. However, while the fix worked for an incremental 'make', it did not work for someone that directly reran './autogen.sh'. Reported by Laine Stump. My local tree isn't in a state that I can conveniently test it right now, but it looks correct to me. ACK. Thanks; pushed. -- Eric Blake ebl...@redhat.com+1-919-301-3266 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv3 1/3] v2.0: allow guest to guest IPv6 without gateway definition
ACK with the changes I've squashed in. I've attached an interdiff of the changes I've made to this message, and will push as soon as someone else ACKs those additional changes. Thanks for the contribution! I know it's sometimes a pain to get things all the way to pushing, but it really does help keep the expansion manageable and prevent unwanted surprises for our diverse set of users. From 35d259fa52534a8b64a1784674820e06034b03c8 Mon Sep 17 00:00:00 2001 From: Laine Stump la...@laine.org Date: Mon, 3 Dec 2012 15:53:04 -0500 Subject: [PATCH] Squash into 'allow guest to guest IPv6 without gateway definition' ACK - those additional changes make sense given your review. -- Eric Blake ebl...@redhat.com+1-919-301-3266 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv3 1/3] v2.0: allow guest to guest IPv6 without gateway definition
On 12/03/2012 04:03 PM, Laine Stump wrote:
On 12/03/2012 11:13 AM, Gene Czarcinski wrote:
This patch adds the capability for virtual guests to do IPv6
communication via a virtual network interface with no IPv6
(gateway) addresses specified. This capability currently
exists for IPv4.
This patch allows creation of a completely isolated IPv6 network.
Note that virtual guests cannot communication with the virtualization
host via this interface. Also note that:
net.ipv6.conf.interface_name.disable_ipv6 = 1
Also not that starting libvirtd has set the following:
net.bridge.bridge-nf-call-arptables = 1
net.bridge.bridge-nf-call-ip6tables = 1
net.bridge.bridge-nf-call-iptables = 1
although /etc/syslog.conf has them all set to 0.
To control this behavior so that it is not enabled by default, the parameter
ipv6='yes' on the network statement has been added.
Documentation related to this patch has been updated.
The network schema has also been updated.
---
docs/formatnetwork.html.in | 28 +++-
docs/schemas/network.rng| 10 ++
src/conf/network_conf.c | 8
src/conf/network_conf.h | 5 +
src/network/bridge_driver.c | 25 -
5 files changed, 70 insertions(+), 6 deletions(-)
diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
index 49206dd..a3a5ced 100644
--- a/docs/formatnetwork.html.in
+++ b/docs/formatnetwork.html.in
@@ -33,7 +33,7 @@
/p
pre
- lt;networkgt;
+ lt;network ipv6='yes'gt;
lt;namegt;defaultlt;/namegt;
lt;uuidgt;3e3fce45-4f53-4fa7-bb32-11f34168b82blt;/uuidgt;
.../pre
@@ -52,6 +52,12 @@
The format must be RFC 4122 compliant, eg
code3e3fce45-4f53-4fa7-bb32-11f34168b82b/code.
If omitted when defining/creating a new network, a random
UUID is generated. span class=sinceSince 0.3.0/span/dd
+ dtcodeipv6='yes'/code/dt
+ ddThe new, optional parameter codeipv6='yes'/code enables
+a network definition with no IPv6 gateway addresses specified
+to have guest-to-guest communications. For further information,
+see the example below for the example with no gateway addresses.
+span class=sinceSince 1.0.1/span/dd
/dl
h3a name=elementsConnectConnectivity/a/h3
@@ -773,5 +779,25 @@
lt;/forwardgt;
lt;/networkgt;/pre
+h3a name=examplesNoGatewayNetwork config with no gateway
addresses/a/h3
+
+p
+A valid network definition can contain no IPv4 or IPv6 addresses. Such a
definition
+can be used for a very private or very isolated network since it will
not be
+possible to communicate with the virtualization host via this network.
However,
+this virtual network interface can be used for communication between
virtual guest
+systems. This works for IPv4 and span class=since(Since 1.0.1)/span
IPv6.
+However, the new ipv6='yes' must be added for guest-to-guest IPv6
+communication.
+/p
I reformatted this paragraph to fit within 80 columns.
+
+pre
+ lt;network ipv6='yes'gt;
+lt;namegt;nogwlt;/namegt;
+lt;uuidgt;7a3b7497-1ec7-8aef-6d5c-38dff9109e93lt;/uuidgt;
+lt;bridge name=virbr2 stp=on delay=0 /gt;
+lt;mac address='00:16:3E:5D:C7:9E'/gt;
+ lt;/networkgt;/pre
+
/body
/html
diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng
index 4abfd91..0d67f7f 100644
--- a/docs/schemas/network.rng
+++ b/docs/schemas/network.rng
@@ -17,6 +17,16 @@
data type=unsignedInt/
/attribute
/optional
+ !-- Enables IPv6 guest-to-guest communications on a network
+ with no gateways addresses specified --
+ optional
+attribute name=ipv6
+ choice
+ valueyes/value
+ valueno/value
+ /choice
+ /attribute
+ /optional
interleave
!-- The name of the network, used to refer to it through the API
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 6ce2e63..3f9e13c 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -1264,6 +1264,12 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
def-uuid_specified = true;
}
+/* check if definitions with no IPv6 gateway addresses is to
+ * allow guest-to-guest communications.
+ */
+if (virXPathBoolean(boolean(./@ipv6), ctxt) == 1)
+def-ipv6nogw = true;
I don't think virXPathBoolean does what you think it does (although I
haven't figured out exactly what it *does* do, I've noticed that 1) none
of the rest of libvirt uses it for yes/no attributes, and 2) in the
libxml2 source code, when an XPATH_BOOLEAN is formatted into a string,
it becomes True or False. I'm changing the above lines to the following:
ipv6nogwStr = virXPathString(string(./@ipv6), ctxt);
if (ipv6nogwStr) {
if (STREQ(ipv6nogwStr, yes)) {
Re: [libvirt] [PATCH] build: fix incremental autogen.sh when no AUTHORS is present
I'm not sure how if it impacts this particular change, but why don't we switch to AM_INIT_AUTOMAKE([foreign]) ? Since AUTHORS or ChangeLog are no longer static, it seems we are just causing ourselves pain by trying to work around auto* insisting those files exist. Then again I don't know what benefits non-foreign gives us... Interesting idea; reading the automake manual, I see: https://www.gnu.org/software/automake/manual/automake.html#Strictness foreign Automake will check for only those things that are absolutely required for proper operations. For instance, whereas GNU standards dictate the existence of a NEWS file, it will not be required in this mode. This strictness will also turn off some warnings by default (among them, portability warnings). The name comes from the fact that Automake is intended to be used for GNU programs; these relaxed rules are not the standard mode of operation. then further details here: https://www.gnu.org/software/automake/manual/automake.html#Gnits The files INSTALL, NEWS, README, AUTHORS, and ChangeLog, plus one of COPYING.LIB, COPYING.LESSER or COPYING, are required at the topmost directory of the package. If the --add-missing option is given, automake will add a generic version of the INSTALL file as well as the COPYING file containing the text of the current version of the GNU General Public License existing at the time of this Automake release (version 3 as this is written, http://www.gnu.org/copyleft/gpl.html). However, an existing COPYING file will never be overwritten by automake. The options no-installman and no-installinfo are prohibited. Right now, we are relying on 'automake --add-missing' to generate our INSTALL, so switching to 'foreign' would break that. But it wouldn't be too hard to check a static copy of INSTALL into libvirt.git if that's the only thing in the way of us using a more relaxed automake mode. -- Eric Blake ebl...@redhat.com+1-919-301-3266 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] build: fix incremental autogen.sh when no AUTHORS is present
On 12/03/2012 05:37 PM, Eric Blake wrote: I'm not sure how if it impacts this particular change, but why don't we switch to AM_INIT_AUTOMAKE([foreign]) ? Since AUTHORS or ChangeLog are no longer static, it seems we are just causing ourselves pain by trying to work around auto* insisting those files exist. Then again I don't know what benefits non-foreign gives us... Interesting idea; reading the automake manual, I see: https://www.gnu.org/software/automake/manual/automake.html#Strictness foreign Automake will check for only those things that are absolutely required for proper operations. For instance, whereas GNU standards dictate the existence of a NEWS file, it will not be required in this mode. This strictness will also turn off some warnings by default (among them, portability warnings). The name comes from the fact that Automake is intended to be used for GNU programs; these relaxed rules are not the standard mode of operation. then further details here: https://www.gnu.org/software/automake/manual/automake.html#Gnits The files INSTALL, NEWS, README, AUTHORS, and ChangeLog, plus one of COPYING.LIB, COPYING.LESSER or COPYING, are required at the topmost directory of the package. If the --add-missing option is given, automake will add a generic version of the INSTALL file as well as the COPYING file containing the text of the current version of the GNU General Public License existing at the time of this Automake release (version 3 as this is written, http://www.gnu.org/copyleft/gpl.html). However, an existing COPYING file will never be overwritten by automake. The options no-installman and no-installinfo are prohibited. Right now, we are relying on 'automake --add-missing' to generate our INSTALL, so switching to 'foreign' would break that. But it wouldn't be too hard to check a static copy of INSTALL into libvirt.git if that's the only thing in the way of us using a more relaxed automake mode. And in fact I think the default INSTALL file is total overkill and daunting for anyone that doesn't have already have a basic understanding of autotools. This isn't a libvirt specific problem obviously. I think INSTALL should really be under 40 lines of texts, so as not to intimidate, and cover quick install from git, quick install from release tarball, and probably use of ./run for people that just want to run from the build directory. If need be we can keep INSTALL.autotools in git and point to that for more info, but a URL is probably sufficient. - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] implement managedsave in libvirt xen legacy driver
Jim Fehlig wrote:
Bamvor Jian Zhang wrote:
src/xen/xen_driver.c | 109
++-
src/xen/xen_driver.h | 2 +
2 files changed, 110 insertions(+), 1 deletion(-)
diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c
index 5a40757..0b2418d 100644
--- a/src/xen/xen_driver.c
+++ b/src/xen/xen_driver.c
@@ -67,6 +67,7 @@
#include nodeinfo.h
#define VIR_FROM_THIS VIR_FROM_XEN
+#define XEN_SAVE_DIR LOCALSTATEDIR /lib/libvirt/xen/save
#include configmake.h is needed for LOCALSTATEDIR
BTW, you will also need to install this directory and add it to %files
in the spec file. Squashing in the attached patch should do it.
Regards,
Jim
diff --git a/libvirt.spec.in b/libvirt.spec.in
index 5b3f4e4..ec6fc8b 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -1719,6 +1719,9 @@ rm -f $RPM_BUILD_ROOT%{_sysconfdir}/sysctl.d/libvirtd
%ghost %dir %{_localstatedir}/run/libvirt/libxl/
%dir %attr(0700, root, root) %{_localstatedir}/lib/libvirt/libxl/
%endif
+%if %{with_xen}
+%dir %attr(0700, root, root) %{_localstatedir}/lib/libvirt/xen/
+%endif
%if %{with_network}
%ghost %dir %{_localstatedir}/run/libvirt/network/
%dir %attr(0700, root, root) %{_localstatedir}/lib/libvirt/network/
diff --git a/src/Makefile.am b/src/Makefile.am
index b5c20c8..2d36380 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1768,6 +1768,9 @@ if WITH_UML
$(MKDIR_P) $(DESTDIR)$(localstatedir)/lib/libvirt/uml
$(MKDIR_P) $(DESTDIR)$(localstatedir)/run/libvirt/uml
endif
+if WITH_XEN
+ $(MKDIR_P) $(DESTDIR)$(localstatedir)/lib/libvirt/xen
+endif
if WITH_NETWORK
$(MKDIR_P) $(DESTDIR)$(localstatedir)/lib/libvirt/network
$(MKDIR_P) $(DESTDIR)$(localstatedir)/lib/libvirt/dnsmasq
@@ -1814,6 +1817,9 @@ if WITH_UML
rmdir $(DESTDIR)$(localstatedir)/lib/libvirt/uml ||:
rmdir $(DESTDIR)$(localstatedir)/run/libvirt/uml ||:
endif
+if WITH_XEN
+ rmdir $(DESTDIR)$(localstatedir)/lib/libvirt/xen ||:
+endif
if WITH_NETWORK
rm -f $(DESTDIR)$(sysconfdir)/libvirt/qemu/networks/autostart/default.xml
rm -f $(DESTDIR)$(sysconfdir)/libvirt/qemu/networks/default.xml
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
