Re: [libvirt] [Qemu-devel] [PATCHv2] Don't log an internal error when the guest hasn't updated balloon stats
Copying Luiz...
Eric Blake ebl...@redhat.com writes:
On 05/15/2014 01:22 AM, Ján Tomko wrote:
If virDomainMemoryStats is called too soon after domain startup,
QEMU returns:
error:{class:GenericError,desc:guest hasn't updated any stats yet}
when we try to query balloon stats.
Check for this reply and log it as OPERATION_INVALID instead of
INTERNAL_ERROR. This means the daemon only logs it at the debug level,
without polluting system logs.
Reported by Laszlo Pal:
https://www.redhat.com/archives/libvirt-users/2014-May/msg00023.html
---
v1: https://www.redhat.com/archives/libvir-list/2014-May/msg00420.html
v2:
return 0 in this case - even though balloon stats are not yet available,
we can still return 'rss' in qemuDomainMemoryStats
jump to cleanup if CheckError returns 0
src/qemu/qemu_monitor_json.c | 18 ++
1 file changed, 14 insertions(+), 4 deletions(-)
+if ((data = virJSONValueObjectGet(reply, error))) {
+const char *klass = virJSONValueObjectGetString(data, class);
+const char *desc = virJSONValueObjectGetString(data, desc);
-if (ret 0)
+if (STREQ_NULLABLE(klass, GenericError)
+STREQ_NULLABLE(desc, guest hasn't updated any stats yet)) {
You snipped so much of the diff that I have trouble finding the place
this applies.
Adding qemu. Uggh - the qemu documentation of QMP states:
- The desc member is a human-readable error message. Clients should
not attempt to parse this message.
because the contents of that field are NOT guaranteed to be stable.
We're stuck parsing that field for old versions of qemu, but this is one
case where upstream qemu (for future versions) should change the class
member of that particular error case to a distinct value other than
GenericError so that it is trivially obvious when this particular
condition has occurred, since it is a case where libvirt wants to treat
it as a non-error.
reluctant ACK, while hoping that we can do something more reliable in
the future.
Is no stats yet really an error? Libvirt has done nothing wrong, and
I'd argue the guest hasn't done anything wrong, either. Should we
simply return an empty result? Like cat on a file that hasn't gotten
its data, yet.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCHv2] Don't log an internal error when the guest hasn't updated balloon stats
On 05/15/2014 11:59 PM, Markus Armbruster wrote:
Copying Luiz...
src/qemu/qemu_monitor_json.c | 18 ++
1 file changed, 14 insertions(+), 4 deletions(-)
+if ((data = virJSONValueObjectGet(reply, error))) {
+const char *klass = virJSONValueObjectGetString(data, class);
+const char *desc = virJSONValueObjectGetString(data, desc);
-if (ret 0)
+if (STREQ_NULLABLE(klass, GenericError)
+STREQ_NULLABLE(desc, guest hasn't updated any stats yet)) {
You snipped so much of the diff that I have trouble finding the place
this applies.
Apologies; it is for qemuMonitorJSONGetMemoryStats() calling qom-get:
http://libvirt.org/git/?p=libvirt.git;a=blob;f=src/qemu/qemu_monitor_json.c;h=f8ab975695108f3585dfdd869f52b1996e737994;hb=HEAD#l1462
Is no stats yet really an error? Libvirt has done nothing wrong, and
I'd argue the guest hasn't done anything wrong, either. Should we
simply return an empty result? Like cat on a file that hasn't gotten
its data, yet.
Yes, that would be reasonable.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/4] more enum cleanups
On 05/15/2014 10:37 PM, Michal Privoznik wrote:
On 15.05.2014 00:45, Eric Blake wrote:
Inspired by the cleanups contributed by Julio Faracco, I did some
more cleanups of my own. My end goal is to turn on a syntax-check
rule that forbids 'enum vir' (since we should always be declaring
'typedef enum { ... } vir...;'), but there's still a lot of
violations in the code base, even after this series.
ACK to all.
Thanks; pushed
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] vircgroup: Don't leak keypath if failed to kill process
ping
-Original Message-
From: Chen, Hanxiao/陈 晗霄
Sent: Tuesday, May 13, 2014 4:01 PM
To: libvir-list@redhat.com
Cc: Chen, Hanxiao/陈 晗霄
Subject: [libvirt][PATCH] vircgroup: Don't leak keypath if failed to kill
process
Signed-off-by: Chen Hanxiao chenhanx...@cn.fujitsu.com
---
src/util/vircgroup.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index fce380a..c578bd0 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -3370,7 +3370,7 @@ virCgroupKillRecursiveInternal(virCgroupPtr group,
int rc;
bool killedAny = false;
char *keypath = NULL;
-DIR *dp;
+DIR *dp = NULL;
virCgroupPtr subgroup = NULL;
struct dirent *ent;
int direrr;
@@ -3381,7 +3381,7 @@ virCgroupKillRecursiveInternal(virCgroupPtr group,
return -1;
if ((rc = virCgroupKillInternal(group, signum, pids)) 0)
-return -1;
+goto cleanup;
if (rc == 1)
killedAny = true;
@@ -3394,7 +3394,7 @@ virCgroupKillRecursiveInternal(virCgroupPtr group,
}
virReportSystemError(errno,
_(Cannot open %s), keypath);
-return -1;
+goto cleanup;
}
while ((direrr = virDirRead(dp, ent, keypath)) 0) {
@@ -3429,7 +3429,9 @@ virCgroupKillRecursiveInternal(virCgroupPtr group,
cleanup:
virCgroupFree(subgroup);
-closedir(dp);
+VIR_FREE(keypath);
+if (dp)
+closedir(dp);
return ret;
}
--
1.9.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-python PATCH v2] override: add virDomainFSFreeze and virDomainFSThaw API
On 14.5.2014 05:41, tomoki.sekiy...@hds.com wrote: From: Tomoki Sekiyama tomoki.sekiy...@hds.com Hello Michael, Thank you for posting v2. May be we should add following diff to avoid sanitytest.pl's mapping error. --- diff --git a/sanitytest.py b/sanitytest.py index cff30d5..62fe42b 100644 --- a/sanitytest.py +++ b/sanitytest.py @@ -207,6 +207,8 @@ for name in sorted(basicklassmap): func = func[0:1].lower() + func[1:] if func[0:8] == nWFilter: func = nwfilter + func[8:] +if func[0:8] == fSFreeze or func[0:6] == fSThaw: +func = fs + func[2:] # ...except when they don't. More stupid naming # decisions we can't fix Tested and it works only with this change. ACK with the diff above squashed into the patch. Pavel -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] vircgroup: Don't leak keypath if failed to kill process
On 05/13/2014 11:01 AM, Chen Hanxiao wrote:
Signed-off-by: Chen Hanxiao chenhanx...@cn.fujitsu.com
---
src/util/vircgroup.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index fce380a..c578bd0 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -3370,7 +3370,7 @@ virCgroupKillRecursiveInternal(virCgroupPtr group,
int rc;
bool killedAny = false;
char *keypath = NULL;
-DIR *dp;
+DIR *dp = NULL;
virCgroupPtr subgroup = NULL;
struct dirent *ent;
int direrr;
@@ -3381,7 +3381,7 @@ virCgroupKillRecursiveInternal(virCgroupPtr group,
return -1;
if ((rc = virCgroupKillInternal(group, signum, pids)) 0)
-return -1;
+goto cleanup;
if (rc == 1)
killedAny = true;
@@ -3394,7 +3394,7 @@ virCgroupKillRecursiveInternal(virCgroupPtr group,
}
virReportSystemError(errno,
_(Cannot open %s), keypath);
-return -1;
+goto cleanup;
}
while ((direrr = virDirRead(dp, ent, keypath)) 0) {
@@ -3429,7 +3429,9 @@ virCgroupKillRecursiveInternal(virCgroupPtr group,
cleanup:
virCgroupFree(subgroup);
-closedir(dp);
+VIR_FREE(keypath);
+if (dp)
+closedir(dp);
return ret;
}
ACK and pushed (with small change to commit log message)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V3 7/7] security_dac: honor relabel='no' in chardev config
On 05/16/2014 06:16 AM, Jim Fehlig wrote:
The DAC driver ignores the relabel='no' attribute in chardev config
serial type='file'
source path='/tmp/jim/test.file'
seclabel model='dac' relabel='no'/
/source
target port='0'/
/serial
This patch avoids labeling chardevs when relabel='no' is specified.
Signed-off-by: Michal Privoznik mpriv...@redhat.com
Signed-off-by: Jim Fehlig jfeh...@suse.com
---
src/security/security_dac.c | 65
-
1 file changed, 46 insertions(+), 19 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 4434cd0..20f349f 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -705,25 +707,35 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
-if (virSecurityDACGetIds(seclabel, priv, user, group, NULL, NULL))
-return -1;
+if (dev)
+chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev,
+ SECURITY_DAC_NAME);
A check for seclabel-norelabel and chr_seclabel-norelabel is missing here.
-switch ((enum virDomainChrType) dev-type) {
+if (chr_seclabel chr_seclabel-label) {
+if (virParseOwnershipIds(chr_seclabel-label, user, group) 0)
+return -1;
+} else {
+if (virSecurityDACGetIds(seclabel, priv, user, group, NULL, NULL)
0)
+return -1;
+}
+
+switch ((enum virDomainChrType) dev_source-type) {
case VIR_DOMAIN_CHR_TYPE_DEV:
case VIR_DOMAIN_CHR_TYPE_FILE:
Jan
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V3 6/7] security_dac: avoid relabeling hostdevs when relabel='no'
On 05/16/2014 06:16 AM, Jim Fehlig wrote:
When relabel='no' at the domain level, there is no need to call
the hostdev relabeling functions.
Signed-off-by: Michal Privoznik mpriv...@redhat.com
Signed-off-by: Jim Fehlig jfeh...@suse.com
---
src/security/security_dac.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index d6ca303..4434cd0 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -485,6 +485,9 @@
virSecurityDACSetSecurityHostdevLabel(virSecurityManagerPtr mgr,
cbdata.manager = mgr;
cbdata.secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
+if (cbdata.secdef cbdata.secdef-norelabel)
+return 0;
+
switch ((enum virDomainHostdevSubsysType) dev-source.subsys.type) {
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: {
virUSBDevicePtr usb;
@@ -601,10 +604,13 @@
virSecurityDACRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr,
{
virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
+virSecurityLabelDefPtr secdef;
int ret = -1;
-if (!priv-dynamicOwnership)
-return 0;
+secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
+
+if (!priv-dynamicOwnership || (secdef secdef-norelabel))
+ return 0;
Indentation is off here.
Jan
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V3 0/7] Honor DAC norelabel attribute
On 05/16/2014 06:16 AM, Jim Fehlig wrote: V3 of Michal's series to honor relabel='no' in device config https://www.redhat.com/archives/libvir-list/2014-April/msg00196.html In V3, the patches have been further split to ease review as requested by Jan Tomko. Jim Fehlig (7): security_dac: annotate some functions with ATTRIBUTE_NONNULL security_dac: cleanup use of enum types security_dac: rework callback parameter passing security_dac: avoid relabeling when relabel='no' security_dac: honor relabel='no' in disk config security_dac: avoid relabeling hostdevs when relabel='no' security_dac: honor relabel='no' in chardev config src/security/security_dac.c | 333 +++- 1 file changed, 203 insertions(+), 130 deletions(-) ACK series with 7/7 fixed. Thanks for splitting these, I forgot about them. Jan signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Quorum block driver libvirt support proposal
Hello list, I want to implement libvirt Quorum support. (https://github.com/qemu/qemu/commit/c88a1de51ab2f26a9a37ffc317249736de8c015c) Quorum is a QEMU RAID like block storage driver. Data are written on n replicas and when a read is done a comparison between the replica read is done. If more than threshold reads are identical the read succeed else it's and error. For example a Quorum with n = 3 and threshold = 2 would be made of three QCOW2 backing chains used as identicals replicas. threshold = 2 means that at least 2 replica must be identical when doing a read. I want to make use of the new backingStore xml element to implement quorum. Proposed Quorum libvirt format: --- disk type='quorum' device='disk' driver name='qemu' type='quorum'/ threshold value=2/ backingStore type='file' format type='qcow2'/ source file='/var/lib/libvirt/images/file1.qcow2'/ /backingStore backingStore type='file' format type='qcow2'/ source file='/var/lib/libvirt/images/file2.qcow2'/ /backingStore backingStore type='file' format type='qcow2'/ source file='/var/lib/libvirt/images/file3.qcow2'/ /backingStore target dev='vda' bus='virtio'/ /disk Implementation plan: * Add VIR_STORAGE_TYPE_QUORUM * In src/util/virstoragefile.h change _virStorageSource to contain a virStorageSourcePtrPtr backingStores. I think doing it at this level allow to keep a 1-1 mapping with the qemu BlockDriverState hiearchy * Add a int quorum_threshold field to the same structure * Add support for parsing treshold in virDomainDiskDefParseXML * Change virDomainDiskBackingStoreParse to virDomainDiskBackingStoresParse to parse all the backingStore at once an use realloc to grow the backingStores field. * Modify virDomainDiskDefFormat to call virDomainDiskBackingStoreFormat in a loop for saving * hook into qemuBuildDriveStr around line 3442 to create the quorum parameters Do you feel that I am missing something ? Best regards Benoît -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCHv2] Don't log an internal error when the guest hasn't updated balloon stats
On Fri, 16 May 2014 00:11:24 -0600 Eric Blake ebl...@redhat.com wrote: Is no stats yet really an error? This is a special case where the guest hasn't ever filled QEMU with balloon stats. There are two possible cases. Either the guest hasn't done it yet, but will do in the future or the guest will never do it (eg. the guest doesn't support balloon, the guest crashed, etc). Libvirt has done nothing wrong, and I'd argue the guest hasn't done anything wrong, either. Should we simply return an empty result? Like cat on a file that hasn't gotten its data, yet. Yes, that would be reasonable. I'm fine with the two possible solutions here: adding a new TryAgain error class or returning an empty result. I say empty because those fields are not optionals, so we'll have to fill them with some value. Shouldn't be a problem for most fields, as the spec (docs/virtio-balloon-stats.txt) already defines that stats that the guest doesn't report are returned as -1. The only exception here is the last-update field, which can't hold a negative iirc. The only choice is to return 0 there. I guess that this shouldn't be a problem either. Who volunteers to fix this? -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] Fix seclabels for chardevs
We allow a seclabel to be specified in the source element
of a chardev:
serial type='file'
source path='/tmp/serial.file'
seclabel model='dac' relabel='no'/
/source
/serial
But we format it outside the source:
serial type='file'
source path='/tmp/serial.file'/
target port='0'/
seclabel model='dac' relabel='no'/
/serial
Move the formatting inside the source to fix this to make the
seclabel persistent across XML format-parse.
Introduced by commit f8b08d0 'Add seclabel to character devices.'
---
src/conf/domain_conf.c | 27 +++
.../qemuxml2argv-chardev-label.xml | 40 ++
tests/qemuxml2xmltest.c| 2 ++
3 files changed, 54 insertions(+), 15 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-chardev-label.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 041a113..81e9436 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15862,6 +15862,7 @@ virDomainNetDefFormat(virBufferPtr buf,
* output at type='type'. */
static int
virDomainChrSourceDefFormat(virBufferPtr buf,
+virDomainChrDefPtr chr_def,
virDomainChrSourceDefPtr def,
bool tty_compat,
unsigned int flags)
@@ -15898,8 +15899,11 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
if (def-type != VIR_DOMAIN_CHR_TYPE_PTY ||
(def-data.file.path
!(flags VIR_DOMAIN_XML_INACTIVE))) {
-virBufferEscapeString(buf, source path='%s'/\n,
+virBufferEscapeString(buf, source path='%s',
def-data.file.path);
+virDomainSourceDefFormatSeclabel(buf, chr_def-nseclabels,
+ chr_def-seclabels,
+ flags);
}
break;
@@ -15957,7 +15961,9 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
virBufferAsprintf(buf, source mode='%s',
def-data.nix.listen ? bind : connect);
virBufferEscapeString(buf, path='%s', def-data.nix.path);
-virBufferAddLit(buf, /\n);
+virDomainSourceDefFormatSeclabel(buf, chr_def-nseclabels,
+ chr_def-seclabels,
+ flags);
break;
case VIR_DOMAIN_CHR_TYPE_SPICEPORT:
@@ -15979,7 +15985,6 @@ virDomainChrDefFormat(virBufferPtr buf,
const char *targetType = virDomainChrTargetTypeToString(def-deviceType,
def-targetType);
bool tty_compat;
-size_t n;
int ret = 0;
@@ -15997,7 +16002,7 @@ virDomainChrDefFormat(virBufferPtr buf,
def-source.type == VIR_DOMAIN_CHR_TYPE_PTY
!(flags VIR_DOMAIN_XML_INACTIVE)
def-source.data.file.path);
-if (virDomainChrSourceDefFormat(buf, def-source, tty_compat, flags) 0)
+if (virDomainChrSourceDefFormat(buf, def, def-source, tty_compat, flags)
0)
return -1;
/* Format target block */
@@ -16069,14 +16074,6 @@ virDomainChrDefFormat(virBufferPtr buf,
return -1;
}
-/* Security label overrides, if any. */
-if (def-seclabels def-nseclabels 0) {
-virBufferAdjustIndent(buf, 2);
-for (n = 0; n def-nseclabels; n++)
-virSecurityDeviceLabelDefFormat(buf, def-seclabels[n], flags);
-virBufferAdjustIndent(buf, -2);
-}
-
virBufferAdjustIndent(buf, -2);
virBufferAsprintf(buf, /%s\n, elementName);
@@ -16119,7 +16116,7 @@ virDomainSmartcardDefFormat(virBufferPtr buf,
break;
case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
-if (virDomainChrSourceDefFormat(buf, def-data.passthru, false,
+if (virDomainChrSourceDefFormat(buf, NULL, def-data.passthru, false,
flags) 0)
return -1;
break;
@@ -16384,7 +16381,7 @@ virDomainRNGDefFormat(virBufferPtr buf,
case VIR_DOMAIN_RNG_BACKEND_EGD:
virBufferAdjustIndent(buf, 2);
-if (virDomainChrSourceDefFormat(buf, def-source.chardev,
+if (virDomainChrSourceDefFormat(buf, NULL, def-source.chardev,
false, flags) 0)
return -1;
virBufferAdjustIndent(buf, -2);
@@ -16976,7 +16973,7 @@ virDomainRedirdevDefFormat(virBufferPtr buf,
virBufferAsprintf(buf, redirdev bus='%s', bus);
virBufferAdjustIndent(buf, 2);
-if (virDomainChrSourceDefFormat(buf, def-source.chr, false, flags) 0)
+if (virDomainChrSourceDefFormat(buf, NULL, def-source.chr, false, flags)
0)
return -1;
if (virDomainDeviceInfoFormat(buf, def-info,
flags | VIR_DOMAIN_XML_INTERNAL_ALLOW_BOOT)
0)
diff --git
[libvirt] [PATCH 1/2] Rename virDomainDiskSourceDefFormatSeclabel
Drop the 'Disk' from the name, as there is nothing disk-specific
about the function.
---
src/conf/domain_conf.c | 24
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e5ae7c6..041a113 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -14809,16 +14809,16 @@ virDomainDiskBlockIoDefFormat(virBufferPtr buf,
}
-/* virDomainDiskSourceDefFormatSeclabel:
+/* virDomainSourceDefFormatSeclabel:
*
* This function automaticaly closes the source element and formats any
* possible seclabels.
*/
static void
-virDomainDiskSourceDefFormatSeclabel(virBufferPtr buf,
- size_t nseclabels,
- virSecurityDeviceLabelDefPtr *seclabels,
- unsigned int flags)
+virDomainSourceDefFormatSeclabel(virBufferPtr buf,
+ size_t nseclabels,
+ virSecurityDeviceLabelDefPtr *seclabels,
+ unsigned int flags)
{
size_t n;
@@ -14853,17 +14853,17 @@ virDomainDiskSourceFormat(virBufferPtr buf,
virBufferEscapeString(buf, file='%s', src-path);
virBufferEscapeString(buf, startupPolicy='%s', startupPolicy);
-virDomainDiskSourceDefFormatSeclabel(buf, src-nseclabels,
- src-seclabels, flags);
- break;
+virDomainSourceDefFormatSeclabel(buf, src-nseclabels,
+ src-seclabels, flags);
+break;
case VIR_STORAGE_TYPE_BLOCK:
virBufferAddLit(buf, source);
virBufferEscapeString(buf, dev='%s', src-path);
virBufferEscapeString(buf, startupPolicy='%s', startupPolicy);
-virDomainDiskSourceDefFormatSeclabel(buf, src-nseclabels,
- src-seclabels, flags);
+virDomainSourceDefFormatSeclabel(buf, src-nseclabels,
+ src-seclabels, flags);
break;
case VIR_STORAGE_TYPE_DIR:
@@ -14917,8 +14917,8 @@ virDomainDiskSourceFormat(virBufferPtr buf,
}
virBufferEscapeString(buf, startupPolicy='%s', startupPolicy);
-virDomainDiskSourceDefFormatSeclabel(buf, src-nseclabels,
- src-seclabels, flags);
+virDomainSourceDefFormatSeclabel(buf, src-nseclabels,
+ src-seclabels, flags);
break;
case VIR_STORAGE_TYPE_NONE:
--
1.8.3.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] Fix seclabels for chardevs
Ján Tomko (2): Rename virDomainDiskSourceDefFormatSeclabel Fix seclabels for chardevs src/conf/domain_conf.c | 51 ++ .../qemuxml2argv-chardev-label.xml | 40 + tests/qemuxml2xmltest.c| 2 + 3 files changed, 66 insertions(+), 27 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-chardev-label.xml -- 1.8.3.2 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] Rename virDomainDiskSourceDefFormatSeclabel
in subject: s/Rename/conf: Rename/ On 05/16/14 15:23, Ján Tomko wrote: Drop the 'Disk' from the name, as there is nothing disk-specific about the function. --- src/conf/domain_conf.c | 24 1 file changed, 12 insertions(+), 12 deletions(-) ACK, good to see it reused. Peter signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 1/1] migration: add support for migrateURI configuration
On Thu, May 15, 2014 at 11:50:54AM +0200, Jiri Denemark wrote: On Wed, May 14, 2014 at 15:18:09 +0800, Chen Fan wrote: For now, we set the migration URI via command line '--migrate_uri' or construct the URI by looking up the dest host's hostname which could be solved by DNS automatically. But in cases the dest host have two or more NICs to reach, we may need to send the migration data over a specific NIC which is different from the automatically resloved one for some reason like performance, security, etc. thus we must explicitly specify the migrateuri in command line everytime, but it is too troublesome if there are many such hosts(and don't forget virt-manager). This patches add a configuration file option on dest host to save the default migrate uri which explicitly specify which of this host's addresses is used for transferring data, thus user doesn't boring to specify it in command line everytime. Signed-off-by: Chen Fan chen.fan.f...@cn.fujitsu.com --- v3-v4: move up the default uri_in setting to qemuDomainMigratePrepare3Params() src/qemu/qemu.conf | 6 +- src/qemu/qemu_conf.c | 1 + src/qemu/qemu_conf.h | 1 + src/qemu/qemu_driver.c | 2 +- 4 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index f0e802f..6b443d0 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -449,7 +449,11 @@ # #seccomp_sandbox = 1 - +# Override the migration URI for specifying one of host's IP addresses +# to transfer the migration data stream. +# Defaults to hostname, both IPv4 and IPv6 addresses are accepted. +# +#migrate_uri = tcp://192.168.0.1 # Override the listen address for all incoming migrations. Defaults to # 0.0.0.0, or :: if both host and qemu are capable of IPv6. The more I think about this the more I incline to a slightly different approach. Rather than providing a way to override migration URI, we could just provide an option in libvirtd.conf to override what virGetHostname returns. That is, the option (naturally called hostname) would tell libvirt to use the configured hostname (which might even be just an IP address) instead of trying to detect it. I'm not sure this is a good idea. The hostname is used in a number of places in libvirt, and we don't neccessarily want them all to use the same interface as the migration data. eg if you have a NIC that is dedicated just for migration traffic, you won't want to force other parts of libvirt to use that. So having the migration hostname specified separately is a good idea IMHO. Perhaps we could simplify though by having 'migrate_host' rather than 'migrate_uri' ? And perhaps we do want a hostname override globally anyway, for other reasons. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Quorum block driver libvirt support proposal
On Fri, May 16, 2014 at 12:33:04PM +0200, Benoît Canet wrote: Hello list, I want to implement libvirt Quorum support. (https://github.com/qemu/qemu/commit/c88a1de51ab2f26a9a37ffc317249736de8c015c) Quorum is a QEMU RAID like block storage driver. Data are written on n replicas and when a read is done a comparison between the replica read is done. If more than threshold reads are identical the read succeed else it's and error. For example a Quorum with n = 3 and threshold = 2 would be made of three QCOW2 backing chains used as identicals replicas. threshold = 2 means that at least 2 replica must be identical when doing a read. I want to make use of the new backingStore xml element to implement quorum. Proposed Quorum libvirt format: --- disk type='quorum' device='disk' driver name='qemu' type='quorum'/ threshold value=2/ backingStore type='file' format type='qcow2'/ source file='/var/lib/libvirt/images/file1.qcow2'/ /backingStore backingStore type='file' format type='qcow2'/ source file='/var/lib/libvirt/images/file2.qcow2'/ /backingStore backingStore type='file' format type='qcow2'/ source file='/var/lib/libvirt/images/file3.qcow2'/ /backingStore target dev='vda' bus='virtio'/ /disk It feels rather odd to have backingStore elements but no top level disk images. Really these are all top level images Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Quorum block driver libvirt support proposal
The Friday 16 May 2014 à 09:54:43 (-0400), Daniel P. Berrange wrote : On Fri, May 16, 2014 at 12:33:04PM +0200, Benoît Canet wrote: Hello list, I want to implement libvirt Quorum support. (https://github.com/qemu/qemu/commit/c88a1de51ab2f26a9a37ffc317249736de8c015c) Quorum is a QEMU RAID like block storage driver. Data are written on n replicas and when a read is done a comparison between the replica read is done. If more than threshold reads are identical the read succeed else it's and error. For example a Quorum with n = 3 and threshold = 2 would be made of three QCOW2 backing chains used as identicals replicas. threshold = 2 means that at least 2 replica must be identical when doing a read. I want to make use of the new backingStore xml element to implement quorum. Proposed Quorum libvirt format: --- disk type='quorum' device='disk' driver name='qemu' type='quorum'/ threshold value=2/ backingStore type='file' format type='qcow2'/ source file='/var/lib/libvirt/images/file1.qcow2'/ /backingStore backingStore type='file' format type='qcow2'/ source file='/var/lib/libvirt/images/file2.qcow2'/ /backingStore backingStore type='file' format type='qcow2'/ source file='/var/lib/libvirt/images/file3.qcow2'/ /backingStore target dev='vda' bus='virtio'/ /disk It feels rather odd to have backingStore elements but no top level disk images. Really these are all top level images It reflect the ways QEMU does it. A single BlockDriverState holding n quorum BlockDriverState children. There is a 1-1 mapping. How would you see it ? Best regards Benoît Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] Fix seclabels for chardevs
In subject:
s/Fix/conf: Fix/
On 05/16/14 15:23, Ján Tomko wrote:
We allow a seclabel to be specified in the source element
of a chardev:
serial type='file'
source path='/tmp/serial.file'
seclabel model='dac' relabel='no'/
/source
/serial
There is one paragraph mentioning that in the XML format documentation.
I think it would be worth adding (as a separate patch) an example of the
usage too.
But we format it outside the source:
serial type='file'
source path='/tmp/serial.file'/
target port='0'/
seclabel model='dac' relabel='no'/
/serial
Move the formatting inside the source to fix this to make the
seclabel persistent across XML format-parse.
Introduced by commit f8b08d0 'Add seclabel to character devices.'
---
src/conf/domain_conf.c | 27 +++
.../qemuxml2argv-chardev-label.xml | 40
++
tests/qemuxml2xmltest.c| 2 ++
3 files changed, 54 insertions(+), 15 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-chardev-label.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 041a113..81e9436 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15862,6 +15862,7 @@ virDomainNetDefFormat(virBufferPtr buf,
* output at type='type'. */
static int
virDomainChrSourceDefFormat(virBufferPtr buf,
+virDomainChrDefPtr chr_def,
virDomainChrSourceDefPtr def,
bool tty_compat,
unsigned int flags)
@@ -15898,8 +15899,11 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
if (def-type != VIR_DOMAIN_CHR_TYPE_PTY ||
(def-data.file.path
!(flags VIR_DOMAIN_XML_INACTIVE))) {
-virBufferEscapeString(buf, source path='%s'/\n,
+virBufferEscapeString(buf, source path='%s',
def-data.file.path);
+virDomainSourceDefFormatSeclabel(buf, chr_def-nseclabels,
+ chr_def-seclabels,
+ flags);
}
break;
I think that case VIR_DOMAIN_CHR_TYPE_UNIX: should be probably handled
too.
@@ -15957,7 +15961,9 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
virBufferAsprintf(buf, source mode='%s',
def-data.nix.listen ? bind : connect);
virBufferEscapeString(buf, path='%s', def-data.nix.path);
-virBufferAddLit(buf, /\n);
+virDomainSourceDefFormatSeclabel(buf, chr_def-nseclabels,
+ chr_def-seclabels,
+ flags);
break;
case VIR_DOMAIN_CHR_TYPE_SPICEPORT:
@@ -15979,7 +15985,6 @@ virDomainChrDefFormat(virBufferPtr buf,
const char *targetType = virDomainChrTargetTypeToString(def-deviceType,
def-targetType);
bool tty_compat;
-size_t n;
int ret = 0;
@@ -15997,7 +16002,7 @@ virDomainChrDefFormat(virBufferPtr buf,
def-source.type == VIR_DOMAIN_CHR_TYPE_PTY
!(flags VIR_DOMAIN_XML_INACTIVE)
def-source.data.file.path);
-if (virDomainChrSourceDefFormat(buf, def-source, tty_compat, flags)
0)
+if (virDomainChrSourceDefFormat(buf, def, def-source, tty_compat,
flags) 0)
return -1;
/* Format target block */
@@ -16069,14 +16074,6 @@ virDomainChrDefFormat(virBufferPtr buf,
return -1;
}
-/* Security label overrides, if any. */
-if (def-seclabels def-nseclabels 0) {
-virBufferAdjustIndent(buf, 2);
-for (n = 0; n def-nseclabels; n++)
-virSecurityDeviceLabelDefFormat(buf, def-seclabels[n], flags);
-virBufferAdjustIndent(buf, -2);
-}
-
virBufferAdjustIndent(buf, -2);
virBufferAsprintf(buf, /%s\n, elementName);
@@ -16119,7 +16116,7 @@ virDomainSmartcardDefFormat(virBufferPtr buf,
break;
case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
-if (virDomainChrSourceDefFormat(buf, def-data.passthru, false,
+if (virDomainChrSourceDefFormat(buf, NULL, def-data.passthru,
false,
flags) 0)
Passing NULL as chr_def to virDomainChrSourceDefFormat will induce a
crash once you will try to format a RNG, smartcard or other device with
a chardev backend with type PIPE or other.
return -1;
break;
@@ -16384,7 +16381,7 @@ virDomainRNGDefFormat(virBufferPtr buf,
case VIR_DOMAIN_RNG_BACKEND_EGD:
virBufferAdjustIndent(buf, 2);
-if (virDomainChrSourceDefFormat(buf, def-source.chardev,
+if (virDomainChrSourceDefFormat(buf, NULL, def-source.chardev,
Re: [libvirt] Quorum block driver libvirt support proposal
On 05/16/14 16:05, Benoît Canet wrote: The Friday 16 May 2014 à 09:54:43 (-0400), Daniel P. Berrange wrote : On Fri, May 16, 2014 at 12:33:04PM +0200, Benoît Canet wrote: Hello list, I want to implement libvirt Quorum support. (https://github.com/qemu/qemu/commit/c88a1de51ab2f26a9a37ffc317249736de8c015c) Quorum is a QEMU RAID like block storage driver. Data are written on n replicas and when a read is done a comparison between the replica read is done. If more than threshold reads are identical the read succeed else it's and error. For example a Quorum with n = 3 and threshold = 2 would be made of three QCOW2 backing chains used as identicals replicas. threshold = 2 means that at least 2 replica must be identical when doing a read. I want to make use of the new backingStore xml element to implement quorum. Proposed Quorum libvirt format: --- disk type='quorum' device='disk' driver name='qemu' type='quorum'/ threshold value=2/ backingStore type='file' format type='qcow2'/ source file='/var/lib/libvirt/images/file1.qcow2'/ /backingStore backingStore type='file' format type='qcow2'/ source file='/var/lib/libvirt/images/file2.qcow2'/ /backingStore backingStore type='file' format type='qcow2'/ source file='/var/lib/libvirt/images/file3.qcow2'/ /backingStore target dev='vda' bus='virtio'/ /disk It feels rather odd to have backingStore elements but no top level disk images. Really these are all top level images It reflect the ways QEMU does it. A single BlockDriverState holding n quorum BlockDriverState children. There is a 1-1 mapping. How would you see it ? We'd rather see multiple source elements for the top level disk. Backing store is the property of the source image, thus every single of those sources should have it's own list. (or perhaps a tree?) Best regards Benoît Peter signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Quorum block driver libvirt support proposal
On 05/16/2014 07:54 AM, Daniel P. Berrange wrote: disk type='quorum' device='disk' driver name='qemu' type='quorum'/ threshold value=2/ backingStore type='file' format type='qcow2'/ source file='/var/lib/libvirt/images/file1.qcow2'/ /backingStore backingStore type='file' format type='qcow2'/ source file='/var/lib/libvirt/images/file2.qcow2'/ /backingStore backingStore type='file' format type='qcow2'/ source file='/var/lib/libvirt/images/file3.qcow2'/ /backingStore target dev='vda' bus='virtio'/ /disk It feels rather odd to have backingStore elements but no top level disk images. Really these are all top level images Unfortunately, we are allowed to have a quorum with mixed-mode sources - I could have a quorum where file 1 is a local file, file 2 is a block device, and file 3 is a gluster protocol. But since we encode the type of file at the disk type='...' level, there is NO way to list three different source elements for those three quorum members. I think Benoit's proposal makes sense - a quorum is a node in the backing chain with NO source element, but instead has MULTIPLE backingStore elements. disk type='quorum' device='disk' driver name='qemu' type='quorum'/ threshold value=2/ backingStore type='file' format type='qcow2'/ source file='/var/lib/libvirt/images/file1.qcow2'/ /backingStore backingStore type='block' format type='qcow2'/ source dev='/var/lib/libvirt/images/file2.qcow2'/ /backingStore backingStore type='network' format type='qcow2'/ source protocol='gluster' name='Volume1/Image' host name='example.org'/ /source /backingStore target dev='vda' bus='virtio'/ /disk -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Quorum block driver libvirt support proposal
On 05/16/2014 08:07 AM, Peter Krempa wrote: It feels rather odd to have backingStore elements but no top level disk images. Really these are all top level images It reflect the ways QEMU does it. A single BlockDriverState holding n quorum BlockDriverState children. There is a 1-1 mapping. How would you see it ? We'd rather see multiple source elements for the top level disk. Backing store is the property of the source image, thus every single of those sources should have it's own list. (or perhaps a tree?) I don't see how you can possibly have multiple source elements. Remember, part of the determination of what forms a valid source element is the type='...' attribute tied to the disk parent element - but you can't have duplicate attributes. As I see it, a quorum HAS to be a special chain element with 0 sources and multiple backingStore children, where each backingStore then includes the type='...' attribute for how to interpret the source element of that child. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Quorum block driver libvirt support proposal
On 05/16/2014 04:33 AM, Benoît Canet wrote: I want to make use of the new backingStore xml element to implement quorum. Proposed Quorum libvirt format: --- disk type='quorum' device='disk' driver name='qemu' type='quorum'/ threshold value=2/ Rather than making threshold a sub-element, I'd stick it as an attribute, as in: disk type='quorum' threshold='2' device='disk' * Add VIR_STORAGE_TYPE_QUORUM * In src/util/virstoragefile.h change _virStorageSource to contain a virStorageSourcePtrPtr backingStores. PtrPtr doesn't make sense. Just keep it as a single pointer, but add an nBackingStores field and treat it as an array (all existing callers are now an array of 1, quorum is a new array of N). I think doing it at this level allow to keep a 1-1 mapping with the qemu BlockDriverState hiearchy * Add a int quorum_threshold field to the same structure size_t, not int * Add support for parsing treshold in virDomainDiskDefParseXML * Change virDomainDiskBackingStoreParse to virDomainDiskBackingStoresParse to parse all the backingStore at once an use realloc to grow the backingStores field. * Modify virDomainDiskDefFormat to call virDomainDiskBackingStoreFormat in a loop for saving * hook into qemuBuildDriveStr around line 3442 to create the quorum parameters Do you feel that I am missing something ? We'll probably find more as you go, but this sounds like a reasonable start -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Quorum block driver libvirt support proposal
The Friday 16 May 2014 à 08:20:22 (-0600), Eric Blake wrote : On 05/16/2014 08:07 AM, Peter Krempa wrote: It feels rather odd to have backingStore elements but no top level disk images. Really these are all top level images It reflect the ways QEMU does it. A single BlockDriverState holding n quorum BlockDriverState children. There is a 1-1 mapping. How would you see it ? We'd rather see multiple source elements for the top level disk. Backing store is the property of the source image, thus every single of those sources should have it's own list. (or perhaps a tree?) I don't see how you can possibly have multiple source elements. Remember, part of the determination of what forms a valid source element is the type='...' attribute tied to the disk parent element - but you can't have duplicate attributes. As I see it, a quorum HAS to be a special chain element with 0 sources and multiple backingStore children, where each backingStore then includes the type='...' attribute for how to interpret the source element of that child. Additionally quorum support taking snapshots so we need one entity to bind them together. Best regards Benoît -- Eric Blake eblake 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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [RFC 0/5] Allow object-add on X86CPU subclasses, for CPU model probing
On Thu, 15 May 2014 11:03:49 -0300 Eduardo Habkost ehabk...@redhat.com wrote: On Thu, May 15, 2014 at 03:48:16PM +0200, Igor Mammedov wrote: On Thu, 15 May 2014 10:07:51 -0300 Eduardo Habkost ehabk...@redhat.com wrote: On Thu, May 15, 2014 at 02:35:01PM +0200, Igor Mammedov wrote: On Tue, 06 May 2014 22:29:24 +0200 Andreas Färber afaer...@suse.de wrote: Am 06.05.2014 22:19, schrieb Eduardo Habkost: On Tue, May 06, 2014 at 10:01:11PM +0200, Igor Mammedov wrote: On Tue, 6 May 2014 11:42:56 -0300 Eduardo Habkost ehabk...@redhat.com wrote: On Tue, May 06, 2014 at 09:22:38AM +0200, Igor Mammedov wrote: On Fri, 2 May 2014 11:43:05 -0300 Eduardo Habkost ehabk...@redhat.com wrote: On Fri, May 02, 2014 at 03:45:03PM +0200, Igor Mammedov wrote: On Wed, 30 Apr 2014 17:29:28 -0300 Eduardo Habkost ehabk...@redhat.com wrote: This series allows management code to use object-add on X86CPU subclasses, so it Is there any reason why device-add couldn't be used? It needs to work with -machine none, device_add requires a bus to exist, and there is no icc-bus on machine_none. The thing is that CPUID is a function of machine so using -machine none will provide only approximately accurate data. I'm not sure that retrieved possibly not accurate data are useful for libvirt. -cpu host doesn't depend on machine, and is the most important thing right now (because libvirt's checks for host QEMU/kernel/CPU capabilities is completely broken). true, but device-add/-cpu host could be used right now to get the same CPUID data wile using any machine type or default one without any of this patches. device_add can't be used with -machine none. I see no reason why we couldn't *make* CPUs work on -machine none. The ICC bus and bridge were a hack to make APIC(?) hot-add work in face of SysBus; if that prohibits other valid uses now, then evaluating Igor's memory work for CPU might be an option. Yep, if CPU is hot-plugged as bus-less device. There is a little concern of APIC device if we go that direction since in addition to hotpluggable BUS, BUS provides address-space for APIC MMIO. With that resolved, x86-cpu shouldn't depend on any bus and if there isn't any current user that uses QOM path to CPU for introspecting (Eduardo's ABI concern), then it could be done in time for 2.1. Maybe there are no users of the current QOM path, but we do need a stable path to allow management to locate the CPU objects. Do we have one, already? Can't we add query-cpus QMP command or something like this to hide path from user. That would work, too. But why is a dedicated query-cpus command better than something like qom-list path=/machine/cpus (that could simply return a list of links to the actual CPU objects)? So that not to stall the work on deciding on - if exposing not yet stables QOM paths as stable ABI is a good thing, I recall Andreas objecting to using QOM paths with device hotplug - what paths to CPUs should be wrt stalled topology discussion -- Eduardo -- Regards, Igor -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] PCI: Introduce new device binding path using pci_dev.driver_override
On Fri, May 16, 2014 at 10:48:00AM -0400, Konrad Rzeszutek Wilk wrote: On Fri, May 9, 2014 at 12:50 PM, Alex Williamson alex.william...@redhat.com wrote: The driver_override field allows us to specify the driver for a device ... ... Signed-off-by: Alex Williamson alex.william...@redhat.com Cc: Greg Kroah-Hartman gre...@linuxfoundation.org Reviewed-by: Konrad Rzeszutek Wilk konrad.w...@oracle.com And somehow my emailer (Google) decided to add yucky HTML crud to the end. So here is an nice plain email so that it can go the the right mailing lists - and sorry for the extra emails to those on the 'To' and 'CC'! ___ iommu mailing list io...@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [RFC 0/5] Allow object-add on X86CPU subclasses, for CPU model probing
Am 15.05.2014 14:35, schrieb Igor Mammedov: PS: As side effect cpu/apic will disappear from info qtree HMP command output. Solutions are already on the list and in need of feedback: http://patchwork.ozlabs.org/patch/317224/ http://patchwork.ozlabs.org/patch/343136/ http://patchwork.ozlabs.org/patch/347064/ Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2] conf: fix seclabels for chardevs
We allow a seclabel to be specified in the source element
of a chardev:
serial type='file'
source path='/tmp/serial.file'
seclabel model='dac' relabel='no'/
/source
/serial
But we format it outside the source:
serial type='file'
source path='/tmp/serial.file'/
target port='0'/
seclabel model='dac' relabel='no'/
/serial
Move the formatting inside the source to fix this to make the
seclabel persistent across XML format-parse.
Introduced by commit f8b08d0 'Add seclabel to character devices.'
---
v2: don't crash (and add a test for that too)
src/conf/domain_conf.c | 30 +++
.../qemuxml2argv-chardev-label.xml | 45 ++
tests/qemuxml2xmltest.c| 2 +
3 files changed, 62 insertions(+), 15 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-chardev-label.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 041a113..b5a9a66 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15862,11 +15862,19 @@ virDomainNetDefFormat(virBufferPtr buf,
* output at type='type'. */
static int
virDomainChrSourceDefFormat(virBufferPtr buf,
+virDomainChrDefPtr chr_def,
virDomainChrSourceDefPtr def,
bool tty_compat,
unsigned int flags)
{
const char *type = virDomainChrTypeToString(def-type);
+size_t nseclabels = 0;
+virSecurityDeviceLabelDefPtr *seclabels = NULL;
+
+if (chr_def) {
+nseclabels = chr_def-nseclabels;
+seclabels = chr_def-seclabels;
+}
if (!type) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -15898,8 +15906,9 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
if (def-type != VIR_DOMAIN_CHR_TYPE_PTY ||
(def-data.file.path
!(flags VIR_DOMAIN_XML_INACTIVE))) {
-virBufferEscapeString(buf, source path='%s'/\n,
+virBufferEscapeString(buf, source path='%s',
def-data.file.path);
+virDomainSourceDefFormatSeclabel(buf, nseclabels, seclabels,
flags);
}
break;
@@ -15957,7 +15966,7 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
virBufferAsprintf(buf, source mode='%s',
def-data.nix.listen ? bind : connect);
virBufferEscapeString(buf, path='%s', def-data.nix.path);
-virBufferAddLit(buf, /\n);
+virDomainSourceDefFormatSeclabel(buf, nseclabels, seclabels, flags);
break;
case VIR_DOMAIN_CHR_TYPE_SPICEPORT:
@@ -15979,7 +15988,6 @@ virDomainChrDefFormat(virBufferPtr buf,
const char *targetType = virDomainChrTargetTypeToString(def-deviceType,
def-targetType);
bool tty_compat;
-size_t n;
int ret = 0;
@@ -15997,7 +16005,7 @@ virDomainChrDefFormat(virBufferPtr buf,
def-source.type == VIR_DOMAIN_CHR_TYPE_PTY
!(flags VIR_DOMAIN_XML_INACTIVE)
def-source.data.file.path);
-if (virDomainChrSourceDefFormat(buf, def-source, tty_compat, flags) 0)
+if (virDomainChrSourceDefFormat(buf, def, def-source, tty_compat, flags)
0)
return -1;
/* Format target block */
@@ -16069,14 +16077,6 @@ virDomainChrDefFormat(virBufferPtr buf,
return -1;
}
-/* Security label overrides, if any. */
-if (def-seclabels def-nseclabels 0) {
-virBufferAdjustIndent(buf, 2);
-for (n = 0; n def-nseclabels; n++)
-virSecurityDeviceLabelDefFormat(buf, def-seclabels[n], flags);
-virBufferAdjustIndent(buf, -2);
-}
-
virBufferAdjustIndent(buf, -2);
virBufferAsprintf(buf, /%s\n, elementName);
@@ -16119,7 +16119,7 @@ virDomainSmartcardDefFormat(virBufferPtr buf,
break;
case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
-if (virDomainChrSourceDefFormat(buf, def-data.passthru, false,
+if (virDomainChrSourceDefFormat(buf, NULL, def-data.passthru, false,
flags) 0)
return -1;
break;
@@ -16384,7 +16384,7 @@ virDomainRNGDefFormat(virBufferPtr buf,
case VIR_DOMAIN_RNG_BACKEND_EGD:
virBufferAdjustIndent(buf, 2);
-if (virDomainChrSourceDefFormat(buf, def-source.chardev,
+if (virDomainChrSourceDefFormat(buf, NULL, def-source.chardev,
false, flags) 0)
return -1;
virBufferAdjustIndent(buf, -2);
@@ -16976,7 +16976,7 @@ virDomainRedirdevDefFormat(virBufferPtr buf,
virBufferAsprintf(buf, redirdev bus='%s', bus);
virBufferAdjustIndent(buf, 2);
-if (virDomainChrSourceDefFormat(buf, def-source.chr, false, flags) 0)
+if (virDomainChrSourceDefFormat(buf, NULL, def-source.chr, false, flags)
0)
return -1;
[libvirt] [PATCH 2.718/2] docs: add a serial device with a seclabel example
--- docs/formatdomain.html.in | 6 ++ 1 file changed, 6 insertions(+) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 76b2bc2..691a451 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4311,6 +4311,12 @@ qemu-kvm -net nic,model=? /dev/null lt;source path='/dev/pts/3'/gt; lt;target port='0'/gt; lt;/serialgt; +lt;serial type='filegt; + lt;source path='/tmp/file'gt; +lt;seclabel model='dac' relabel='no'/gt; + lt;/sourcegt; + lt;target port='0'gt; +lt;/serialgt; lt;console type='pty'gt; lt;source path='/dev/pts/4'/gt; lt;target port='0'/gt; -- 1.8.3.2 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [RFC 0/5] Allow object-add on X86CPU subclasses, for CPU model probing
On Fri, May 16, 2014 at 04:52:21PM +0200, Igor Mammedov wrote: On Thu, 15 May 2014 11:03:49 -0300 Eduardo Habkost ehabk...@redhat.com wrote: On Thu, May 15, 2014 at 03:48:16PM +0200, Igor Mammedov wrote: [...] Can't we add query-cpus QMP command or something like this to hide path from user. That would work, too. But why is a dedicated query-cpus command better than something like qom-list path=/machine/cpus (that could simply return a list of links to the actual CPU objects)? So that not to stall the work on deciding on - if exposing not yet stables QOM paths as stable ABI is a good thing, I recall Andreas objecting to using QOM paths with device hotplug This surprises me. - what paths to CPUs should be wrt stalled topology discussion I don't see why it depends on the topology discussion: if we are capable of keeping query-cpus working after we introduce the topology hierarchy, I believe we are perfectly capable of keeping symlinks on /machine/cpus working, too. Even if we change the actual paths later and introduce a more complex QOM hierarchy somewhere else. Isn't the reuse of infrastructure for list/get/set operations the whole point of QOM? -- Eduardo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] PCI: Introduce new device binding path using pci_dev.driver_override
On Fri, May 9, 2014 at 12:50 PM, Alex Williamson alex.william...@redhat.com wrote: The driver_override field allows us to specify the driver for a device ... ... Signed-off-by: Alex Williamson alex.william...@redhat.com Cc: Greg Kroah-Hartman gre...@linuxfoundation.org Reviewed-by: Konrad Rzeszutek Wilk konrad.w...@oracle.com -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: refactor virNetlinkCommand to fix several bugs / style problems
On 05/13/2014 01:51 PM, Laine Stump wrote:
Inspired by a simpler patch from Wangrui (K) moon.wang...@huawei.com.
A submitted patch pointed out that virNetlinkCommand() was doing an
improper typecast of the return value from nl_recv() (int to
unsigned), causing it to miss error returns, and that even after
remedying that problem, virNetlinkCommand() was calling VIR_FREE() on
the pointer returned from nl_recv() (*resp) even if nl_recv() had
returned an error, and that in this case the pointer was verifiably
invalid, as it was pointing to memory that had been allocated by
libnl, but then freed prior to returning the error.
While reviewing this patch, I noticed several other problems with this
seemingly simple function (at least one of them as serious as the
problem being reported/fixed by the aforementioned patch), and decided
they all deserved to be fixed. Here is the list:
1) The return value from nl_recv() must be assigned to an int (rather
than unsigned int) in order to detect failure.
2) When nl_recv() returns an error, the contents of *resp is invalid,
and should be simply set to 0, *not* VIR_FREE()'d.
3) The first error return from virNetlinkCommand returns -EINVAL,
incorrectly implying that the caller can expect the return value to
be of the -errno variety, which is not true in any other case.
4) The 2nd error return returns directly with garbage in *resp. While
the caller should never use *resp in this case, it's still good
practice to set it to NULL.
5) For the next 5 (!!) error conditions, *resp will contain garbage,
and virNetlinkCommand() will goto it's cleanup code which will
VIR_FREE(*resp), almost surely leading to a segfault.
In addition to fixing these 5 problems, this patch also makes the
following two changes to make the function conform more closely to the
style of other libvirt code:
1) Change the handling of return code from named rc and defaulted to
0, but changed to -1 on error to the more common named ret and
defaulted to -1, but changed to 0 on success.
2) Rename the error label to cleanup, since the code that follows
is executed in success cases as well as failure.
---
src/util/virnetlink.c | 42 --
1 file changed, 20 insertions(+), 22 deletions(-)
ACK. Just nits below.
@@ -253,26 +250,27 @@ int virNetlinkCommand(struct nl_msg *nl_msg,
if (n == 0)
virReportSystemError(ETIMEDOUT, %s,
_(no valid netlink response was
received));
-rc = -1;
-goto error;
+goto cleanup;
}
-*respbuflen = nl_recv(nlhandle, nladdr,
- (unsigned char **)resp, NULL);
-if (*respbuflen = 0) {
+len = nl_recv(nlhandle, nladdr, (unsigned char **)resp, NULL);
+if (len = 0) {
Does nl_recv set errno when it returns 0?
virReportSystemError(errno,
%s, _(nl_recv failed));
-rc = -1;
+goto cleanup;
}
- error:
-if (rc == -1) {
-VIR_FREE(*resp);
+
[1] here more.
+ret = 0;
+ cleanup:
+if (ret 0) {
*resp = NULL;
*respbuflen = 0;
+} else {
+*respbuflen = len;
Personally, I'd like this assignment [1]
}
virNetlinkFree(nlhandle);
-return rc;
+return ret;
}
static void
Jan
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Quorum block driver libvirt support proposal
On Fri, May 16, 2014 at 08:18:36AM -0600, Eric Blake wrote: On 05/16/2014 07:54 AM, Daniel P. Berrange wrote: disk type='quorum' device='disk' driver name='qemu' type='quorum'/ threshold value=2/ backingStore type='file' format type='qcow2'/ source file='/var/lib/libvirt/images/file1.qcow2'/ /backingStore backingStore type='file' format type='qcow2'/ source file='/var/lib/libvirt/images/file2.qcow2'/ /backingStore backingStore type='file' format type='qcow2'/ source file='/var/lib/libvirt/images/file3.qcow2'/ /backingStore target dev='vda' bus='virtio'/ /disk It feels rather odd to have backingStore elements but no top level disk images. Really these are all top level images Unfortunately, we are allowed to have a quorum with mixed-mode sources - I could have a quorum where file 1 is a local file, file 2 is a block device, and file 3 is a gluster protocol. But since we encode the type of file at the disk type='...' level, there is NO way to list three different source elements for those three quorum members. I think Benoit's proposal makes sense - a quorum is a node in the backing chain with NO source element, but instead has MULTIPLE backingStore elements. Ok, I reluctantly agree. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2.718/2] docs: add a serial device with a seclabel example
On 05/16/14 16:59, Ján Tomko wrote: --- docs/formatdomain.html.in | 6 ++ 1 file changed, 6 insertions(+) ACK, Peter signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2] conf: fix seclabels for chardevs
On 05/16/14 16:57, Ján Tomko wrote: We allow a seclabel to be specified in the source element of a chardev: serial type='file' source path='/tmp/serial.file' seclabel model='dac' relabel='no'/ /source /serial But we format it outside the source: serial type='file' source path='/tmp/serial.file'/ target port='0'/ seclabel model='dac' relabel='no'/ /serial Move the formatting inside the source to fix this to make the seclabel persistent across XML format-parse. Introduced by commit f8b08d0 'Add seclabel to character devices.' --- v2: don't crash (and add a test for that too) src/conf/domain_conf.c | 30 +++ .../qemuxml2argv-chardev-label.xml | 45 ++ tests/qemuxml2xmltest.c| 2 + 3 files changed, 62 insertions(+), 15 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-chardev-label.xml ACK, Peter signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] maint: fix typos related to disk name resolution
In a number of APIs, the text implied that a user might have target dev='xvda'/ - but common convention is to use vda, not xvda. For example, virDomainGetDiskErrors was correct, while virDomainBlockStats was confusing. * src/libvirt.c: Make examples consistent. Signed-off-by: Eric Blake ebl...@redhat.com --- Copy and paste is to blame for the size of this patch :) Pushing under the trivial rule. src/libvirt.c | 22 +++--- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index ccb7113..19fa18b 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -7850,7 +7850,7 @@ virDomainSetSchedulerParametersFlags(virDomainPtr domain, * devices attached to the domain. * * The @disk parameter is either the device target shorthand (the - * target dev='...'/ sub-element, such as xvda), or (since 0.9.8) + * target dev='...'/ sub-element, such as vda), or (since 0.9.8) * an unambiguous source name of the block device (the source * file='...'/ sub-element, such as /path/to/image). Valid names * can be found by calling virDomainGetXMLDesc() and inspecting @@ -7918,7 +7918,7 @@ virDomainBlockStats(virDomainPtr dom, const char *disk, * devices attached to the domain. * * The @disk parameter is either the device target shorthand (the - * target dev='...'/ sub-element, such as xvda), or (since 0.9.8) + * target dev='...'/ sub-element, such as vda), or (since 0.9.8) * an unambiguous source name of the block device (the source * file='...'/ sub-element, such as /path/to/image). Valid names * can be found by calling virDomainGetXMLDesc() and inspecting @@ -8270,7 +8270,7 @@ virDomainMemoryStats(virDomainPtr dom, virDomainMemoryStatPtr stats, * The @disk parameter is either an unambiguous source name of the * block device (the source file='...'/ sub-element, such as * /path/to/image), or (since 0.9.5) the device target shorthand - * (the target dev='...'/ sub-element, such as xvda). Valid names + * (the target dev='...'/ sub-element, such as vda). Valid names * can be found by calling virDomainGetXMLDesc() and inspecting * elements within //domain/devices/disk. * @@ -8348,7 +8348,7 @@ virDomainBlockPeek(virDomainPtr dom, * The @disk parameter is either an unambiguous source name of the * block device (the source file='...'/ sub-element, such as * /path/to/image), or (since 0.9.5) the device target shorthand - * (the target dev='...'/ sub-element, such as xvda). Valid names + * (the target dev='...'/ sub-element, such as vda). Valid names * can be found by calling virDomainGetXMLDesc() and inspecting * elements within //domain/devices/disk. * @@ -8510,7 +8510,7 @@ virDomainMemoryPeek(virDomainPtr dom, * The @disk parameter is either an unambiguous source name of the * block device (the source file='...'/ sub-element, such as * /path/to/image), or (since 0.9.5) the device target shorthand - * (the target dev='...'/ sub-element, such as xvda). Valid names + * (the target dev='...'/ sub-element, such as vda). Valid names * can be found by calling virDomainGetXMLDesc() and inspecting * elements within //domain/devices/disk. * @@ -19448,7 +19448,7 @@ virDomainOpenChannel(virDomainPtr dom, * The @disk parameter is either an unambiguous source name of the * block device (the source file='...'/ sub-element, such as * /path/to/image), or (since 0.9.5) the device target shorthand - * (the target dev='...'/ sub-element, such as xvda). Valid names + * (the target dev='...'/ sub-element, such as vda). Valid names * can be found by calling virDomainGetXMLDesc() and inspecting * elements within //domain/devices/disk. * @@ -19522,7 +19522,7 @@ virDomainBlockJobAbort(virDomainPtr dom, const char *disk, * The @disk parameter is either an unambiguous source name of the * block device (the source file='...'/ sub-element, such as * /path/to/image), or (since 0.9.5) the device target shorthand - * (the target dev='...'/ sub-element, such as xvda). Valid names + * (the target dev='...'/ sub-element, such as vda). Valid names * can be found by calling virDomainGetXMLDesc() and inspecting * elements within //domain/devices/disk. * @@ -19576,7 +19576,7 @@ virDomainGetBlockJobInfo(virDomainPtr dom, const char *disk, * The @disk parameter is either an unambiguous source name of the * block device (the source file='...'/ sub-element, such as * /path/to/image), or (since 0.9.5) the device target shorthand - * (the target dev='...'/ sub-element, such as xvda). Valid names + * (the target dev='...'/ sub-element, such as vda). Valid names * can be found by calling virDomainGetXMLDesc() and inspecting * elements within //domain/devices/disk. * @@ -19633,7 +19633,7 @@ virDomainBlockJobSetSpeed(virDomainPtr dom, const char *disk, * The @disk parameter is either an unambiguous source name of the * block device (the source file='...'/ sub-element, such as * /path/to/image), or (since 0.9.5)
[libvirt] RFC: Any interest in a weekly(?) dev community meeting ?
Hi Libvirt team, A number of opensource projects have weekly meetings between their community of contributors to facilitate their day-to-day working and particularly to resolve roadblocks that people are having. I feel that libvirt is large enough, with contributors from many different organizations, that a meeting could be beneficial to our operation. This could serve a number of purposes - Remind us of patches that have been posted and accidentally forgotten by reviewers - Resolve hard debates that are not making adequate progress on the mailing list(s) - Track progress of major ongoing pieces of work with many collaborators - Forum for those new to the community to introduce themselves their ideas before starting work - Discuss release critical bugs during freeze periods - Place for downstream users of libvirt (eg openstack/ovirt/etc) to interact with libvirt team. - Place for projects we use (eg KVM, Xen) to interact with libvirt team. I like to keep the overhead of this low, so I'd suggest we try todo it on IRC, since that has been fairly effective for OpenStack teams. If people think this is worth while I'd suggest an arbitrary time of 1500 UTC using the #virt-meeting IRC channel on irc.oftc.net, to last an absolute max of 1 hour. Currently this time point works out as 08:00 San Francisco 11:00 Boston 15:00 UTC 16:00 London 17:00 Berlin 20:30 Mumbai 23:00 Bejing 24:00 Tokyo http://www.timeanddate.com/worldclock/fixedtime.html?hour=15min=00sec=0p1=0 Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] RFC: Any interest in a weekly(?) dev community meeting ?
On Fri, May 16, 2014 at 01:53:50PM -0400, Daniel P. Berrange wrote: Hi Libvirt team, A number of opensource projects have weekly meetings between their community of contributors to facilitate their day-to-day working and particularly to resolve roadblocks that people are having. I feel that libvirt is large enough, with contributors from many different organizations, that a meeting could be beneficial to our operation. This could serve a number of purposes - Remind us of patches that have been posted and accidentally forgotten by reviewers - Resolve hard debates that are not making adequate progress on the mailing list(s) - Track progress of major ongoing pieces of work with many collaborators - Forum for those new to the community to introduce themselves their ideas before starting work - Discuss release critical bugs during freeze periods - Place for downstream users of libvirt (eg openstack/ovirt/etc) to interact with libvirt team. - Place for projects we use (eg KVM, Xen) to interact with libvirt team. I like to keep the overhead of this low, so I'd suggest we try todo it on IRC, since that has been fairly effective for OpenStack teams. If people think this is worth while I'd suggest an arbitrary time of 1500 UTC using the #virt-meeting IRC channel on irc.oftc.net, to last an absolute max of 1 hour. Currently this time point works out as Opps, missd the day - I was meaning to suggest *1500 UTC on thursdays* 08:00 San Francisco 11:00 Boston 15:00 UTC 16:00 London 17:00 Berlin 20:30 Mumbai 23:00 Bejing 24:00 Tokyo http://www.timeanddate.com/worldclock/fixedtime.html?hour=15min=00sec=0p1=0 Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/4] maint: prefer enum over int for virstoragefile structs
On 05/14/2014 04:45 PM, Eric Blake wrote:
For internal structs, we might as well be type-safe and let the
compiler help us with less typing required on our part (getting
rid of casts is always nice). In trying to use enums directly,
I noticed two problems in virstoragefile.h that can't be fixed
without more invasive refactoring: virStorageSource.format is
used as more of a union of multiple enums in storage volume
code (so it has to remain an int), and virStorageSourcePoolDef
refers to pooltype whose enum is declared in src/conf, but where
src/util can't pull in headers from src/conf.
I'm probably going to have to revert this patch :(
There is a subtle bug here, where the set of machines where the bug
surfaces depends on the whims of the compiler.
CC conf/libvirt_conf_la-domain_conf.lo
conf/domain_conf.c: In function 'virDomainDiskSourceParse':
conf/domain_conf.c:4992:9: error: comparison of unsigned expression 0
is always false [-Werror=type-limits]
Basically, compilers are allowed to choose whether an enum value with no
negative constants behaves as signed or unsigned; if the compiler
chooses unsigned, but we then do 'if ((value = virBlahFromString(str))
0)', we cause the compiler warning; if the compiler chooses signed, then
everything just works. Worse, if we do 'if ((value =
virBlahFromString(str)) = 0)', the compiler silently does the wrong
thing (-1 returned from virBlahFromString is promoted to unsigned, so
the condition is false, and we are stuck using an out-of-range value
without warning).
On IRC, I talked about several workarounds that could keep us with an
enum type; each looks worse than what this change would cure, which is
why I think reverting is the only sane option.
1. Force the comparison in int (yucky because it adds a cast, but my
stated intention of this commit was to avoid casts, and requires a code
audit):
if ((int)(value = virBlahFromString(str)) = 0)
2. Use temporaries (no ugly casts, but requires a code audit):
int tmp;
if ((tmp = virBlahFromString(str)) 0)
...
value = tmp;
3. Force the enum to be signed (yucky because we have to inject an
otherwise unused -1 element in each enum):
typedef enum {
VIR_BLAH__FORCE_SIGNED = -1,
VIR_BLAH_A,
VIR_BLAH_B,
VIR_BLAH_LAST
} virBlah;
4. Enhance VIR_ENUM_DECL/VIR_ENUM_IMPL to provide yet another conversion
function. The existing virBlahFromString(str) returns -1 on failure or
enum values on success; the new virBlahEnumFromString(str) returns -1 on
failure or 0 on success with the correct value stored into the address.
(yucky because you now have to know which of the two FromString variants
to call based on what type you are assigning into, because 'int*' and
'enum*' are not compatible types):
enum value;
int other;
if (virBlahEnumFromString(str, value) 0)
...
if ((other = virBlahFromString(str)) 0)
,,,
5. Alter VIR_ENUM_DECL/VIR_ENUM_IMPL to make the FromString variant have
new semantics - the _LAST value is now hard-coded as the failure value
rather than -1, and the return signature is an enum (ugly because it
requires a full tree audit, and because checking for ==_LAST is not as
nice as checking for 0; and it gets worse when also excluding 0)
if ((value = virBlahFromString(str)) == VIR_BLAH_LAST)
...error about unknown value
value = virBlahFromString(str);
if (!value || value == VIR_BLAH_LAST)
...error about bad value
Any other opinions, or should I just go ahead with the revert? This is
breaking the build for some people, depending on their gcc version.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/4] maint: prefer enum over int for virstoragefile structs
Eric Blake wrote:
On 05/14/2014 04:45 PM, Eric Blake wrote:
For internal structs, we might as well be type-safe and let the
compiler help us with less typing required on our part (getting
rid of casts is always nice). In trying to use enums directly,
I noticed two problems in virstoragefile.h that can't be fixed
without more invasive refactoring: virStorageSource.format is
used as more of a union of multiple enums in storage volume
code (so it has to remain an int), and virStorageSourcePoolDef
refers to pooltype whose enum is declared in src/conf, but where
src/util can't pull in headers from src/conf.
I'm probably going to have to revert this patch :(
There is a subtle bug here, where the set of machines where the bug
surfaces depends on the whims of the compiler.
CC conf/libvirt_conf_la-domain_conf.lo
conf/domain_conf.c: In function 'virDomainDiskSourceParse':
conf/domain_conf.c:4992:9: error: comparison of unsigned expression 0
is always false [-Werror=type-limits]
Basically, compilers are allowed to choose whether an enum value with no
negative constants behaves as signed or unsigned; if the compiler
chooses unsigned, but we then do 'if ((value = virBlahFromString(str))
0)', we cause the compiler warning; if the compiler chooses signed, then
everything just works. Worse, if we do 'if ((value =
virBlahFromString(str)) = 0)', the compiler silently does the wrong
thing (-1 returned from virBlahFromString is promoted to unsigned, so
the condition is false, and we are stuck using an out-of-range value
without warning).
On IRC, I talked about several workarounds that could keep us with an
enum type; each looks worse than what this change would cure, which is
why I think reverting is the only sane option.
1. Force the comparison in int (yucky because it adds a cast, but my
stated intention of this commit was to avoid casts, and requires a code
audit):
if ((int)(value = virBlahFromString(str)) = 0)
2. Use temporaries (no ugly casts, but requires a code audit):
int tmp;
if ((tmp = virBlahFromString(str)) 0)
...
value = tmp;
I have a patch along these lines to fix the build failures, but agree
that it does not sit well with your intentions of this patch.
3. Force the enum to be signed (yucky because we have to inject an
otherwise unused -1 element in each enum):
typedef enum {
VIR_BLAH__FORCE_SIGNED = -1,
VIR_BLAH_A,
VIR_BLAH_B,
VIR_BLAH_LAST
} virBlah;
4. Enhance VIR_ENUM_DECL/VIR_ENUM_IMPL to provide yet another conversion
function. The existing virBlahFromString(str) returns -1 on failure or
enum values on success; the new virBlahEnumFromString(str) returns -1 on
failure or 0 on success with the correct value stored into the address.
(yucky because you now have to know which of the two FromString variants
to call based on what type you are assigning into, because 'int*' and
'enum*' are not compatible types):
enum value;
int other;
if (virBlahEnumFromString(str, value) 0)
...
if ((other = virBlahFromString(str)) 0)
,,,
5. Alter VIR_ENUM_DECL/VIR_ENUM_IMPL to make the FromString variant have
new semantics - the _LAST value is now hard-coded as the failure value
rather than -1, and the return signature is an enum (ugly because it
requires a full tree audit, and because checking for ==_LAST is not as
nice as checking for 0; and it gets worse when also excluding 0)
if ((value = virBlahFromString(str)) == VIR_BLAH_LAST)
...error about unknown value
value = virBlahFromString(str);
if (!value || value == VIR_BLAH_LAST)
...error about bad value
Heh, hard to argue that any of these options _improve_ the code base.
Any other opinions, or should I just go ahead with the revert? This is
breaking the build for some people, depending on their gcc version.
I'm torn. This intent of the patch is a nice improvement, but with
diminishing returns when adjusting it to work with some versions of gcc
and clang.
Regards,
Jim
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] blockcommit: document semantics of committing active layer
Now that qemu 2.0 allows commit of the active layer, people are
attempting to use virsh blockcommit and getting into a stuck
state, because libvirt is unprepared to handle the two-phase
commit required by qemu.
Stepping back a bit, there are two valid semantics for a
commit operation:
1. Maintain a 'golden' base, and a transient overlay. Make
changes in the overlay, and if everything appears to work,
commit those changes into the base, but still keep the overlay
for the next round of changes; repeat the cycle as desired.
2. Create an external snapshot, then back up the stable state
in the backing file. Once the backup is complete, commit the
overlay back into the base, and delete the temporary snapshot.
Since qemu doesn't know up front which of the two styles is
preferred, a block commit of the active layer merely gets
the job into a synchronized state, and sends an event; then
the user must either cancel (case 1) or complete (case 2),
where qemu then sends a second event that actually ends the
job. However, libvirt was blindly assuming the semantics that
apply to a commit of an intermediate image, where there is
only one sane conclusion (the job automatically ends with
fewer elements in the chain); and getting stuck because it
wasn't prepared for qemu to enter a second phase of the job.
This patch adds a flag to the libvirt API that a user MUST
supply in order to acknowledge that they will be using two-phase
semantics. It might be possible to have a mode where if the
flag is omitted, we automatically do the case 2 semantics on
the user's behalf; but before that happens, I must do additional
patches to track the fact that we are doing an active commit
in the domain XML. So the safest thing for now is to reject the
new flag in qemu, as well as prevent commits of the active layer
without the flag. Later patches will add support of the flag,
and once 2-phase semantics are working, we can then decide
whether to relax things to allow an omitted flag to cause an
automatic pivot.
* include/libvirt/libvirt.h.in (VIR_DOMAIN_BLOCK_COMMIT_ACTIVE)
(VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT): New enums.
* src/libvirt.c (virDomainBlockCommit): Document two-phase job
when committing active layer, through new flag.
(virDomainBlockJobAbort): Document that pivot also occurs after
active commit.
* src/qemu/qemu_driver.c (qemuDomainBlockCommit): Explicitly
reject active copy; later patches will add it in.
Signed-off-by: Eric Blake ebl...@redhat.com
---
This patch should probably be backported, even if later patches
in the series are not, so that users avoid getting hung.
include/libvirt/libvirt.h.in | 12 ++
src/libvirt.c| 55 +---
src/qemu/qemu_driver.c | 9
3 files changed, 54 insertions(+), 22 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index 260c971..127de11 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -2513,6 +2513,7 @@ typedef enum {
VIR_DOMAIN_BLOCK_JOB_TYPE_PULL = 1,
VIR_DOMAIN_BLOCK_JOB_TYPE_COPY = 2,
VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT = 3,
+VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT = 4,
#ifdef VIR_ENUM_SENTINELS
VIR_DOMAIN_BLOCK_JOB_TYPE_LAST
@@ -2523,7 +2524,8 @@ typedef enum {
* virDomainBlockJobAbortFlags:
*
* VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC: Request only, do not wait for completion
- * VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT: Pivot to mirror when ending a copy job
+ * VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT: Pivot to new file when ending a copy or
+ * active commit job
*/
typedef enum {
VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC = 1 0,
@@ -2588,6 +2590,8 @@ typedef enum {
VIR_DOMAIN_BLOCK_COMMIT_DELETE = 1 1, /* Delete any files that are now
invalid after their contents
have been committed */
+VIR_DOMAIN_BLOCK_COMMIT_ACTIVE = 1 2, /* Allow a two-phase commit when
+ top is the active layer */
} virDomainBlockCommitFlags;
int virDomainBlockCommit(virDomainPtr dom, const char *disk, const char *base,
@@ -4823,8 +4827,8 @@ typedef void
(*virConnectDomainEventGraphicsCallback)(virConnectPtr conn,
/**
* virConnectDomainEventBlockJobStatus:
*
- * The final status of a virDomainBlockPull() or virDomainBlockRebase()
- * operation
+ * Tracks status of a virDomainBlockPull(), virDomainBlockRebase(),
+ * or virDomainBlockCommit() operation
*/
typedef enum {
VIR_DOMAIN_BLOCK_JOB_COMPLETED = 0,
@@ -4843,7 +4847,7 @@ typedef enum {
* @dom: domain on which the event occurred
* @disk: fully-qualified filename of the affected disk
* @type: type of block job (virDomainBlockJobType)
- * @status: final status of the operation (virConnectDomainEventBlockJobStatus)
+ * @status: status of the operation (virConnectDomainEventBlockJobStatus)
Re: [libvirt] [PATCH V3 7/7] security_dac: honor relabel='no' in chardev config
Ján Tomko wrote: On 05/16/2014 06:16 AM, Jim Fehlig wrote: The DAC driver ignores the relabel='no' attribute in chardev config serial type='file' source path='/tmp/jim/test.file' seclabel model='dac' relabel='no'/ /source target port='0'/ /serial This patch avoids labeling chardevs when relabel='no' is specified. Signed-off-by: Michal Privoznik mpriv...@redhat.com Signed-off-by: Jim Fehlig jfeh...@suse.com --- src/security/security_dac.c | 65 - 1 file changed, 46 insertions(+), 19 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 4434cd0..20f349f 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -705,25 +707,35 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); -if (virSecurityDACGetIds(seclabel, priv, user, group, NULL, NULL)) -return -1; +if (dev) +chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev, + SECURITY_DAC_NAME); A check for seclabel-norelabel and chr_seclabel-norelabel is missing here. virSecurityDACSetChardevLabel() is only called internally, and in all cases via virSecurityDACSetSecurityAllLabel(), which already checks for seclabel-norelabel. But you are right about the missing check for chr_seclabel-norelabel. I added it to the patch before pushing. Regards, Jim -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V3 0/7] Honor DAC norelabel attribute
Ján Tomko wrote: On 05/16/2014 06:16 AM, Jim Fehlig wrote: V3 of Michal's series to honor relabel='no' in device config https://www.redhat.com/archives/libvir-list/2014-April/msg00196.html In V3, the patches have been further split to ease review as requested by Jan Tomko. Jim Fehlig (7): security_dac: annotate some functions with ATTRIBUTE_NONNULL security_dac: cleanup use of enum types security_dac: rework callback parameter passing security_dac: avoid relabeling when relabel='no' security_dac: honor relabel='no' in disk config security_dac: avoid relabeling hostdevs when relabel='no' security_dac: honor relabel='no' in chardev config src/security/security_dac.c | 333 +++- 1 file changed, 203 insertions(+), 130 deletions(-) ACK series with 7/7 fixed. I fixed the indentation in 6/7, addressed your comment on 7/7 (see my response), and pushed the series. Thanks for the review! Regards, Jim -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Revert maint: prefer enum over int for virstoragefile structs
This partially reverts commits b279e52f7 and ea18f8b2.
It turns out our code base is full of:
if ((struct.member = virBlahFromString(str)) 0)
goto error;
Meanwhile, the C standard says it is up to the compiler whether
an enum is signed or unsigned when all of its declared values
happen to be positive. In my testing (Fedora 20, gcc 4.8.2),
the compiler picked signed, and nothing changed. But others
testing with gcc 4.7 got compiler warnings, because it picked
the enum to be unsigned, but no unsigned value is less than 0.
Even worse:
if ((struct.member = virBlahFromString(str)) = 0)
goto error;
is silently compiled without warning, but incorrectly treats -1
from a bad parse as a large positive number with no warning; and
without the compiler's help to find these instances, it is a
nightmare to maintain correctly. We could force signed enums
a dummy negative declaration in each enum, or cast the result of
virBlahFromString back to int after assigning to an enum value,
but that's uglier than what we were trying to cure by directly
using enum types for struct values. It's better off to just
live with int members, and use 'switch ((virFoo) struct.member)'
where we want the compiler to help, than to track down all the
conversions from string to enum and ensure they don't suffer
from type problems.
* src/util/virstorageencryption.h: Revert back to int declarations
with comment about enum usage.
* src/util/virstoragefile.h: Likewise.
* src/conf/domain_conf.c: Restore back to casts in switches.
* src/qemu/qemu_driver.c: Likewise.
* src/qemu/qemu_command.c: Add cast rather than revert.
---
src/conf/domain_conf.c | 4 ++--
src/qemu/qemu_command.c | 2 +-
src/qemu/qemu_driver.c | 8
src/util/virstorageencryption.h | 4 ++--
src/util/virstoragefile.h | 14 +++---
5 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e5ae7c6..e65b62b 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4972,7 +4972,7 @@ virDomainDiskSourceParse(xmlNodePtr node,
memset(host, 0, sizeof(host));
-switch (src-type) {
+switch ((virStorageType)src-type) {
case VIR_STORAGE_TYPE_FILE:
src-path = virXMLPropString(node, file);
break;
@@ -14847,7 +14847,7 @@ virDomainDiskSourceFormat(virBufferPtr buf,
startupPolicy = virDomainStartupPolicyTypeToString(policy);
if (src-path || src-nhosts 0 || src-srcpool || startupPolicy) {
-switch (src-type) {
+switch ((virStorageType)src-type) {
case VIR_STORAGE_TYPE_FILE:
virBufferAddLit(buf, source);
virBufferEscapeString(buf, file='%s', src-path);
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 9ae1a96..193959f 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -11021,7 +11021,7 @@ qemuParseCommandLine(virCapsPtr qemuCaps,
if (disk-src.type == VIR_STORAGE_TYPE_NETWORK) {
char *port;
-switch (disk-src.protocol) {
+switch ((virStorageNetProtocol) disk-src.protocol) {
case VIR_STORAGE_NET_PROTOCOL_NBD:
if (qemuParseNBDString(disk) 0)
goto error;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 55b4755..cab653b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12368,7 +12368,7 @@
qemuDomainSnapshotPrepareDiskExternalBackingInactive(virDomainDiskDefPtr disk)
return 0;
case VIR_STORAGE_TYPE_NETWORK:
-switch (disk-src.protocol) {
+switch ((virStorageNetProtocol) disk-src.protocol) {
case VIR_STORAGE_NET_PROTOCOL_NBD:
case VIR_STORAGE_NET_PROTOCOL_RBD:
case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG:
@@ -12430,7 +12430,7 @@
qemuDomainSnapshotPrepareDiskExternalOverlayActive(virDomainSnapshotDiskDefPtr d
return 0;
case VIR_STORAGE_TYPE_NETWORK:
-switch (disk-src.protocol) {
+switch ((virStorageNetProtocol) disk-src.protocol) {
case VIR_STORAGE_NET_PROTOCOL_GLUSTER:
return 0;
@@ -12575,7 +12575,7 @@ qemuDomainSnapshotPrepareDiskInternal(virConnectPtr
conn,
return 0;
case VIR_STORAGE_TYPE_NETWORK:
-switch (disk-src.protocol) {
+switch ((virStorageNetProtocol) disk-src.protocol) {
case VIR_STORAGE_NET_PROTOCOL_NBD:
case VIR_STORAGE_NET_PROTOCOL_RBD:
case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG:
@@ -12801,7 +12801,7 @@
qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,
VIR_STRDUP(persistSource, snap-src.path) 0)
goto cleanup;
-switch (snap-src.type) {
+switch ((virStorageType)snap-src.type) {
case VIR_STORAGE_TYPE_BLOCK:
reuse = true;
/* fallthrough */
diff --git a/src/util/virstorageencryption.h b/src/util/virstorageencryption.h
index
