- Devel - libvirt List Archives

[libvirt] A couple of questions on snapshot-create/ snapshot-create-as
by Kashyap Chamarthy 30 Apr '12

30 Apr '12

Hi (Eric?), I got a couple of questions for 'snapshot-create' / 'snapshot-create-as' : 1/ can '--disk-only' option (when used with virsh snapshot-create-as) be run on an offline guest and generate an 'external snapshot' ? Or a running guest is needed? 2/ Can anyone point me to a bit more verbose info on these below options while creating snapshots? --quiesce quiesce guest's file systems --atomic require atomic operation --reuse-external reuse any existing external files Thanks. /kashyap

2 2

[libvirt] [PATCH] storage: Break out the loop if duplicate pool is found
by Osier Yang 30 Apr '12

30 Apr '12

It doesn't break out the "for" loop even if duplicate pool is found, and thus the "matchpool" could be overriden as NULL again if there is different pool afterwards. To address the problem in libvirt-user list: https://www.redhat.com/archives/libvirt-users/2012-April/msg00150.html --- src/conf/storage_conf.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 7579327..9a42e34 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1710,6 +1710,9 @@ int virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools, break; } virStoragePoolObjUnlock(pool); + + if (matchpool) + break; } if (matchpool) { -- 1.7.7.3

2 2

[libvirt] [PATCH] storage: Allow multiple hosts for a storage pool
by Wido den Hollander 30 Apr '12

30 Apr '12

The current storage pools for NFS and iSCSI only require one host to connect to. Future storage pools like RBD and Sheepdog will require multiple hosts. This patch allows multiple source hosts and rewrites the current storage drivers. Signed-off-by: Wido den Hollander <wido(a)widodh.nl> --- src/conf/storage_conf.c | 59 ++++++++++++++++++++++++---------- src/conf/storage_conf.h | 5 ++- src/esx/esx_storage_driver.c | 6 +++- src/storage/storage_backend_fs.c | 12 +++--- src/storage/storage_backend_iscsi.c | 12 +++--- src/test/test_driver.c | 4 +- 6 files changed, 63 insertions(+), 35 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 2330fa1..aa23ff3 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -276,7 +276,11 @@ virStoragePoolSourceClear(virStoragePoolSourcePtr source) if (!source) return; - VIR_FREE(source->host.name); + for (i = 0 ; i < source->nhost ; i++) { + VIR_FREE(source->hosts[i].name); + } + VIR_FREE(source->hosts); + for (i = 0 ; i < source->ndevice ; i++) { VIR_FREE(source->devices[i].freeExtents); VIR_FREE(source->devices[i].path); @@ -404,6 +408,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, char *authType = NULL; int nsource, i; virStoragePoolOptionsPtr options; + char *name = NULL; char *port = NULL; relnode = ctxt->node; @@ -431,17 +436,34 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, VIR_FREE(format); } - source->host.name = virXPathString("string(./host/@name)", ctxt); - port = virXPathString("string(./host/@port)", ctxt); - if (port) { - if (virStrToLong_i(port, NULL, 10, &source->host.port) < 0) { - virStorageReportError(VIR_ERR_XML_ERROR, - _("Invalid port number: %s"), - port); + source->nhost = virXPathNodeSet("./host", ctxt, &nodeset); + + if (source->nhost) { + if (VIR_ALLOC_N(source->hosts, source->nhost) < 0) { + virReportOOMError(); goto cleanup; } - } + for (i = 0 ; i < source->nhost ; i++) { + name = virXMLPropString(nodeset[i], "name"); + if (name == NULL) { + virStorageReportError(VIR_ERR_XML_ERROR, + "%s", _("missing storage pool host name")); + goto cleanup; + } + source->hosts[i].name = name; + + port = virXMLPropString(nodeset[i], "port"); + if (port) { + if (virStrToLong_i(port, NULL, 10, &source->hosts[i].port) < 0) { + virStorageReportError(VIR_ERR_XML_ERROR, + _("Invalid port number: %s"), + port); + goto cleanup; + } + } + } + } source->initiator.iqn = virXPathString("string(./initiator/iqn/@name)", ctxt); @@ -674,7 +696,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) { } if (options->flags & VIR_STORAGE_POOL_SOURCE_HOST) { - if (!ret->source.host.name) { + if (!ret->source.nhost) { virStorageReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage pool source host name")); @@ -800,12 +822,13 @@ virStoragePoolSourceFormat(virBufferPtr buf, int i, j; virBufferAddLit(buf," <source>\n"); - if ((options->flags & VIR_STORAGE_POOL_SOURCE_HOST) && - src->host.name) { - virBufferAsprintf(buf, " <host name='%s'", src->host.name); - if (src->host.port) - virBufferAsprintf(buf, " port='%d'", src->host.port); - virBufferAddLit(buf, "/>\n"); + if ((options->flags & VIR_STORAGE_POOL_SOURCE_HOST) && src->nhost) { + for (i = 0; i < src->nhost; i++) { + virBufferAsprintf(buf, " <host name='%s'", src->hosts[i].name); + if (src->hosts[i].port) + virBufferAsprintf(buf, " port='%d'", src->hosts[i].port); + virBufferAddLit(buf, "/>\n"); + } } if ((options->flags & VIR_STORAGE_POOL_SOURCE_DEVICE) && @@ -1677,7 +1700,7 @@ int virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools, break; case VIR_STORAGE_POOL_NETFS: if ((STREQ(pool->def->source.dir, def->source.dir)) \ - && (STREQ(pool->def->source.host.name, def->source.host.name))) + && (STREQ(pool->def->source.hosts[0].name, def->source.hosts[0].name))) matchpool = pool; break; case VIR_STORAGE_POOL_SCSI: @@ -1688,7 +1711,7 @@ int virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools, { matchpool = virStoragePoolSourceFindDuplicateDevices(pool, def); if (matchpool) { - if (STREQ(matchpool->def->source.host.name, def->source.host.name)) { + if (STREQ(matchpool->def->source.hosts[0].name, def->source.hosts[0].name)) { if ((matchpool->def->source.initiator.iqn) && (def->source.initiator.iqn)) { if (STREQ(matchpool->def->source.initiator.iqn, def->source.initiator.iqn)) break; diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 1ef9295..9222c4a 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -212,8 +212,9 @@ struct _virStoragePoolSourceDevice { typedef struct _virStoragePoolSource virStoragePoolSource; typedef virStoragePoolSource *virStoragePoolSourcePtr; struct _virStoragePoolSource { - /* An optional host */ - virStoragePoolSourceHost host; + /* An optional (maybe multiple) host(s) */ + size_t nhost; + virStoragePoolSourceHostPtr hosts; /* And either one or more devices ... */ int ndevice; diff --git a/src/esx/esx_storage_driver.c b/src/esx/esx_storage_driver.c index adf1edb..9b64891 100644 --- a/src/esx/esx_storage_driver.c +++ b/src/esx/esx_storage_driver.c @@ -544,8 +544,12 @@ esxStoragePoolGetXMLDesc(virStoragePoolPtr pool, unsigned int flags) if (esxVI_LocalDatastoreInfo_DynamicCast(info) != NULL) { def.type = VIR_STORAGE_POOL_DIR; } else if ((nasInfo = esxVI_NasDatastoreInfo_DynamicCast(info)) != NULL) { + if (VIR_ALLOC_N(def.source.hosts, 1) < 0) { + virReportOOMError(); + goto cleanup; + } def.type = VIR_STORAGE_POOL_NETFS; - def.source.host.name = nasInfo->nas->remoteHost; + def.source.hosts[0].name = nasInfo->nas->remoteHost; def.source.dir = nasInfo->nas->remotePath; if (STRCASEEQ(nasInfo->nas->type, "NFS")) { diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 1af12e6..79eefd3 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -206,7 +206,7 @@ virStorageBackendFileSystemNetFindPoolSourcesFunc(virStoragePoolObjPtr pool ATTR if (!(src = virStoragePoolSourceListNewSource(&state->list))) goto cleanup; - if (!(src->host.name = strdup(state->host)) || + if (!(src->hosts[0].name = strdup(state->host)) || !(src->dir = strdup(path))) { virReportOOMError(); goto cleanup; @@ -260,8 +260,8 @@ virStorageBackendFileSystemNetFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSE if (!source) goto cleanup; - state.host = source->host.name; - prog[3] = source->host.name; + state.host = source->hosts[0].name; + prog[3] = source->hosts[0].name; if (virStorageBackendRunProgRegex(NULL, prog, 1, regexes, vars, virStorageBackendFileSystemNetFindPoolSourcesFunc, @@ -387,7 +387,7 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) { int ret; if (pool->def->type == VIR_STORAGE_POOL_NETFS) { - if (pool->def->source.host.name == NULL) { + if (pool->def->source.hosts[0].name == NULL) { virStorageReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing source host")); return -1; @@ -415,7 +415,7 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) { if (pool->def->type == VIR_STORAGE_POOL_NETFS) { if (virAsprintf(&src, "%s:%s", - pool->def->source.host.name, + pool->def->source.hosts[0].name, pool->def->source.dir) == -1) { virReportOOMError(); return -1; @@ -452,7 +452,7 @@ virStorageBackendFileSystemUnmount(virStoragePoolObjPtr pool) { int ret; if (pool->def->type == VIR_STORAGE_POOL_NETFS) { - if (pool->def->source.host.name == NULL) { + if (pool->def->source.hosts[0].name == NULL) { virStorageReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing source host")); return -1; diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index f04473d..3acc5fe 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -96,13 +96,13 @@ virStorageBackendISCSIPortal(virStoragePoolSourcePtr source) char ipaddr[NI_MAXHOST]; char *portal; - if (virStorageBackendISCSITargetIP(source->host.name, + if (virStorageBackendISCSITargetIP(source->hosts[0].name, ipaddr, sizeof(ipaddr)) < 0) return NULL; if (virAsprintf(&portal, "%s:%d,1", ipaddr, - source->host.port ? - source->host.port : 3260) < 0) { + source->hosts[0].port ? + source->hosts[0].port : 3260) < 0) { virReportOOMError(); return NULL; } @@ -581,7 +581,7 @@ virStorageBackendISCSIFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, virReportOOMError(); goto cleanup; } - list.sources[i].host = source->host; + list.sources[i].host = source->hosts[0]; list.sources[i].initiator = source->initiator; list.sources[i].ndevice = 1; list.sources[i].devices[0].path = targets[i]; @@ -617,7 +617,7 @@ virStorageBackendISCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED, *isActive = false; - if (pool->def->source.host.name == NULL) { + if (pool->def->source.hosts[0].name == NULL) { virStorageReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing source host")); return -1; @@ -649,7 +649,7 @@ virStorageBackendISCSIStartPool(virConnectPtr conn ATTRIBUTE_UNUSED, int ret = -1; const char *loginargv[] = { "--login", NULL }; - if (pool->def->source.host.name == NULL) { + if (pool->def->source.hosts[0].name == NULL) { virStorageReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing source host")); return -1; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 39ac503..73a7cb1 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4053,14 +4053,14 @@ testStorageFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, break; case VIR_STORAGE_POOL_NETFS: - if (!source || !source->host.name) { + if (!source || !source->hosts[0].name) { testError(VIR_ERR_INVALID_ARG, "%s", "hostname must be specified for netfs sources"); goto cleanup; } if (virAsprintf(&ret, defaultPoolSourcesNetFSXML, - source->host.name) < 0) + source->hosts[0].name) < 0) virReportOOMError(); break; -- 1.7.0.4

3 4

[libvirt] [PATCH] qemu: Make sure qemu can access its directory in hugetlbfs
by Jiri Denemark 29 Apr '12

29 Apr '12

When libvirtd is started, we create "libvirt/qemu" directories under hugetlbfs mount point. Only the "qemu" subdirectory is chowned to qemu user and "libvirt" remains owned by root. If umask was too restrictive when libvirtd started, qemu user may lose access to "qemu" subdirectory. Let's explicitly grant search permissions to "libvirt" directory for all users. --- src/qemu/qemu_driver.c | 27 +++++++++++++++++---------- src/util/virfile.c | 37 +++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 4 ++++ 3 files changed, 58 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ce31e09..29bab3c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -456,6 +456,8 @@ qemudStartup(int privileged) { int rc; virConnectPtr conn = NULL; char ebuf[1024]; + char *membase = NULL; + char *mempath = NULL; if (VIR_ALLOC(qemu_driver) < 0) return -1; @@ -660,23 +662,26 @@ qemudStartup(int privileged) { */ if (qemu_driver->hugetlbfs_mount && qemu_driver->hugetlbfs_mount[0] == '/') { - char *mempath = NULL; - if (virAsprintf(&mempath, "%s/libvirt/qemu", qemu_driver->hugetlbfs_mount) < 0) + if (virAsprintf(&membase, "%s/libvirt", + qemu_driver->hugetlbfs_mount) < 0 || + virAsprintf(&mempath, "%s/qemu", membase) < 0) goto out_of_memory; if (virFileMakePath(mempath) < 0) { virReportSystemError(errno, _("unable to create hugepage path %s"), mempath); - VIR_FREE(mempath); goto error; } - if (qemu_driver->privileged && - chown(mempath, qemu_driver->user, qemu_driver->group) < 0) { - virReportSystemError(errno, - _("unable to set ownership on %s to %d:%d"), - mempath, qemu_driver->user, qemu_driver->group); - VIR_FREE(mempath); - goto error; + if (qemu_driver->privileged) { + if (virFileUpdatePerm(membase, 0, S_IXGRP | S_IXOTH) < 0) + goto error; + if (chown(mempath, qemu_driver->user, qemu_driver->group) < 0) { + virReportSystemError(errno, + _("unable to set ownership on %s to %d:%d"), + mempath, qemu_driver->user, + qemu_driver->group); + goto error; + } } qemu_driver->hugepage_path = mempath; @@ -737,6 +742,8 @@ error: virConnectClose(conn); VIR_FREE(base); VIR_FREE(driverConf); + VIR_FREE(membase); + VIR_FREE(mempath); qemudShutdown(); return -1; } diff --git a/src/util/virfile.c b/src/util/virfile.c index 66160dc..db3d737 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -437,3 +437,40 @@ int virFileTouch(const char *path, mode_t mode) return 0; } + + +#define MODE_BITS (S_ISUID | S_ISGID | S_ISVTX | S_IRWXU | S_IRWXG | S_IRWXO) + +int virFileUpdatePerm(const char *path, + mode_t mode_remove, + mode_t mode_add) +{ + struct stat sb; + mode_t mode; + + if (mode_remove & ~MODE_BITS || mode_add & ~MODE_BITS) { + virFileError(VIR_ERR_INVALID_ARG, "%s", _("invalid mode")); + return -1; + } + + if (stat(path, &sb) < 0) { + virReportSystemError(errno, _("cannot stat '%s'"), path); + return -1; + } + + mode = sb.st_mode & MODE_BITS; + + if ((mode & mode_remove) == 0 && (mode & mode_add) == mode_add) + return 0; + + mode &= MODE_BITS ^ mode_remove; + mode |= mode_add; + + if (chmod(path, mode) < 0) { + virReportSystemError(errno, _("cannot change permission of '%s'"), + path); + return -1; + } + + return 0; +} diff --git a/src/util/virfile.h b/src/util/virfile.h index 184677c..05f5048 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -83,4 +83,8 @@ int virFileRewrite(const char *path, int virFileTouch(const char *path, mode_t mode); +int virFileUpdatePerm(const char *path, + mode_t mode_remove, + mode_t mode_add); + #endif /* __VIR_FILES_H */ -- 1.7.8.6

2 2

[libvirt] libvirt stable releases
by Cole Robinson 29 Apr '12

29 Apr '12

Hi all, An idea we've kicked around for awhile in Red Hat/Fedora land is doing official libvirt stable releases, but nothing ever took shape. The idea was brought up again recently and I've offered to help get something going. I've pushed an upstream v0.9.11-maint branch with a bunch of patches cherry-picked to libvirt 0.9.11. Shortly I'll be cutting a 0.9.11.1 and pushing it to the website, like other releases. Why 0.9.11? Because that's what we will be shipping in Fedora 17 :) Typically our policy with fedora is to stick with one libvirt version for the length of a release. Cutting stable releases should save us from having to backport patches, and hopefully get us more bug fixes than backporting only what our users report. So I don't plan on doing a similar branch for 0.9.12 or 0.9.13, but will probably do a branch in 6 months time for whatever libvirt version we are shipping in Fedora 18. While my primary motiviation at the moment is making Fedora maintenance easier, I hope that other distros will use the stable releases as well, where we can all benefit from each others QA and attention. Any feedback appreciated! Thanks, Cole

5 7

[libvirt] Schedule for next release libvirt-0.9.12
by Daniel Veillard 28 Apr '12

28 Apr '12

Assuming we try to stay on a monthly schedule, then I would suggest to enter the freeze around next Friday (27th April) and shoot for a release around the 4th of May. I'm supposed to be in vacation most of next week, but I think I will still be able to make the release candidate 1 We have a number of patch set which need further review to be pushed fully, and there are a few things in http://libvirt.org/pending.html which aren't false positive that we really ought to check and push :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

2 2

[libvirt] [PATCH] build: Fix version of gettext macros
by Peter Krempa 28 Apr '12

28 Apr '12

Commit c9cd419caba9effa11ca53e8696e5f6a4b424d60 added copying of the makefile for translation files from gnulib. The makefile from gnulib is of version 0.18 but the build configuration cretes macros from version 0.17 which breaks the build with message: *** error: gettext infrastructure mismatch: using a Makefile.in.in from gettext version 0.18 but the autoconf macros are from gettext version 0.17 --- bootstrap.conf | 2 +- configure.ac | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/bootstrap.conf b/bootstrap.conf index c6620e5..56911e0 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -195,7 +195,7 @@ buildreq="\ autoconf 2.59 automake 1.9.6 autopoint - -gettext 0.17 +gettext 0.18 git 1.5.5 gzip - libtool - diff --git a/configure.ac b/configure.ac index 89fe818..5936d31 100644 --- a/configure.ac +++ b/configure.ac @@ -2300,7 +2300,7 @@ dnl compute the difference between save_CPPFLAGS and CPPFLAGS and append it dnl to INCLUDES in order to preserve changes made by gettext but in a place dnl that does not break the build save_CPPFLAGS="$CPPFLAGS" -AM_GNU_GETTEXT_VERSION([0.17]) +AM_GNU_GETTEXT_VERSION([0.18]) AM_GNU_GETTEXT([external]) GETTEXT_CPPFLAGS= if test "x$save_CPPFLAGS" != "x$CPPFLAGS"; then -- 1.7.3.4

5 14

[libvirt] [PATCH v2] qemu_agent: Report error class at least
by Michal Privoznik 27 Apr '12

27 Apr '12

Currently, qemu GA is not providing 'desc' field for errors like we are used to from qemu monitor. Therefore, we fall back to this general 'unknown error' string. However, GA is reporting 'class' which is not perfect, but much more helpful than generic error string. Thus we should fall back to class firstly and if even no class is presented, then we can fall back to that generic string. Before this patch: virsh # dompmsuspend --target mem f16 error: Domain f16 could not be suspended error: internal error unable to execute QEMU command 'guest-suspend-ram': unknown QEMU command error After this patch: virsh # dompmsuspend --target mem f16 error: Domain f16 could not be suspended error: internal error unable to execute QEMU command 'guest-suspend-ram': The command has not been found --- src/qemu/qemu_agent.c | 37 +++++++++++++++++++++++++++++++++++-- 1 files changed, 35 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index b759b7f..bc4ceff 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1025,6 +1025,40 @@ cleanup: return ret; } +static const char * +qemuAgentStringifyErrorClass(const char *klass) +{ + if (STREQ_NULLABLE(klass, "BufferOverrun")) + return "Buffer overrun"; + else if (STREQ_NULLABLE(klass, "CommandDisabled")) + return "The command has been disabled for this instance"; + else if (STREQ_NULLABLE(klass, "CommandNotFound")) + return "The command has not been found"; + else if (STREQ_NULLABLE(klass, "FdNotFound")) + return "File descriptor not found"; + else if (STREQ_NULLABLE(klass, "InvalidParameter")) + return "Invalid parameter"; + else if (STREQ_NULLABLE(klass, "InvalidParameterType")) + return "Invalid parameter type"; + else if (STREQ_NULLABLE(klass, "InvalidParameterValue")) + return "Invalid parameter value"; + else if (STREQ_NULLABLE(klass, "OpenFileFailed")) + return "Cannot open file"; + else if (STREQ_NULLABLE(klass, "QgaCommandFailed")) + return "Guest agent command failed"; + else if (STREQ_NULLABLE(klass, "QMPBadInputObjectMember")) + return "Bad QMP input object member"; + else if (STREQ_NULLABLE(klass, "QMPExtraInputObjectMember")) + return "Unexpected extra object member"; + else if (STREQ_NULLABLE(klass, "UndefinedError")) + return "An undefined error has occurred"; + else if (STREQ_NULLABLE(klass, "Unsupported")) + return "this feature or command is not currently supported"; + else if (klass) + return klass; + else + return "unknown QEMU command error"; +} /* Ignoring OOM in this method, since we're already reporting * a more important error @@ -1043,9 +1077,8 @@ qemuAgentStringifyError(virJSONValuePtr error) if (klass) detail = virJSONValueObjectGetString(error, "desc"); - if (!detail) - detail = "unknown QEMU command error"; + detail = qemuAgentStringifyErrorClass(klass); return detail; } -- 1.7.8.5

2 3

[libvirt] [PATCH] More coverity findings addressed
by Stefan Berger 27 Apr '12

27 Apr '12

More bug extermination in the category of: Error: CHECKED_RETURN: /libvirt/src/conf/network_conf.c:595: check_return: Calling function "virAsprintf" without checking return value (as is done elsewhere 515 out of 543 times). /libvirt/src/qemu/qemu_process.c:2780: unchecked_value: No check of the return value of "virAsprintf(&msg, "was paused (%s)", virDomainPausedReasonTypeToString(reason))". /libvirt/tests/commandtest.c:809: check_return: Calling function "setsid" without checking return value (as is done elsewhere 4 out of 5 times). /libvirt/tests/commandtest.c:830: unchecked_value: No check of the return value of "virTestGetDebug()". /libvirt/tests/commandtest.c:831: check_return: Calling function "virTestGetVerbose" without checking return value (as is done elsewhere 41 out of 42 times). /libvirt/tests/commandtest.c:833: check_return: Calling function "virInitialize" without checking return value (as is done elsewhere 18 out of 21 times). One note about the error in commandtest line 809: setsid() seems to fail when running the test -- could be removed ? --- src/conf/network_conf.c | 7 ++----- src/qemu/qemu_process.c | 4 ++-- tests/commandtest.c | 9 +++++---- 3 files changed, 9 insertions(+), 11 deletions(-) Index: libvirt-acl/src/conf/network_conf.c =================================================================== --- libvirt-acl.orig/src/conf/network_conf.c +++ libvirt-acl/src/conf/network_conf.c @@ -590,12 +590,9 @@ virNetworkDNSSrvDefParseXML(virNetworkDN } if (strlen(service) > DNS_RECORD_LENGTH_SRV) { - char *name = NULL; - - virAsprintf(&name, _("Service name is too long, limit is %d bytes"), DNS_RECORD_LENGTH_SRV); virNetworkReportError(VIR_ERR_XML_DETAIL, - "%s", name); - VIR_FREE(name); + _("Service name is too long, limit is %d bytes"), + DNS_RECORD_LENGTH_SRV); goto error; } Index: libvirt-acl/src/qemu/qemu_process.c =================================================================== --- libvirt-acl.orig/src/qemu/qemu_process.c +++ libvirt-acl/src/qemu/qemu_process.c @@ -2777,8 +2777,8 @@ qemuProcessUpdateState(struct qemud_driv } else { newState = VIR_DOMAIN_PAUSED; newReason = reason; - virAsprintf(&msg, "was paused (%s)", - virDomainPausedReasonTypeToString(reason)); + ignore_value(virAsprintf(&msg, "was paused (%s)", + virDomainPausedReasonTypeToString(reason))); } } else if (state == VIR_DOMAIN_SHUTOFF && running) { newState = VIR_DOMAIN_RUNNING; Index: libvirt-acl/tests/commandtest.c =================================================================== --- libvirt-acl.orig/tests/commandtest.c +++ libvirt-acl/tests/commandtest.c @@ -806,7 +806,7 @@ mymain(void) return EXIT_FAILURE; setpgid(0, 0); - setsid(); + ignore_value(setsid()); /* Our test expects particular fd values; to get that, we must not * leak fds that we inherited from a lazy parent. At the same @@ -827,10 +827,11 @@ mymain(void) /* Prime the debug/verbose settings from the env vars, * since we're about to reset 'environ' */ - virTestGetDebug(); - virTestGetVerbose(); + ignore_value(virTestGetDebug()); + ignore_value(virTestGetVerbose()); - virInitialize(); + if (virInitialize() < 0) + return EXIT_FAILURE; /* Phase two of killing interfering fds; see above. */ fd = 3;

3 3

[libvirt] [PATCHv2] blockjob: fix block-stream bandwidth race
by Eric Blake 27 Apr '12

27 Apr '12

With RHEL 6.2, virDomainBlockPull(dom, dev, bandwidth, 0) has a race with non-zero bandwidth: there is a window between the block_stream and block_job_set_speed monitor commands where an unlimited amount of data was let through, defeating the point of a throttle. This race was first identified in commit a9d3495e, and libvirt was able to reduce the size of the window for that race. In the meantime, the qemu developers decided to fix things properly; per this message: https://lists.gnu.org/archive/html/qemu-devel/2012-04/msg03793.html the fix will be in qemu 1.1, and changes block-job-set-speed to use a different parameter name, as well as adding a new optional parameter to block-stream, which eliminates the race altogether. Since our documentation already mentioned that we can refuse a non-zero bandwidth for some hypervisors, I think the best solution is to do just that for RHEL 6.2 qemu, so that the race is obvious to the user (anyone using stock RHEL 6.2 binaries won't have this patch, and anyone building their own libvirt with this patch for RHEL can also rebuild qemu to get the modern semantics, so it is no real loss in behavior). Meanwhile the code must be fixed to honor actual qemu 1.1 naming. Rename the parameter to 'modern', since the naming difference now covers more than just 'async' block-job-cancel. And while at it, fix an unchecked integer overflow. * src/qemu/qemu_monitor.h (enum BLOCK_JOB_CMD): Drop unused value, rename enum to match conventions. * src/qemu/qemu_monitor.c (qemuMonitorBlockJob): Reflect enum rename. * src/qemu_qemu_monitor_json.h (qemuMonitorJSONBlockJob): Likewise. * src/qemu/qemu_monitor_json.c (qemuMonitorJSONBlockJob): Likewise, and support difference between RHEL 6.2 and qemu 1.1 block pull. * src/qemu/qemu_driver.c (qemuDomainBlockJobImpl): Reject bandwidth during pull with too-old qemu. * src/libvirt.c (virDomainBlockPull, virDomainBlockRebase): Document this. --- v2: fix integer overflow, improve variable naming src/libvirt.c | 8 ++++- src/qemu/qemu_driver.c | 8 +++-- src/qemu/qemu_monitor.c | 25 +++++++++++++----- src/qemu/qemu_monitor.h | 15 +++++----- src/qemu/qemu_monitor_json.c | 59 +++++++++++++++++++++++------------------ src/qemu/qemu_monitor_json.h | 6 ++-- 6 files changed, 72 insertions(+), 49 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index b01ebba..cfd7711 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -18145,7 +18145,9 @@ error: * The maximum bandwidth (in Mbps) that will be used to do the copy can be * specified with the bandwidth parameter. If set to 0, libvirt will choose a * suitable default. Some hypervisors do not support this feature and will - * return an error if bandwidth is not 0. + * return an error if bandwidth is not 0; in this case, it might still be + * possible for a later call to virDomainBlockJobSetSpeed() to succeed. + * The actual speed can be determined with virDomainGetBlockJobInfo(). * * This is shorthand for virDomainBlockRebase() with a NULL base. * @@ -18263,7 +18265,9 @@ error: * The maximum bandwidth (in Mbps) that will be used to do the copy can be * specified with the bandwidth parameter. If set to 0, libvirt will choose a * suitable default. Some hypervisors do not support this feature and will - * return an error if bandwidth is not 0. + * return an error if bandwidth is not 0; in this case, it might still be + * possible for a later call to virDomainBlockJobSetSpeed() to succeed. + * The actual speed can be determined with virDomainGetBlockJobInfo(). * * When @base is NULL and @flags is 0, this is identical to * virDomainBlockPull(). diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c3555ca..d3aa34d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11654,6 +11654,11 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const char *base, _("partial block pull not supported with this " "QEMU binary")); goto cleanup; + } else if (mode == BLOCK_JOB_PULL && bandwidth) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("setting bandwidth at start of block pull not " + "supported with this QEMU binary")); + goto cleanup; } device = qemuDiskPathToAlias(vm, path, &idx); @@ -11676,9 +11681,6 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const char *base, * relying on qemu to do this. */ ret = qemuMonitorBlockJob(priv->mon, device, base, bandwidth, info, mode, async); - if (ret == 0 && mode == BLOCK_JOB_PULL && bandwidth) - ret = qemuMonitorBlockJob(priv->mon, device, NULL, bandwidth, NULL, - BLOCK_JOB_SPEED_INTERNAL, async); qemuDomainObjExitMonitorWithDriver(driver, vm); if (ret < 0) goto endjob; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 2f66c46..ca1c7aa 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2768,23 +2768,34 @@ int qemuMonitorScreendump(qemuMonitorPtr mon, return ret; } +/* bandwidth is in MB/sec */ int qemuMonitorBlockJob(qemuMonitorPtr mon, const char *device, const char *base, unsigned long bandwidth, virDomainBlockJobInfoPtr info, - int mode, - bool async) + qemuMonitorBlockJobCmd mode, + bool modern) { int ret = -1; + unsigned long long speed; - VIR_DEBUG("mon=%p, device=%s, base=%s, bandwidth=%lu, info=%p, mode=%o, " - "async=%d", mon, device, NULLSTR(base), bandwidth, info, mode, - async); + VIR_DEBUG("mon=%p, device=%s, base=%s, bandwidth=%luM, info=%p, mode=%o, " + "modern=%d", mon, device, NULLSTR(base), bandwidth, info, mode, + modern); + + /* Convert bandwidth MiB to bytes */ + if (bandwidth > ULLONG_MAX / 1024 / 1024) { + qemuReportError(VIR_ERR_OVERFLOW, + _("bandwidth must be less than %llu"), + ULLONG_MAX / 1024 / 1024); + return -1; + } + speed = bandwidth * 1024ULL * 1024ULL; if (mon->json) - ret = qemuMonitorJSONBlockJob(mon, device, base, bandwidth, info, mode, - async); + ret = qemuMonitorJSONBlockJob(mon, device, base, speed, info, mode, + modern); else qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("block jobs require JSON monitor")); diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index f3cdcdd..ffe8fe7 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -527,20 +527,19 @@ int qemuMonitorSendKey(qemuMonitorPtr mon, unsigned int nkeycodes); typedef enum { - BLOCK_JOB_ABORT = 0, - BLOCK_JOB_INFO = 1, - BLOCK_JOB_SPEED = 2, - BLOCK_JOB_SPEED_INTERNAL = 3, - BLOCK_JOB_PULL = 4, -} BLOCK_JOB_CMD; + BLOCK_JOB_ABORT, + BLOCK_JOB_INFO, + BLOCK_JOB_SPEED, + BLOCK_JOB_PULL, +} qemuMonitorBlockJobCmd; int qemuMonitorBlockJob(qemuMonitorPtr mon, const char *device, const char *back, unsigned long bandwidth, virDomainBlockJobInfoPtr info, - int mode, - bool async) + qemuMonitorBlockJobCmd mode, + bool modern) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int qemuMonitorOpenGraphics(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index eb58f13..e1f5453 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3423,29 +3423,36 @@ static int qemuMonitorJSONGetBlockJobInfo(virJSONValuePtr reply, } +/* speed is in bytes/sec */ int qemuMonitorJSONBlockJob(qemuMonitorPtr mon, const char *device, const char *base, - unsigned long bandwidth, + unsigned long long speed, virDomainBlockJobInfoPtr info, - int mode, - bool async) + qemuMonitorBlockJobCmd mode, + bool modern) { int ret = -1; virJSONValuePtr cmd = NULL; virJSONValuePtr reply = NULL; const char *cmd_name = NULL; - if (base && (mode != BLOCK_JOB_PULL || !async)) { + if (base && (mode != BLOCK_JOB_PULL || !modern)) { qemuReportError(VIR_ERR_INTERNAL_ERROR, _("only modern block pull supports base: %s"), base); return -1; } + if (speed && mode == BLOCK_JOB_PULL && !modern) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("only modern block pull supports speed: %llu"), + speed); + return -1; + } - switch ((BLOCK_JOB_CMD) mode) { + switch (mode) { case BLOCK_JOB_ABORT: - cmd_name = async ? "block-job-cancel" : "block_job_cancel"; + cmd_name = modern ? "block-job-cancel" : "block_job_cancel"; cmd = qemuMonitorJSONMakeCommand(cmd_name, "s:device", device, NULL); break; case BLOCK_JOB_INFO: @@ -3453,19 +3460,24 @@ qemuMonitorJSONBlockJob(qemuMonitorPtr mon, cmd = qemuMonitorJSONMakeCommand(cmd_name, NULL); break; case BLOCK_JOB_SPEED: - case BLOCK_JOB_SPEED_INTERNAL: - cmd_name = async ? "block-job-set-speed" : "block_job_set_speed"; - cmd = qemuMonitorJSONMakeCommand(cmd_name, "s:device", - device, "U:value", - bandwidth * 1024ULL * 1024ULL, - NULL); + cmd_name = modern ? "block-job-set-speed" : "block_job_set_speed"; + cmd = qemuMonitorJSONMakeCommand(cmd_name, "s:device", device, + modern ? "U:speed" : "U:value", + speed, NULL); break; case BLOCK_JOB_PULL: - cmd_name = async ? "block-stream" : "block_stream"; - cmd = qemuMonitorJSONMakeCommand(cmd_name, - "s:device", device, - base ? "s:base" : NULL, base, - NULL); + cmd_name = modern ? "block-stream" : "block_stream"; + if (speed) + cmd = qemuMonitorJSONMakeCommand(cmd_name, + "s:device", device, + "U:speed", speed, + base ? "s:base" : NULL, base, + NULL); + else + cmd = qemuMonitorJSONMakeCommand(cmd_name, + "s:device", device, + base ? "s:base" : NULL, base, + NULL); break; } @@ -3477,14 +3489,9 @@ qemuMonitorJSONBlockJob(qemuMonitorPtr mon, if (ret == 0 && virJSONValueObjectHasKey(reply, "error")) { ret = -1; if (qemuMonitorJSONHasError(reply, "DeviceNotActive")) { - /* If a job completes before we get a chance to set the - * speed, we don't want to fail the original command. */ - if (mode == BLOCK_JOB_SPEED_INTERNAL) - ret = 0; - else - qemuReportError(VIR_ERR_OPERATION_INVALID, - _("No active operation on device: %s"), - device); + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("No active operation on device: %s"), + device); } else if (qemuMonitorJSONHasError(reply, "DeviceInUse")){ qemuReportError(VIR_ERR_OPERATION_FAILED, _("Device %s in use"), device); @@ -3497,7 +3504,7 @@ qemuMonitorJSONBlockJob(qemuMonitorPtr mon, _("Command '%s' is not found"), cmd_name); } else { qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unexpected error")); + _("Unexpected error")); } } diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index aacbb5f..22a3adf 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -251,10 +251,10 @@ int qemuMonitorJSONScreendump(qemuMonitorPtr mon, int qemuMonitorJSONBlockJob(qemuMonitorPtr mon, const char *device, const char *base, - unsigned long bandwidth, + unsigned long long speed, virDomainBlockJobInfoPtr info, - int mode, - bool async) + qemuMonitorBlockJobCmd mode, + bool modern) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int qemuMonitorJSONSetLink(qemuMonitorPtr mon, -- 1.7.7.6

2 2