Re: [libvirt] [PATCH] qemu: Don't use -mem-prealloc among with .prealloc=yes
On 11/5/18 9:49 AM, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1624223
>
> There are two ways to request memory preallocation on cmd line:
> -mem-prealloc and .prealloc attribute to memory-backend-file.
s/to/for a/ ?
> However, as it turns out it's not safe to use both at the same
> time. Prefer -mem-prealloc as it is more backward compatible
> compared to switching to "-numa node,memdev= + -object
> memory-backend-file".
>
FWIW: Issue introduced by commit 1c4f3b56..
While I understand the reasoning, it's really too bad we couldn't "move"
the determination over which conflicting qualifier is used to earlier.
By the time we call the -numa backend we would already have had to make
the choice if I'm reading the ordering right.
But if it doesn't matter for the -numa object to use the -mem-prealloc,
then who am I to complain. Of course the "future thinking" me that is
living in the present issues surrounding machine and pc makes me wonder
if choosing this as the default going forward into the future where
someone could deprecate the -mem-prealloc because -numa will be so
prevelant won't bite us down the road.
Curious how others feel - I'm not against this choice, just trying to
supply an opposing/differing viewpoint. We really have to start coding
for the future and consider what deprecation could mean especially for
arguments that essentially mean the same thing.
> Signed-off-by: Michal Privoznik
> ---
> src/qemu/qemu_command.c | 37 +--
> src/qemu/qemu_command.h | 1 +
> src/qemu/qemu_domain.c| 2 +
> src/qemu/qemu_domain.h| 3 ++
> src/qemu/qemu_hotplug.c | 3 +-
> .../hugepages-numa-default-dimm.args | 2 +-
> 6 files changed, 35 insertions(+), 13 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index e338d3172e..0294030f0e 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3123,6 +3123,7 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd,
> * @def: domain definition object
> * @mem: memory definition object
> * @autoNodeset: fallback nodeset in case of automatic NUMA placement
> + * @forbidPrealloc: don't set prealloc attribute
Slight bikeshed, but this changes the priv->memAlloc to @forbidPrealloc
which is IMO a bit odd.
Beyond that, this becomes the 3rd @priv field to be passed along...
Maybe @priv should just be passed to access qemuCaps, autoNodeset, and
memPrealloc.
> * @force: forcibly use one of the backends
> *
> * Creates a configuration object that represents memory backend of given
> guest
> @@ -3136,6 +3137,9 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd,
> * Then, if one of the two memory-backend-* should be used, the @qemuCaps is
> * consulted to check if qemu does support it.
> *
> + * If @forbidPrealloc is true then 'prealloc' attribute of the backend is not
> + * set. This may come handy when global -mem-prealloc is already specified.
> + *
> * Returns: 0 on success,
> * 1 on success and if there's no need to use memory-backend-*
> * -1 on error.
> @@ -3148,6 +3152,7 @@ qemuBuildMemoryBackendProps(virJSONValuePtr
> *backendProps,
> virDomainDefPtr def,
> virDomainMemoryDefPtr mem,
> virBitmapPtr autoNodeset,
> +bool forbidPrealloc,
> bool force)
> {
> const char *backendType = "memory-backend-file";
> @@ -3265,11 +3270,13 @@ qemuBuildMemoryBackendProps(virJSONValuePtr
> *backendProps,
> if (mem->nvdimmPath) {
> if (VIR_STRDUP(memPath, mem->nvdimmPath) < 0)
> goto cleanup;
> -prealloc = true;
> +if (!forbidPrealloc)
> +prealloc = true;
> } else if (useHugepage) {
> if (qemuGetDomainHupageMemPath(def, cfg, pagesize, ) < 0)
> goto cleanup;
> -prealloc = true;
> +if (!forbidPrealloc)
> +prealloc = true;
> } else {
> /* We can have both pagesize and mem source. If that's the case,
> * prefer hugepages as those are more specific. */
> @@ -3398,7 +3405,8 @@ qemuBuildMemoryCellBackendStr(virDomainDefPtr def,
> mem.info.alias = alias;
>
> if ((rc = qemuBuildMemoryBackendProps(, alias, cfg, priv->qemuCaps,
> - def, , priv->autoNodeset,
> false)) < 0)
> + def, , priv->autoNodeset,
> + priv->memPrealloc, false)) < 0)
> goto cleanup;
>
> if (virQEMUBuildObjectCommandlineFromJSON(buf, props) < 0)
> @@ -3435,7 +3443,8 @@ qemuBuildMemoryDimmBackendStr(virBufferPtr buf,
> goto cleanup;
>
> if
[libvirt] [PATCH] libxl: Properly dispose libxl_domain_config object
V2 of the libxl soft reset patch, which was pushed as commit da4b0fd9, dropped the hunk that disposed of the libxl_domain_config object. Add the missing hunk to properly dispose the object. Signed-off-by: Jim Fehlig --- Pushing as trivial... src/libxl/libxl_domain.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 6aca7eebf8..5fe3f44fbe 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -605,6 +605,7 @@ libxlDomainShutdownThread(void *opaque) virObjectEventStateQueue(driver->domainEventState, dom_event); libxl_event_free(cfg->ctx, ev); VIR_FREE(shutdown_info); +libxl_domain_config_dispose(_config); virObjectUnref(cfg); } -- 2.18.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virNetworkDefUpdateDNSHost: Only require IP to match
I've filed [1] to track this in Bugzilla. Cheers, Trevor [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1647211 -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] lxc: Include support to lxc version 3.0 or higher.
On 11/5/18 1:57 PM, Julio Faracco wrote:
> This patch introduce the new settings for LXC 3.0 or higher. The older
> versions keep the compatibility to deprecated settings for LXC, but
> after release 3.0, the compatibility was removed. This commit adds the
> support to the refactored settings.
>
> Signed-off-by: Julio Faracco
> ---
> src/lxc/lxc_native.c | 18 +-
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c
> index cbdec723dd..8604cbaf46 100644
> --- a/src/lxc/lxc_native.c
> +++ b/src/lxc/lxc_native.c
> @@ -200,7 +200,9 @@ lxcSetRootfs(virDomainDefPtr def,
> int type = VIR_DOMAIN_FS_TYPE_MOUNT;
> VIR_AUTOFREE(char *) value = NULL;
>
> -if (virConfGetValueString(properties, "lxc.rootfs", ) <= 0)
> +if (virConfGetValueString(properties, "lxc.rootfs", ) <= 0 &&
> +/* Legacy config keys were removed after release 3.0. */
> +virConfGetValueString(properties, "lxc.rootfs.path", ) <= 0)
> return -1;
So "integrating" Pino's v2 comment and Michal's RFC/v1 comment plus some
error cleansing:
if (virConfGetValueString(properties, "lxc.rootfs.path", ) <= 0) {
virResetLastError();
/* Check for pre LXC 3.0 legacy key */
if (virConfGetValueString(properties, "lxc.rootfs", ) <= 0)
return -1;
}
>
> if (STRPREFIX(value, "/dev/"))
> @@ -684,7 +686,9 @@ lxcCreateConsoles(virDomainDefPtr def, virConfPtr
> properties)
> virDomainChrDefPtr console;
> size_t i;
>
> -if (virConfGetValueString(properties, "lxc.tty", ) <= 0)
> +if (virConfGetValueString(properties, "lxc.tty", ) <= 0 &&
> +/* Legacy config keys were removed after release 3.0. */
> +virConfGetValueString(properties, "lxc.tty.max", ) <= 0)
> return 0;
if (virConfGetValueString(properties, "lxc.tty.max", ) <= 0) {
virResetLastError();
/* Check for pre LXC 3.0 legacy key */
if (virConfGetValueString(properties, "lxc.tty", ) <= 0)
return 0;
}
Although it could be argued that return 0 should also have a reset last error.
>
> if (virStrToLong_i(value, NULL, 10, ) < 0) {
> @@ -724,7 +728,7 @@ lxcIdmapWalkCallback(const char *name, virConfValuePtr
> value, void *data)
> char type;
> unsigned long start, target, count;
>
> -if (STRNEQ(name, "lxc.id_map") || !value->str)
> +if (STRNEQ(name, "lxc.id_map") || STRNEQ(name, "lxc.idmap") ||
> !value->str)
> return 0;
>
/* LXC 3.0 uses "lxc.idmap", while legacy used "lxc.id_map" */
if (STRNEQ(name, "lxc.idmap") || STRNEQ(name, "lxc.id_map") || !value->str)
return 0;
For this one, not only reverse the order, but also fix the error message:
virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid %s: '%s'"),
name, value->str);
where name will be either "lxc.id_map" or "lxc.idmap"
> if (sscanf(value->str, "%c %lu %lu %lu", ,
> @@ -1041,7 +1045,9 @@ lxcParseConfigString(const char *config,
> if (VIR_STRDUP(vmdef->os.init, "/sbin/init") < 0)
> goto error;
>
> -if (virConfGetValueString(properties, "lxc.utsname", ) <= 0 ||
> +if ((virConfGetValueString(properties, "lxc.utsname", ) <= 0 &&
> + /* Legacy config keys were removed after release 3.0. */
> + virConfGetValueString(properties, "lxc.uts.name", ) <= 0) ||
> VIR_STRDUP(vmdef->name, value) < 0)
> goto error;
if (virConfGetValueString(properties, "lxc.uts.name", ) <= 0) {
virResetLastError();
/* Check for pre LXC 3.0 legacy key */
if (virConfGetValueString(properties, "lxc.utsname", ) <= 0)
goto error;
}
if (VIR_STRDUP(vmdef->name, value) < 0)
goto error;
> if (!vmdef->name && (VIR_STRDUP(vmdef->name, "unnamed") < 0))
> @@ -1051,7 +1057,9 @@ lxcParseConfigString(const char *config,
> goto error;
>
> /* Look for fstab: we shouldn't have it */
> -if (virConfGetValue(properties, "lxc.mount")) {
> +if (virConfGetValue(properties, "lxc.mount") ||
> +/* Legacy config keys were removed after release 3.0. */
> +virConfGetValue(properties, "lxc.mount.fstab")) {
> virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> _("lxc.mount found, use lxc.mount.entry lines
> instead"));
> goto error;
>
This one's interesting because what does it mean if someone does use
"lxc.mount.fstab"?
Should they be using lxc.mount.entry instead? I assume this is what you want:
/* LXC 3.0 uses "lxc.mount.fstab", while legacy used just "lxc.mount".
* In either case, generate the error to use "lxc.mount.entry" instead */
if (virConfGetValue(properties, "lxc.mount.fstab") ||
virConfGetValue(properties, "lxc.mount")) {
virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
_("lxc.mount.fstab or
[libvirt] [PATCH] virNetworkDefUpdateDNSHost: Only require IP to match
On Tue, Nov 06, 2018 at 09:10:39AM -0800, W. Trevor King wrote: > This commit update the matching logic to only consider the IP > address. Oops, I should have updated the subject to: virNetworkDefUpdateDNSHost: Only require IP to match Cheers, Trevor -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 2/2] storage: Improve error handling on cdrom backend
On 10/25/18 11:39 PM, Han Han wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1596096
>
> Implement virFileCheckCDROM in virStorageBackendVolOpen to check if
> cdrom backend is ok. Report more detailed error if not ok.
>
> Signed-off-by: Han Han
> ---
> src/storage/storage_util.c | 32
> 1 file changed, 32 insertions(+)
>
> diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
> index 318a556656..c9f89b817a 100644
> --- a/src/storage/storage_util.c
> +++ b/src/storage/storage_util.c
> @@ -1601,6 +1601,38 @@ virStorageBackendVolOpen(const char *path, struct stat
> *sb,
> return -1;
> }
>
> +#if defined(__linux__)
This not what I was expecting... Let's not have conditionals in this
module and go with my suggestion from patch1 of changing the non linux
version of virFileCheckCDROM to return VIR_FILE_CDROM_DISC_OK when we
return 1 and to return/handle VIR_FILE_CDROM_NO_DEVICE here when 0 is
returned (even though this code doesn't handle that "yet").
> +virFileCDRomStatus cd_status = VIR_FILE_CDROM_UNKNOWN;
Definition belongs at the top-level...
> +
> +if (virFileCheckCDROM(path, _status) > 0) {
> +switch (cd_status) {
> +case VIR_FILE_CDROM_UNKNOWN:
> +virReportError(VIR_ERR_NO_STORAGE_VOL,
> + _("unknown status for CDROM storage vol
> '%s'"),
> + path);
> +return -1;
> +case VIR_FILE_CDROM_NO_DISC:
> +virReportError(VIR_ERR_NO_STORAGE_VOL,
> + _("no disc in CDROM storage vol '%s'"),
> + path);
> +return -1;
> +case VIR_FILE_CDROM_TRAY_OPEN:
> +virReportError(VIR_ERR_NO_STORAGE_VOL,
> + _("the tray of CDROM storage vol '%s' is
> open"),
> + path);
> +return -1;
> +case VIR_FILE_CDROM_DRIVE_NOT_READY:
> +virReportError(VIR_ERR_NO_STORAGE_VOL,
> + _("CDROM storage vol '%s' is not ready"),
> + path);
> +return -1;
> +case VIR_FILE_CDROM_DISC_OK:
> +VIR_INFO("CDROM storage vol %s is OK", path);
Depending on the level of debugging enabled and the times we go through
here, this could be chatty...
Also, kind of strange to be making a check for a specific issue from the
bz description in a generic place. Check all the callers of
virStorageBackendVolOpen... I think you may need to move this switch.
For callers that may be correctly supplying a CDROM @path we could
return a failure now.
The switch may be nice as a common/shared method too so that no one is
tempted to cut-n-paste whenever the values are needed.
John
> +break;
> +}
> +}
> +#endif /* defined(__linux__) */
> +
> /* O_NONBLOCK should only matter during open() for fifos and
> * sockets, which we already filtered; but using it prevents a
> * TOCTTOU race. However, later on we will want to read() the
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 1/2] util: Refactor virFileIsCDROM to virFileCheckCDROM
On 10/25/18 11:39 PM, Han Han wrote:
> Refactor virFileIsCDROM to virFileCheckCDROM for checking cdrom status.
> Add add enum type virFileCDRomStatus.
>
> Now virFileCheckCDROM could be used to check the cdrom drive status such
> as ok, no disc, tray open, drive not ready, or unknown.
>
> Signed-off-by: Han Han
> ---
> src/libvirt_private.syms | 2 +-
> src/qemu/qemu_domain.c | 4 ++--
> src/util/virfile.c | 36 +++-
> src/util/virfile.h | 12 +++-
> 4 files changed, 45 insertions(+), 9 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 335210c31d..c61b613325 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1802,6 +1802,7 @@ virFileActivateDirOverride;
> virFileBindMountDevice;
> virFileBuildPath;
> virFileCanonicalizePath;
> +virFileCheckCDROM;
> virFileChownFiles;
> virFileClose;
> virFileComparePaths;
> @@ -1824,7 +1825,6 @@ virFileGetMountSubtree;
> virFileHasSuffix;
> virFileInData;
> virFileIsAbsPath;
> -virFileIsCDROM;
> virFileIsDir;
> virFileIsExecutable;
> virFileIsLink;
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index ba3fff607a..f34243d2b2 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -7806,7 +7806,7 @@ void qemuDomainObjCheckDiskTaint(virQEMUDriverPtr
> driver,
>
> if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM &&
> virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_BLOCK &&
> -disk->src->path && virFileIsCDROM(disk->src->path) == 1)
> +disk->src->path && virFileCheckCDROM(disk->src->path, NULL) == 1)
> qemuDomainObjTaint(driver, obj, VIR_DOMAIN_TAINT_CDROM_PASSTHROUGH,
> logCtxt);
>
> @@ -8760,7 +8760,7 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
> if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM &&
> src->format == VIR_STORAGE_FILE_RAW &&
> virStorageSourceIsBlockLocal(src) &&
> -virFileIsCDROM(src->path) == 1)
> +virFileCheckCDROM(src->path, NULL) == 1)
> src->hostcdrom = true;
>
> ret = 0;
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index f6f9e4ceda..04b4c274dd 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -1957,18 +1957,22 @@ int virFileIsMountPoint(const char *file)
>
> #if defined(__linux__)
> /**
> - * virFileIsCDROM:
> + * virFileCheckCDROM:
> * @path: File to check
> + * @cd_status: Filled with the status of the CDROM if non-NULL. See
> + * virFileCDRomStatus.
> *
> * Returns 1 if @path is a cdrom device 0 if it is not a cdrom and -1 on
> * error. 'errno' of the failure is preserved and no libvirt errors are
> * reported.
Again @errno is not preserved, I'll replace that last sentence with:
If is up to the caller to use @cd_status in order to generate any error
to be reported (if desired).
> */
> int
> -virFileIsCDROM(const char *path)
> +virFileCheckCDROM(const char *path,
> + virFileCDRomStatus *cd_status)
> {
> struct stat st;
> VIR_AUTOCLOSE fd = -1;
> +int status;
>
> if ((fd = open(path, O_RDONLY | O_NONBLOCK)) < 0)
> return -1;
> @@ -1980,16 +1984,38 @@ virFileIsCDROM(const char *path)
> return 0;
>
> /* Attempt to detect via a CDROM specific ioctl */
> -if (ioctl(fd, CDROM_DRIVE_STATUS, CDSL_CURRENT) >= 0)
> +if ((status = ioctl(fd, CDROM_DRIVE_STATUS, CDSL_CURRENT)) < 0)
> +return 0;
> +
> +if (!cd_status)
> return 1;
>
> -return 0;
> +switch (status) {
> +case CDS_NO_INFO:
> +*cd_status = VIR_FILE_CDROM_UNKNOWN;
> +break;
I think perhaps this should be last *and* should included "default:"
(just in case).
> +case CDS_NO_DISC:
> +*cd_status = VIR_FILE_CDROM_NO_DISC;
> +break;
> +case CDS_TRAY_OPEN:
> +*cd_status = VIR_FILE_CDROM_TRAY_OPEN;
> +break;
> +case CDS_DRIVE_NOT_READY:
> +*cd_status = VIR_FILE_CDROM_DRIVE_NOT_READY;
> +break;
> +case CDS_DISC_OK:
> +*cd_status = VIR_FILE_CDROM_DISC_OK;
> +break;
> +}
> +
> +return 1;
> }
>
> #else
>
> int
> -virFileIsCDROM(const char *path)
> +virFileCheckCDROM(const char *path,
> + virFileCDRomStatus *cd_status ATTRIBUTE_UNUSED)
> {
Like I noted previously you could have :
if (cd_status)
*cd_status = VIR_FILE_CDROM_DISK_OK;
> if (STRPREFIX(path, "/dev/cd") ||
> STRPREFIX(path, "/dev/acd"))
Then before the return 0 added a:
if (cd_status)
*cd_status = VIR_FILE_CDROM_NO_DEVICE;
something new to add to your list below which could be handled by an
error message...
> diff --git a/src/util/virfile.h b/src/util/virfile.h
> index 0f7dece958..1e5127badb 100644
> ---
Re: [libvirt] [PATCH go] version: Add ParseVersion and a Version struct
Anything I can do to help this along? I've adjusted the subject to have "PATCH go" instead of just "PATCH"; sorry about that. Cheers, Trevor -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] virNetworkDefUpdateDNSHost: Require both IP and a hostname to match
Since fc19a0059 (network: backend functions for updating network dns
host/srv/txt, 2012-11-12), the matching logic for various network
components has been:
1) for HOST records, it's considered a match if the IP address or any
of the hostnames of an existing record matches.
2) for SRV records, it's a match if all of
domain+service+protocol+target *which have been specified* are
matched.
3) for TXT records, there is only a single field to match - name
(value can be the same for multiple records, and isn't considered a
search term), so by definition there can be no ambiguous matches.
But HOST records can have the same hostname for multiple records
(similar to TXT records with the same value). The value that needs to
be distinct for HOST records is the IP address. This commit updates
the matching logic to only consider the IP address. Compared to the
previous HOST logic:
1. You can now delete entries from an existing network like:
example
example
with input like:
or:
example
Previously, only the former would work (the latter used to raise
"multiple matching DNS HOST records were found in network").
2. You can no longer remove entries by hostname alone. Previously,
you may have been able to remove an entry from an existing network
like:
example-1
example-2
with input like:
example-1
using the 'name' property to get through the partialOkay check in
virNetworkDHCPHostDefParseXML. Now that input will raise "Missing
IP address in network '%s' DNS HOST record".
3. You can now add multiple entries with a common hostname (as long as
they have distinct IP addresses). Previously, adding:
example
to an existing:
example
would have raised "there is already at least one DNS HOST record
with a matching field in network".
---
I'm actually not clear on whether the 'ip' attribute is required to be
unique or not. If not, maybe the logic should be:
* Deletes with just an IP remove all entries that match that
IP.
* Deletes with just a hostname remove all entries that
match that hostname.
* Deletes with an IP and a hostname remove matching entries
from entries which match the IP.
* If removal completely empties a , the is
also removed.
Thoughts?
src/conf/network_conf.c | 31 ++-
1 file changed, 10 insertions(+), 21 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 39a13b4..8ed62ac 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -587,14 +587,14 @@ virNetworkDNSHostDefParseXML(const char *networkName,
xmlNodePtr cur;
char *ip;
-if (!(ip = virXMLPropString(node, "ip")) && !partialOkay) {
+if (!(ip = virXMLPropString(node, "ip"))) {
virReportError(VIR_ERR_XML_DETAIL,
_("Missing IP address in network '%s' DNS HOST record"),
networkName);
goto error;
}
-if (ip && (virSocketAddrParse(>ip, ip, AF_UNSPEC) < 0)) {
+if (virSocketAddrParse(>ip, ip, AF_UNSPEC) < 0) {
virReportError(VIR_ERR_XML_DETAIL,
_("Invalid IP address in network '%s' DNS HOST record"),
networkName);
@@ -603,6 +603,13 @@ virNetworkDNSHostDefParseXML(const char *networkName,
}
VIR_FREE(ip);
+if (!VIR_SOCKET_ADDR_VALID(>ip)) {
+virReportError(VIR_ERR_XML_DETAIL,
+ _("Invalid IP address in network '%s' DNS HOST record"),
+ networkName);
+goto error;
+}
+
cur = node->children;
while (cur != NULL) {
if (cur->type == XML_ELEMENT_NODE &&
@@ -631,13 +638,6 @@ virNetworkDNSHostDefParseXML(const char *networkName,
goto error;
}
-if (!VIR_SOCKET_ADDR_VALID(>ip) && def->nnames == 0) {
-virReportError(VIR_ERR_XML_DETAIL,
- _("Missing ip and hostname in network '%s' DNS HOST
record"),
- networkName);
-goto error;
-}
-
return 0;
error:
@@ -3334,18 +3334,7 @@ virNetworkDefUpdateDNSHost(virNetworkDefPtr def,
goto cleanup;
for (i = 0; i < dns->nhosts; i++) {
-bool foundThisTime = false;
-
-if (virSocketAddrEqual(, >hosts[i].ip))
-foundThisTime = true;
-
-for (j = 0; j < host.nnames && !foundThisTime; j++) {
-for (k = 0; k < dns->hosts[i].nnames && !foundThisTime; k++) {
-if (STREQ(host.names[j], dns->hosts[i].names[k]))
-foundThisTime = true;
-}
-}
-if (foundThisTime) {
+if virSocketAddrEqual(, >hosts[i].ip) {
foundCt++;
foundIdx = i;
}
--
1.8.3.1
--
libvir-list mailing list
Re: [libvirt] [PATCHv7 10/18] conf: Remove virDomainResctrlAppend and introduce virDomainResctrlNew
On 11/6/18 4:51 AM, Huaqiang,Wang wrote:
>
>
> On 2018年11月06日 01:26, John Ferlan wrote:
>>
>> On 10/22/18 4:01 AM, Wang Huaqiang wrote:
>>> Introduced virDomainResctrlNew to do the most part of
>>> virDomainResctrlAppend
>>> and move the operation of appending resctrl to @def->resctrls out of
>>> function.
>>>
>>> Rather than rely on virDomainResctrlAppend to perform the allocation,
>>> move
>>> the onus to the caller and make use of virBitmapNewCopy for @vcpus and
>>> virObjectRef for @alloc, thus removing the need to set each to NULL
>>> after the
>>> call.
>>>
>>> Signed-off-by: Wang Huaqiang
>>> ---
>>> src/conf/domain_conf.c | 60
>>> +-
>>> 1 file changed, 35 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index e8e0adc..39bd396 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -18920,26 +18920,22 @@
>>> virDomainCachetuneDefParseCache(xmlXPathContextPtr ctxt,
>>> }
>>> -static int
>>> -virDomainResctrlAppend(virDomainDefPtr def,
>>> - xmlNodePtr node,
>>> - virResctrlAllocPtr alloc,
>>> - virBitmapPtr vcpus,
>>> - unsigned int flags)
>>> +static virDomainResctrlDefPtr
>>> +virDomainResctrlNew(xmlNodePtr node,
>>> + virResctrlAllocPtr *alloc,
>>> + virBitmapPtr *vcpus,
>> Because we're not "stealing" @*alloc and/or @*vcpus, they do not need to
>> be passed by reference. I can change these. There's some minor merge
>> impact in later patches too, but no big deal.
>
> Agree. Please help make change.
>
>
>>
>>> + unsigned int flags)
>>> {
>>> char *vcpus_str = NULL;
>>> char *alloc_id = NULL;
>>> - virDomainResctrlDefPtr tmp_resctrl = NULL;
>>> - int ret = -1;
>>> -
>>> - if (VIR_ALLOC(tmp_resctrl) < 0)
>>> - goto cleanup;
>>> + virDomainResctrlDefPtr resctrl = NULL;
>>> + virDomainResctrlDefPtr ret = NULL;
>>> /* We need to format it back because we need to be consistent
>>> in the naming
>>> * even when users specify some "sub-optimal" string there. */
>>> - vcpus_str = virBitmapFormat(vcpus);
>>> + vcpus_str = virBitmapFormat(*vcpus);
>>> if (!vcpus_str)
>>> - goto cleanup;
>>> + return NULL;
>>> if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE))
>>> alloc_id = virXMLPropString(node, "id");
>>> @@ -18954,18 +18950,23 @@ virDomainResctrlAppend(virDomainDefPtr def,
>>> goto cleanup;
>>> }
>>>
>> /* NB: Callers assume new @alloc, need to fill in ID now */
>>
>> Not that it would prevent someone in the future from passing an @alloc
>> w/ ->id already filled in and overwriting, but at least for now it's not
>> the case.
>
> Yes, it might happen.
> If @alloc->id is specified through XML and is not following the naming
> convention
> virAsprintf(_id, "vcpus_%s", vcpus_str)
>
> If you think it is necessary we might need to through a warning for this
> case.
>
Let's see - virDomainResctrlNew has two callers:
1. virDomainCachetuneDefParse
In this case, we "know" we have a new/empty @alloc because if
virDomainResctrlVcpuMatch found one, then there'd be a failure.
The virDomainCachetuneDefParseCache calls don't seem to fill in
alloc->id, but virDomainResctrlNew will for the first time.
2. virDomainMemorytuneDefParse
The virDomainResctrlVcpuMatch may find a preexisting @alloc, but
@new_alloc is set to true. The virDomainMemorytuneDefParseMemory won't
fill alloc->id. Then only if @new_alloc do we call virDomainResctrlNew
So I think both are safe "for now". If you want I could modify the
virResctrlAllocSetID code to :
if (alloc->id) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Attempt to overwrite alloc->id='%s' with
id='%s'"),
alloc->id, id);
return -1;
}
Let me know.
John
>>
>> With the changes (that I can make),
>>
>> Reviewed-by: John Ferlan
>>
>> John
>
> Thanks for review.
> Huaqiang
>
>>
>> [...]
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 05/18] util: Refactor code for adding PID to the resource group
On 11/6/18 3:41 AM, Huaqiang,Wang wrote: > > > On 2018年11月05日 23:03, John Ferlan wrote: >> >> On 10/22/18 4:01 AM, Wang Huaqiang wrote: >>> The code of adding PID to the allocation could be reused, refactor it >>> for later reuse. >>> >>> Signed-off-by: Wang Huaqiang >>> --- >>> src/util/virresctrl.c | 30 +++--- >>> 1 file changed, 19 insertions(+), 11 deletions(-) >>> >> Reviewed-by: John Ferlan >> >> John > > Thanks for the review. > Huaqiang > While working through patch1 adjustments, I noted an extra space in a comment, so I fixed it: /* If the allocation is empty, then it is impossible to add a PID to * allocation due to lacking of its 'tasks' file. Just return */ ^^ Of course that resulted in a merge conflict in this patch where I (now) note you changed the comment to: /* If allocation is empty, then no resctrl directory and the 'tasks' file * exists, just return */ I'm going to restore the original comment; however, it made me stop and think about future patch14 which used the *tasks (and a new local *pid list) - perhaps you need to rethink the changes... Even patch1... What's the use of altering the *tasks file at all then? Fortunately it seems it's not really used for much more than logging the tids that are running the vcpu. I'll hold off on pushing anything... So far there's no real impact, but you may decide that you need to 'design in' a way to handle this default/system path/issue. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 01/18] docs, util: Refactor schemas and virresctrl to support optional cache
On 11/6/18 3:30 AM, Huaqiang,Wang wrote:
>
>
> On 2018年11月06日 16:18, Huaqiang,Wang wrote:
>>
>>
>> On 2018年11月05日 23:01, John Ferlan wrote:
>>>
>>> On 10/22/18 4:01 AM, Wang Huaqiang wrote:
Refactor schemas and virresctrl to support optional element
in .
Later, the monitor entry will be introduced and to be placed
under . Either cache entry or monitor entry is
an optional element of .
An cachetune has no element is taking the default resource
allocating policy defined in '/sys/fs/resctrl/schemata'.
Signed-off-by: Wang Huaqiang
---
docs/formatdomain.html.in | 4 ++--
docs/schemas/domaincommon.rng | 4 ++--
src/util/virresctrl.c | 28
3 files changed, 32 insertions(+), 4 deletions(-)
>>> [...]
>>>
+ /* If the allocation is empty, then the path will be
SYSFS_RESCTRL_PATH */
+ if (virResctrlAllocIsEmpty(alloc)) {
+ if (!alloc->path &&
+ VIR_STRDUP(alloc->path, SYSFS_RESCTRL_PATH) < 0)
+ return -1;
+
+ return 0;
+ }
+
>>> Because of ...
>>>
if (!alloc->path &&
virAsprintf(>path, "%s/%s-%s",
SYSFS_RESCTRL_PATH, machinename, alloc->id) < 0)
>>> [...]
>>>
@@ -2334,6 +2358,10 @@ virResctrlAllocRemove(virResctrlAllocPtr alloc)
{
int ret = 0;
+ /* No directory have ever been created. Just return */
+ if (virResctrlAllocIsEmpty(alloc))
+ return 0;
>>> ... the change to virResctrlAllocDeterminePath to fill in alloc->path
>>> when virResctrlAllocIsEmpty to be a default path, this should be:
>>>
>>> if (STREQ_NULLABLE(alloc->path, SYSFS_RESCTRL_PATH))
>>> return 0;
>>>
>>> or moved after the next check and the _NULLABLE removed.
>>>
>>> Whether the AllocIsEmpty is true or not shouldn't be the bearing on
>>> whether the directory created because of that
>>
>> Agree with the changes.
>> "No need to create a directory that has already been created by system."
>> (SYSFS_RESCTRL_PATH is the created directory).
>
> Should be "no need to destroy a system created directory, the
> SYSFS_RESCTRL_PATH.
> Directory SYSFS_RESCTRL_PATH is governed by system."
>
I'll add the comment:
+/* Do not destroy if path is the system/default path for the
allocation */
> It might also be reasonable to make similar changes for
> virResctrlAllocCreate,
> because no need to create an already created directory, Right? And
> *alloc->path must
> be properly assigned, by virResctrlAllocDeterminePath, before a call of
> virResctrlAllocCreate.
>
True - I'll adjust the check there too to be:
+/* If using the system/default path for the allocation, then we're
done */
+if (STREQ(alloc->path, SYSFS_RESCTRL_PATH))
+return 0;
+
Tks -
John
>>
+
if (!alloc->path)
return 0;
>>> I can adjust for you, let me know; otherwise, things are fine for the
>>>
>>> Reviewed-by: John Ferlan
>>>
>>> John
>>
>> Thanks for the review.
>> Huaqiang
>>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/3] qemu: Move allow reboot check setting
On 11/6/18 7:48 AM, Michal Privoznik wrote: > On 11/06/2018 01:28 PM, John Ferlan wrote: >> >> >> On 11/6/18 4:38 AM, Michal Privoznik wrote: >>> On 11/01/2018 05:04 PM, John Ferlan wrote: Checking and setting the priv->allowReboot can be done before we start processing the job. A subsequent patch will make use of the value to make decisions in the error label, so we need to have it set properly. Signed-off-by: John Ferlan --- src/qemu/qemu_process.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 9cf971808c..5232f761af 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7767,6 +7767,10 @@ qemuProcessReconnect(void *opaque) cfg = virQEMUDriverGetConfig(driver); priv = obj->privateData; +/* If we are connecting to a guest started by old libvirt there is no + * allowReboot in status XML and we need to initialize it. */ +qemuProcessPrepareAllowReboot(obj); >>> >>> I'm not quite sure why this happens outside of job. It doesn't look like >>> it has to. >>> >> >> Is there a reason in your opinion it needs to occur inside a job? It is >> a void function. > > The type of the return value doesn't matter. > qemuProcessPrepareAllowReboot() changes private data and that is > potentially dangerous if done outside modify job (even though the @vm is > locked at this point so I guess it is not that dangerous after all). > Does that mean we need to cull the code looking for everywhere in the code where @priv data is modified outside a job? and start a job if so? I thought jobs were more related to monitor interactions rather than @priv modification. The answer still doesn't make sense to me. John >> >> It's moved to prior to the first "goto error" because of patch3 which >> would call qemuDomainIsUsingNoShutdown which checks priv->allowReboot >> which is possibly set in *AllowReboot. Without that move, the code would >> need to be reworked, which is fine, but understanding the reason why I >> wrote things the way I did is important, IMO. I can add a comment to >> "warn" the next person trying to move it that the error: logic uses the >> ->allowReboot value. >> >> The allowReboot alteration has nothing to do with a job AFAICT and >> whether on error: there is a job or not. Perhaps no different to what >> qemuDomainObjRestoreJob is doing by using @priv fields for @oldjob. > > Yeah, but RestoreJob is special - we can't call it after job is > acquired, we want to save currently running job to a temp variable so > that we can start a new job. > >> >> Although looking at and quickly thinking about the code now, I wonder if >> the virQEMUDriverGetCapabilities and goto error should be inside a job >> too since error would then call qemuProcessStop without being in a job. > > Ooops, yes. > >> >> If this is dropped then logic in patch3 needs to be altered in order to >> account for jobStarted = true... I think that got too messy when I first >> thought about it. > > Michal > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/3] qemu: Move allow reboot check setting
On 11/06/2018 01:28 PM, John Ferlan wrote: > > > On 11/6/18 4:38 AM, Michal Privoznik wrote: >> On 11/01/2018 05:04 PM, John Ferlan wrote: >>> Checking and setting the priv->allowReboot can be done before we start >>> processing the job. A subsequent patch will make use of the value to >>> make decisions in the error label, so we need to have it set properly. >>> >>> Signed-off-by: John Ferlan >>> --- >>> src/qemu/qemu_process.c | 8 >>> 1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c >>> index 9cf971808c..5232f761af 100644 >>> --- a/src/qemu/qemu_process.c >>> +++ b/src/qemu/qemu_process.c >>> @@ -7767,6 +7767,10 @@ qemuProcessReconnect(void *opaque) >>> cfg = virQEMUDriverGetConfig(driver); >>> priv = obj->privateData; >>> >>> +/* If we are connecting to a guest started by old libvirt there is no >>> + * allowReboot in status XML and we need to initialize it. */ >>> +qemuProcessPrepareAllowReboot(obj); >> >> I'm not quite sure why this happens outside of job. It doesn't look like >> it has to. >> > > Is there a reason in your opinion it needs to occur inside a job? It is > a void function. The type of the return value doesn't matter. qemuProcessPrepareAllowReboot() changes private data and that is potentially dangerous if done outside modify job (even though the @vm is locked at this point so I guess it is not that dangerous after all). > > It's moved to prior to the first "goto error" because of patch3 which > would call qemuDomainIsUsingNoShutdown which checks priv->allowReboot > which is possibly set in *AllowReboot. Without that move, the code would > need to be reworked, which is fine, but understanding the reason why I > wrote things the way I did is important, IMO. I can add a comment to > "warn" the next person trying to move it that the error: logic uses the > ->allowReboot value. > > The allowReboot alteration has nothing to do with a job AFAICT and > whether on error: there is a job or not. Perhaps no different to what > qemuDomainObjRestoreJob is doing by using @priv fields for @oldjob. Yeah, but RestoreJob is special - we can't call it after job is acquired, we want to save currently running job to a temp variable so that we can start a new job. > > Although looking at and quickly thinking about the code now, I wonder if > the virQEMUDriverGetCapabilities and goto error should be inside a job > too since error would then call qemuProcessStop without being in a job. Ooops, yes. > > If this is dropped then logic in patch3 needs to be altered in order to > account for jobStarted = true... I think that got too messy when I first > thought about it. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/3] qemu: Move allow reboot check setting
On 11/6/18 4:38 AM, Michal Privoznik wrote: > On 11/01/2018 05:04 PM, John Ferlan wrote: >> Checking and setting the priv->allowReboot can be done before we start >> processing the job. A subsequent patch will make use of the value to >> make decisions in the error label, so we need to have it set properly. >> >> Signed-off-by: John Ferlan >> --- >> src/qemu/qemu_process.c | 8 >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c >> index 9cf971808c..5232f761af 100644 >> --- a/src/qemu/qemu_process.c >> +++ b/src/qemu/qemu_process.c >> @@ -7767,6 +7767,10 @@ qemuProcessReconnect(void *opaque) >> cfg = virQEMUDriverGetConfig(driver); >> priv = obj->privateData; >> >> +/* If we are connecting to a guest started by old libvirt there is no >> + * allowReboot in status XML and we need to initialize it. */ >> +qemuProcessPrepareAllowReboot(obj); > > I'm not quite sure why this happens outside of job. It doesn't look like > it has to. > Is there a reason in your opinion it needs to occur inside a job? It is a void function. It's moved to prior to the first "goto error" because of patch3 which would call qemuDomainIsUsingNoShutdown which checks priv->allowReboot which is possibly set in *AllowReboot. Without that move, the code would need to be reworked, which is fine, but understanding the reason why I wrote things the way I did is important, IMO. I can add a comment to "warn" the next person trying to move it that the error: logic uses the ->allowReboot value. The allowReboot alteration has nothing to do with a job AFAICT and whether on error: there is a job or not. Perhaps no different to what qemuDomainObjRestoreJob is doing by using @priv fields for @oldjob. Although looking at and quickly thinking about the code now, I wonder if the virQEMUDriverGetCapabilities and goto error should be inside a job too since error would then call qemuProcessStop without being in a job. If this is dropped then logic in patch3 needs to be altered in order to account for jobStarted = true... I think that got too messy when I first thought about it. Tks - John >> + >> if (!(caps = virQEMUDriverGetCapabilities(driver, false))) >> goto error; >> >> @@ -7783,10 +7787,6 @@ qemuProcessReconnect(void *opaque) >> if (qemuDomainMasterKeyReadFile(priv) < 0) >> goto error; >> >> -/* If we are connecting to a guest started by old libvirt there is no >> - * allowReboot in status XML and we need to initialize it. */ >> -qemuProcessPrepareAllowReboot(obj); >> - >> if (qemuHostdevUpdateActiveDomainDevices(driver, obj->def) < 0) >> goto error; >> >> > > Michal > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V2 0/3] libxl: add support for soft reset
On 10/31/2018 08:51 PM, Jim Fehlig wrote: > Patches 1 and 2 are new to V2 and make slight improvements to > libxlDomainShutdownThread. Patch 3 is adjusted to work with Xen 4.6. > > Jim Fehlig (3): > libxl: remove redundant calls to virObjectEventStateQueue > libxl: Remove some goto labels in libxlDomainShutdownThread > libxl: add support for soft reset > > src/libxl/libxl_domain.c | 99 > 1 file changed, 69 insertions(+), 30 deletions(-) > ACK Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V2 3/3] libxl: add support for soft reset
On 10/31/2018 08:51 PM, Jim Fehlig wrote:
> The pvops Linux kernel implements machine_ops.crash_shutdown as
>
> static void xen_hvm_crash_shutdown(struct pt_regs *regs)
> {
> native_machine_crash_shutdown(regs);
> xen_reboot(SHUTDOWN_soft_reset);
> }
>
> but currently the libxl driver does not handle the soft reset
> shutdown event. As a result, the guest domain never proceeds
> past xen_reboot(), making it impossible for HVM domains to save
> a crash dump using kexec.
>
> This patch adds support for handling the soft reset event by
> calling libxl_domain_soft_reset() and re-enabling domain death
> events, which is similar to the xl tool handling of soft reset
> shutdown event.
>
> Signed-off-by: Jim Fehlig
> ---
>
> V2: Check for availability of soft reset with LIBXL_HAVE_SOFT_RESET
>
> src/libxl/libxl_domain.c | 29 +
> 1 file changed, 29 insertions(+)
>
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index 4cdaee0e51..47c1f49538 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -471,8 +471,10 @@ libxlDomainShutdownThread(void *opaque)
> virObjectEventPtr dom_event = NULL;
> libxl_shutdown_reason xl_reason = ev->u.domain_shutdown.shutdown_reason;
> libxlDriverConfigPtr cfg;
> +libxl_domain_config d_config;
>
> cfg = libxlDriverConfigGet(driver);
> +libxl_domain_config_init(_config);
>
> vm = virDomainObjListFindByID(driver->domains, ev->domid);
> if (!vm) {
> @@ -563,6 +565,33 @@ libxlDomainShutdownThread(void *opaque)
> * Similar to the xl implementation, ignore SUSPEND. Any actions
> needed
> * after calling libxl_domain_suspend() are handled by it's callers.
> */
> +#ifdef LIBXL_HAVE_SOFT_RESET
> +} else if (xl_reason == LIBXL_SHUTDOWN_REASON_SOFT_RESET) {
> +libxlDomainObjPrivatePtr priv = vm->privateData;
I'd put an empty line here.
> +if (libxl_retrieve_domain_configuration(cfg->ctx, vm->def->id,
> +_config) != 0) {
> +VIR_ERROR(_("Failed to retrieve config for VM '%s'. "
> +"Unable to perform soft reset. Destroying VM"),
> + vm->def->name);
> +libxlDomainShutdownHandleDestroy(driver, vm);
> +goto endjob;
> +}
> +
> +if (priv->deathW) {
> +libxl_evdisable_domain_death(cfg->ctx, priv->deathW);
> +priv->deathW = NULL;
> +}
> +
> +if (libxl_domain_soft_reset(cfg->ctx, _config, vm->def->id,
> +NULL, NULL) != 0) {
> +VIR_ERROR(_("Failed to soft reset VM '%s'. Destroying VM"),
> + vm->def->name);
> +libxlDomainShutdownHandleDestroy(driver, vm);
> +goto endjob;
> +}
> +libxl_evenable_domain_death(cfg->ctx, vm->def->id, 0, >deathW);
> +libxl_domain_unpause(cfg->ctx, vm->def->id);
> +#endif
> } else {
> VIR_INFO("Unhandled shutdown_reason %d", xl_reason);
> }
>
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 11/18] conf: move virResctrlAllocIsEmpty to a place a litter lower
On 2018年11月06日 01:26, John Ferlan wrote: $SUBJ: s/litter/little Perhaps better said: conf: Alter order of Cachetune virDomainResctrlNew call On 10/22/18 4:01 AM, Wang Huaqiang wrote: This refactor allows to add some code between virDomainResctrlNew and virResctrlAllocIsEmpty to extend the scope of resctrl. this then becomes: Refactor the logic to handle subsequent generation of a resource monitor which would effect whether a non default cache exists. Thanks for wording. Signed-off-by: Wang Huaqiang --- src/conf/domain_conf.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) Reviewed-by: John Ferlan John Thanks for your review. Huaqiang -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] RFC: VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA is not functional?
On Mon, Nov 5, 2018 at 9:38 PM Peter Krempa wrote: > On Fri, Nov 02, 2018 at 15:52:54 +0800, Han Han wrote: > > Hello, > > I found snapshot APIs(like virDomainSnapshotNum) invoked with > > VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA will return 0 even there are > internal > > no-metadata snapshots in the domain. > > We've never implemented the support for loading internal snapshots > without libvirt metadata, thus none will be listed. > > > > > Then I find the reason is in virDomainSnapshotObjListGetNames(): > >937 /* If this common code is being used, we assume that all > > snapshots > >938 ┆* have metadata, and thus can handle METADATA up front as > > an > >939 ┆* all-or-none filter. XXX This might not always be true, if > > we > >940 ┆* add the ability to track qcow2 internal snapshots without > > the > >941 ┆* use of metadata. > > */ > >942 if ((data.flags & VIR_DOMAIN_SNAPSHOT_FILTERS_METADATA) > > == > >943 ┆ > > VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA) > >944 ┆ return 0; > > > > So currently, with VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA, no snapshot will > > be returned. > > > > I checked the commit. It is really old(6478ec1673aEric Blake > > 2012-08-13 18:09:12 -0600) . I guess it was initially designed for the > > function to list internal snapshots without metadata. > > > > However, now internal snapshot seems lower prioritized than external > > snapshot. > > Do we have plan to design this function? Since the APIs invoked with this > > flag don't return the **right** result, if the function for > > VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA will not be designed, how will we > deal > > with the flag? Remove it from code OR update the docs to mark it > > unsupported? > > We cannot remove that flag. It will return no results until somebody > actually implements the support for snapshots with no libvirt metadata. > Since we cannot remove it, it's better to note it in the virsh manual and code comments in case it makes users confused about the none result. > Also note that for other hypervisor drivers there certainly is more > possibility for modelling that as well so removing the flag does not > make sense. > -- Best regards, --- Han Han Quality Engineer Redhat. Email: h...@redhat.com Phone: +861065339333 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 10/18] conf: Remove virDomainResctrlAppend and introduce virDomainResctrlNew
On 2018年11月06日 01:26, John Ferlan wrote:
On 10/22/18 4:01 AM, Wang Huaqiang wrote:
Introduced virDomainResctrlNew to do the most part of virDomainResctrlAppend
and move the operation of appending resctrl to @def->resctrls out of
function.
Rather than rely on virDomainResctrlAppend to perform the allocation, move
the onus to the caller and make use of virBitmapNewCopy for @vcpus and
virObjectRef for @alloc, thus removing the need to set each to NULL after the
call.
Signed-off-by: Wang Huaqiang
---
src/conf/domain_conf.c | 60 +-
1 file changed, 35 insertions(+), 25 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e8e0adc..39bd396 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -18920,26 +18920,22 @@ virDomainCachetuneDefParseCache(xmlXPathContextPtr
ctxt,
}
-static int
-virDomainResctrlAppend(virDomainDefPtr def,
- xmlNodePtr node,
- virResctrlAllocPtr alloc,
- virBitmapPtr vcpus,
- unsigned int flags)
+static virDomainResctrlDefPtr
+virDomainResctrlNew(xmlNodePtr node,
+virResctrlAllocPtr *alloc,
+virBitmapPtr *vcpus,
Because we're not "stealing" @*alloc and/or @*vcpus, they do not need to
be passed by reference. I can change these. There's some minor merge
impact in later patches too, but no big deal.
Agree. Please help make change.
+unsigned int flags)
{
char *vcpus_str = NULL;
char *alloc_id = NULL;
-virDomainResctrlDefPtr tmp_resctrl = NULL;
-int ret = -1;
-
-if (VIR_ALLOC(tmp_resctrl) < 0)
-goto cleanup;
+virDomainResctrlDefPtr resctrl = NULL;
+virDomainResctrlDefPtr ret = NULL;
/* We need to format it back because we need to be consistent in the naming
* even when users specify some "sub-optimal" string there. */
-vcpus_str = virBitmapFormat(vcpus);
+vcpus_str = virBitmapFormat(*vcpus);
if (!vcpus_str)
-goto cleanup;
+return NULL;
if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE))
alloc_id = virXMLPropString(node, "id");
@@ -18954,18 +18950,23 @@ virDomainResctrlAppend(virDomainDefPtr def,
goto cleanup;
}
/* NB: Callers assume new @alloc, need to fill in ID now */
Not that it would prevent someone in the future from passing an @alloc
w/ ->id already filled in and overwriting, but at least for now it's not
the case.
Yes, it might happen.
If @alloc->id is specified through XML and is not following the naming
convention
virAsprintf(_id, "vcpus_%s", vcpus_str)
If you think it is necessary we might need to through a warning for this
case.
With the changes (that I can make),
Reviewed-by: John Ferlan
John
Thanks for review.
Huaqiang
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 12/18] conf: Introduce cache monitor element in cachetune
On 2018年11月06日 01:26, John Ferlan wrote:
On 10/22/18 4:01 AM, Wang Huaqiang wrote:
Introducing element under to represent
a cache monitor.
Signed-off-by: Wang Huaqiang
---
docs/formatdomain.html.in | 26 +++
docs/schemas/domaincommon.rng | 10 +
src/conf/domain_conf.c | 234 -
src/conf/domain_conf.h | 11 +
tests/genericxml2xmlindata/cachetune-cdp.xml | 3 +
.../cachetune-colliding-monitor.xml| 30 +++
tests/genericxml2xmlindata/cachetune-small.xml | 7 +
tests/genericxml2xmltest.c | 2 +
8 files changed, 322 insertions(+), 1 deletion(-)
create mode 100644 tests/genericxml2xmlindata/cachetune-colliding-monitor.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index b1651e3..2fd665c 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -759,6 +759,12 @@
cachetune vcpus='0-3'
cache id='0' level='3' type='both' size='3' unit='MiB'/
cache id='1' level='3' type='both' size='3' unit='MiB'/
+ monitor level='3' vcpus='1'/
+ monitor level='3' vcpus='0-3'/
+/cachetune
+cachetune vcpus='4-5'
+ monitor level='3' vcpus='4'/
+ monitor level='3' vcpus='5'/
/cachetune
memorytune vcpus='0-3'
node id='0' bandwidth='60'/
@@ -978,6 +984,26 @@
+ monitor
+
+The optional element monitor creates the cache
+monitor(s) for current cache allocation and has the following
+required attributes:
+
+ level
+
+Host cache level the monitor belongs to.
+
+ vcpus
+
+vCPU list the monitor applies to. A monitor's vCPU list
+can only be the member(s) of the vCPU list of associating
the associated
Thanks.
+allocation. The default monitor has the same vCPU list as the
+associating allocation. For non-default monitors, there
associated
Thanks.
+are no vCPU overlap permitted.
For non-default monitors, overlapping vCPUs are not permitted.
Thanks.
+
+
+
[...]
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a068d4d..01f795f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2955,13 +2955,31 @@ virDomainLoaderDefFree(virDomainLoaderDefPtr loader)
[...]
@@ -19026,7 +19215,14 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
if (!resctrl)
goto cleanup;
-if (virResctrlAllocIsEmpty(alloc)) {
+if (virDomainResctrlMonDefParse(def, ctxt, node,
+VIR_RESCTRL_MONITOR_TYPE_CACHE,
+resctrl) < 0)
+goto cleanup;
+
+/* If no element or element in , do not
+ * append any resctrl element */
+if (virResctrlAllocIsEmpty(alloc) && !resctrl->nmonitors) {
Swap order since *IsEmpty is more compute intensive, also change to:
if (resctrl->nmonitors == 0 &&
Agree.
ret = 0;
goto cleanup;
}
@@ -27063,12 +27259,41 @@ virDomainCachetuneDefFormatHelper(unsigned int level,
static int
+virDomainResctrlMonDefFormatHelper(virDomainResctrlMonDefPtr domresmon,
+ virResctrlMonitorType tag,
+ virBufferPtr buf)
+{
+char *vcpus = NULL;
+
+if (domresmon->tag != tag)
+return 0;
+
+virBufferAddLit(buf, "
So is this because when is introduced it won't use a cache
level, right? Just trying to recall (and perhaps add a comment).
Yes. 'level' is just for cachetune monitor. Planned to add the comment when
extending this function to support memtune monitor, mentioning the memtune
monitor ( tag == VIR_RESCTRL_MONITOR_TYPE_BW) here will cause confusing.
+
+vcpus = virBitmapFormat(domresmon->vcpus);
+if (!vcpus)
+return -1;
+
+virBufferAsprintf(buf, "vcpus='%s'/>\n", vcpus);
+
+VIR_FREE(vcpus);
+return 0;
+}
+
+
[...]
I can fix the minor nits, just ack them...
Thanks please help fix.
Reviewed-by: John Ferlan
John
Thanks for fixing and the review, I will make changes in my code.
Huaqiang
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/3] qemu: Move allow reboot check setting
On 11/01/2018 05:04 PM, John Ferlan wrote: > Checking and setting the priv->allowReboot can be done before we start > processing the job. A subsequent patch will make use of the value to > make decisions in the error label, so we need to have it set properly. > > Signed-off-by: John Ferlan > --- > src/qemu/qemu_process.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index 9cf971808c..5232f761af 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -7767,6 +7767,10 @@ qemuProcessReconnect(void *opaque) > cfg = virQEMUDriverGetConfig(driver); > priv = obj->privateData; > > +/* If we are connecting to a guest started by old libvirt there is no > + * allowReboot in status XML and we need to initialize it. */ > +qemuProcessPrepareAllowReboot(obj); I'm not quite sure why this happens outside of job. It doesn't look like it has to. > + > if (!(caps = virQEMUDriverGetCapabilities(driver, false))) > goto error; > > @@ -7783,10 +7787,6 @@ qemuProcessReconnect(void *opaque) > if (qemuDomainMasterKeyReadFile(priv) < 0) > goto error; > > -/* If we are connecting to a guest started by old libvirt there is no > - * allowReboot in status XML and we need to initialize it. */ > -qemuProcessPrepareAllowReboot(obj); > - > if (qemuHostdevUpdateActiveDomainDevices(driver, obj->def) < 0) > goto error; > > Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/3] qemu: Narrow the shutdown reconnection failure reason window
On 11/01/2018 05:04 PM, John Ferlan wrote: > The current qemuProcessReconnect logic paints a broad brush > determining that the shutdown reason must be crashed if it was > determined that the domain was started with -no-shutdown; however, > there's many other ways to get to the error label, so let's narrow > our reasoning window for using VIR_DOMAIN_SHUTOFF_CRASHED to the > period where we essentially know we've tried to create to the > monitor and before we were successful in opening the connection. > > Failures that occur outside that window would thus be considered > as VIR_DOMAIN_SHUTOFF_UNKNOWN, at least for now. > > Signed-off-by: John Ferlan > --- > src/qemu/qemu_process.c | 6 +- > 1 file changed, 5 insertions(+), 1 deletion(-) ACK Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 2/3] qemu: Restore lost shutdown reason
On 11/01/2018 05:04 PM, John Ferlan wrote: > When qemuProcessReconnectHelper was introduced (commit d38897a5d) > reconnection failure used VIR_DOMAIN_SHUTOFF_FAILED; however, that > was changed in commit bda2f17d to either VIR_DOMAIN_SHUTOFF_CRASHED > or VIR_DOMAIN_SHUTOFF_UNKNOWN. > > When QEMU_CAPS_NO_SHUTDOWN checking was removed in commit fe35b1ad6 > the conditional state was just left at VIR_DOMAIN_SHUTOFF_CRASHED. > > So introduce qemuDomainIsUsingNoShutdown which will manage the > condition when the domain was started with -no-shutdown so that > when/if reconnection failure occurs we can restore the decision > point used to determine whether CRASHED or UNKNOWN is provided. > > Signed-off-by: John Ferlan > --- > src/qemu/qemu_command.c | 6 +- > src/qemu/qemu_domain.c | 17 + > src/qemu/qemu_domain.h | 3 +++ > src/qemu/qemu_process.c | 15 ++- > 4 files changed, 31 insertions(+), 10 deletions(-) ACK Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] snapshot: Don't hose list on deletion failure
On 10/18/2018 03:45 AM, Eric Blake wrote: > If qemuDomainSnapshotDiscard() fails for any reason (rare, > but possible with an ill-timed ENOMEM or if > qemuDomainSnapshotForEachQcow2() has problems talking to the > qemu guest monitor), then an attempt to retry the snapshot > deletion API will crash because we didn't undo the effects > of virDomainSnapshotDropParent() temporarily rearranging the > internal list structures, and the second attempt to drop > parents will dereference NULL. Fix it by instead noting that > there are only two callers to qemuDomainSnapshotDiscard(), > and only one of the two callers wants the parent to be updated; > thus we can move the call to virDomainSnapshotDropParent() > into a code path that only gets executed on success. > > Signed-off-by: Eric Blake > > --- > v2: avoid use-after-free > --- > src/qemu/qemu_domain.c | 6 -- > src/qemu/qemu_driver.c | 1 - > 2 files changed, 4 insertions(+), 3 deletions(-) ACK Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 04/18] util: Add interface to determine monitor path
On 2018年11月06日 01:19, John Ferlan wrote:
On 10/22/18 4:01 AM, Wang Huaqiang wrote:
Add interface for resctrl monitor to determine the path.
Signed-off-by: Wang Huaqiang
---
src/libvirt_private.syms | 1 +
src/util/virresctrl.c| 55
src/util/virresctrl.h| 5 -
3 files changed, 60 insertions(+), 1 deletion(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index d2573c5..5235046 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2680,6 +2680,7 @@ virResctrlInfoGetCache;
virResctrlInfoGetMonitorPrefix;
virResctrlInfoMonFree;
virResctrlInfoNew;
+virResctrlMonitorDeterminePath;
virResctrlMonitorNew;
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index 956aca8..1d0eb01 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -2462,3 +2462,58 @@ virResctrlMonitorNew(void)
return virObjectNew(virResctrlMonitorClass);
}
+
+
+/*
+ * virResctrlMonitorDeterminePath
+ *
+ * @monitor: Pointer to a resctrl monitor
+ * @machinename: Name string of the VM
+ *
+ * Determines the directory path that the underlying resctrl group will be
+ * created with.
+ *
+ * A monitor represents a directory under resource control file system,
+ * its directory path could be the same path as @monitor->alloc, could be a
+ * path of directory under 'mon_groups' of @monitor->alloc, or a path of
+ * directory under '/sys/fs/resctrl/mon_groups' if @monitor->alloc is NULL.
+ *
+ * Returns 0 on success, -1 on error.
+ */
+int
+virResctrlMonitorDeterminePath(virResctrlMonitorPtr monitor,
+ const char *machinename)
+{
+VIR_AUTOFREE(char *) parentpath = NULL;
+
+if (!monitor) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Invalid resctrl monitor"));
+return -1;
+}
+
+if (monitor->path) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Resctrl monitor path is already set to '%s'"),
+ monitor->path);
+return -1;
+}
+
+if (monitor->id && monitor->alloc && monitor->alloc->id) {
+if (STREQ(monitor->id, monitor->alloc->id)) {
+if (VIR_STRDUP(monitor->path, monitor->alloc->path) < 0)
+return -1;
+return 0;
+}
+}
+
+if (virAsprintf(, "%s/mon_groups", monitor->alloc->path) < 0)
Just ran the changes through Coverity - because of the above
"monitor->alloc" check, Coverity notes right here that monitor->alloc
could be NULL, so I think a check prior to here would be in order,
Yes. Here exists a risk that this line you mentioned could be executed at
the condition that @monitor->alloc is empty pointer, this will cause a
crash.
such
as either before or after the monitor->path check:
if (!monitor->alloc) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Missing resctrl monitor allocation"));
return -1;
}
Putting these lines before and after the monitor->path check should be
OK for me.
(I don't find the influence made by the order. )
Then the monitor->alloc check wouldn't be necessary. Thus the above
STRDUP becomes:
if (STREQ_NULLABLE(monitor->id, monitor->alloc->id)) {
if (VIR_STRDUP(monitor->path, monitor->alloc->path) < 0)
return -1;
return 0;
}
Agree.
Thanks.
Let me know where you want it.
John
Thanks for review.
Huaqiang
+return -1;
+
+monitor->path = virResctrlDeterminePath(parentpath, machinename,
+monitor->id);
+if (!monitor->path)
+return -1;
+
+return 0;
+}
diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
index eaa077d..baae554 100644
--- a/src/util/virresctrl.h
+++ b/src/util/virresctrl.h
@@ -191,7 +191,10 @@ virResctrlInfoGetMonitorPrefix(virResctrlInfoPtr resctrl,
typedef struct _virResctrlMonitor virResctrlMonitor;
typedef virResctrlMonitor *virResctrlMonitorPtr;
-
virResctrlMonitorPtr
virResctrlMonitorNew(void);
+
+int
+virResctrlMonitorDeterminePath(virResctrlMonitorPtr monitor,
+ const char *machinename);
#endif /* __VIR_RESCTRL_H__ */
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 09/18] util: Add more interfaces for resctrl monitor
On 2018年11月05日 23:10, John Ferlan wrote:
On 10/22/18 4:01 AM, Wang Huaqiang wrote:
Add interfaces monitor group to support operations such
as add PID, set ID, remove group ... etc.
Signed-off-by: Wang Huaqiang
---
src/libvirt_private.syms | 5 +
src/util/virresctrl.c| 47 +++
src/util/virresctrl.h| 14 ++
3 files changed, 66 insertions(+)
[...]
+int
+virResctrlMonitorRemove(virResctrlMonitorPtr monitor)
+{
+int ret = 0;
+
+if (!monitor->path)
+return 0;
Similar to patch1 - if we are using a default path, then we don't want
to removed even if it exists, so I *think* (but you need to confirm for
me) that the following should be done:
if (STREQ(monitor->path, monitor->alloc->path))
return 0;
Although I wonder if a !monitor->alloc guard should be used as well.
Whether it's part of a || !monitor->path return 0 or this check should
be "if (monitor->alloc && STREQ(...))"... Thoughts?
You are right, I should do the similar things that have done for removal
of allocation.
otherwise the allocation directory will be removed in removing monitor
group.
I did not find these, because the removal of allocation is just after
removal of
all monitor groups.
@monitor->alloc and @monitor->path should not be NULL here, so following
changes would be fine.
if (STREQ(monitor->path, monitor->alloc->path))
return 0;
Thanks
+
+VIR_DEBUG("Removing resctrl monitor%s", monitor->path);
s/monitor%s/monitor path='%s'
OK. Thanks.
+if (rmdir(monitor->path) != 0 && errno != ENOENT) {
+ret = -errno;
+VIR_ERROR(_("Unable to remove %s (%d)"), monitor->path, errno);
+}
+
+return ret;
+}
I can make the changes - just let me know your preferred way to proceed...
Reviewed-by: John Ferlan
John
Thanks for the review.
Huaqiang
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 03/18] util: Refactor code for determining allocation path
On 2018年11月05日 23:02, John Ferlan wrote:
On 10/22/18 4:01 AM, Wang Huaqiang wrote:
The code for determining resctrl allocation path could be reused
for monitor. Refactor it for reuse.
Signed-off-by: Wang Huaqiang
---
src/util/virresctrl.c | 38 ++
1 file changed, 30 insertions(+), 8 deletions(-)
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index 6530801..956aca8 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -2265,28 +2265,50 @@ virResctrlAllocAssign(virResctrlInfoPtr resctrl,
}
+static char *
+virResctrlDeterminePath(const char *parentpath,
+const char *prefix,
+const char *id)
+{
+char *path = NULL;
+
+if (!id) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Resctrl ID must be set before determining resctrl "
+ "parentpath='%s'"), parentpath);
Add "for prefix='%s'" w/ prefix as argument especially since for Alloc's
the parent path is SYSFS_RESCTRL_PATH so it's perhaps not specific enough.
With this change that I can make for you,
Your change made the message be more clear. Agree with your change.
Reviewed-by: John Ferlan
John
Thanks for your review.
Huaqiang
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v7 13/14] docs: Add 'uid' and 'fid' information
在 2018/11/5 下午9:22, Andrea Bolognani 写道: On Fri, 2018-10-19 at 11:40 +0800, Yi Min Zhao wrote: [...] @@ -3925,7 +3925,15 @@ (since 0.9.7, requires QEMU 0.13). multifunction defaults to 'off', but should be set to 'on' for function 0 of a slot that will -have multiple functions used. +have multiple functions used. +(Since 4.9.0), PCI address extensions Since 4.10.0 now ;) Yes. -- Yi Min -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 04/18] util: Add interface to determine monitor path
On 2018年11月05日 23:02, John Ferlan wrote: On 10/22/18 4:01 AM, Wang Huaqiang wrote: Add interface for resctrl monitor to determine the path. Signed-off-by: Wang Huaqiang --- src/libvirt_private.syms | 1 + src/util/virresctrl.c| 55 src/util/virresctrl.h| 5 - 3 files changed, 60 insertions(+), 1 deletion(-) Reviewed-by: John Ferlan John Thanks for the review. Huaqiang -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 02/18] util: Introduce resctrl monitor for CMT
On 2018年11月05日 23:02, John Ferlan wrote:
On 10/22/18 4:01 AM, Wang Huaqiang wrote:
Cache Monitoring Technology (aka CMT) provides the capability
to report cache utilization information of system task.
This patch introduces the concept of resctrl monitor through
data structure virResctrlMonitor.
Signed-off-by: Wang Huaqiang
---
src/libvirt_private.syms | 1 +
src/util/virresctrl.c| 79
src/util/virresctrl.h| 9 ++
3 files changed, 83 insertions(+), 6 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 335210c..d2573c5 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
[...]
@@ -275,6 +281,18 @@ virResctrlInfoMonFree(virResctrlInfoMonPtr mon)
* a sparse array to represent whether a memory bandwidth allocation happens
* on corresponding node. The available memory controller number is collected
* in 'virResctrlInfo'.
+ *
+ * =Cache monitoring technology (CMT)=
+ *
+ * Cache monitoring technology is used to perceive how many cache the process
+ * is using actually. virResctrlMonitor represents the resource control
+ * monitoring group, it is supported to monitor resource utilization
+ * information on granularity of vcpu.
+ *
+ * From hardware perspective, cache monitoring technology (CMT), memory
From a
+ * bandwidth technology (MBM), as well as the CAT and MBA, are all orthogonal
+ * features. The monitor will be created under the scope of default resctl
*resctrl
+ * group if no specific CAT or MBA entries are provided for the guest."
*/
struct _virResctrlAllocPerType {
/* There could be bool saying whether this is set or not, but since
everything
@@ -320,6 +338,29 @@ struct _virResctrlAlloc {
char *path;
};
+/*
+ * virResctrlMonitor is the data structure for resctrl monitor. Resctrl
+ * monitor represents a resctrl monitoring group, which can be used to
+ * monitor the resource utilization information for either cache or
+ * memory bandwidth.
+ */
+struct _virResctrlMonitor {
+virObject parent;
+
+/* In resctrl, each monitor is associated with one specific allocation,
Each ResctrlMonitor is associated...
+ * either the allocation under / sys / fs / resctrl or allocation of the
either the root directory allocation /sys/fs/resctrl or a specific
allocation defined under the root directory.
With these simple changes that I can make for you,
Reviewed-by: John Ferlan
John
Please help me correct these errors.
Thanks for the review.
Huaqiang
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 05/18] util: Refactor code for adding PID to the resource group
On 2018年11月05日 23:03, John Ferlan wrote: On 10/22/18 4:01 AM, Wang Huaqiang wrote: The code of adding PID to the allocation could be reused, refactor it for later reuse. Signed-off-by: Wang Huaqiang --- src/util/virresctrl.c | 30 +++--- 1 file changed, 19 insertions(+), 11 deletions(-) Reviewed-by: John Ferlan John Thanks for the review. Huaqiang -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 07/18] util: Refactor code for creating resctrl group
On 2018年11月05日 23:03, John Ferlan wrote: On 10/22/18 4:01 AM, Wang Huaqiang wrote: The code for creating resctrl allocation group could be reused for monitoring group, refactor it for reuse in the later patch. Signed-off-by: Wang Huaqiang --- src/util/virresctrl.c | 37 +++-- 1 file changed, 23 insertions(+), 14 deletions(-) Reviewed-by: John Ferlan John Thanks for the review. Huaqiang -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 06/18] util: Add interface for adding PID to the monitor
On 2018年11月05日 23:03, John Ferlan wrote: On 10/22/18 4:01 AM, Wang Huaqiang wrote: Add interface for adding task PID to the monitor. Signed-off-by: Wang Huaqiang --- src/libvirt_private.syms | 1 + src/util/virresctrl.c| 8 src/util/virresctrl.h| 4 3 files changed, 13 insertions(+) Reviewed-by: John Ferlan John Thanks for the review. Huaqiang -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 08/18] util: Add interface for creating monitor group
On 2018年11月05日 23:04, John Ferlan wrote: On 10/22/18 4:01 AM, Wang Huaqiang wrote: Add interface for creating the resource monitoring group according to '@virResctrlMonitor->path'. Signed-off-by: Wang Huaqiang --- src/libvirt_private.syms | 1 + src/util/virresctrl.c| 24 src/util/virresctrl.h| 4 3 files changed, 29 insertions(+) Reviewed-by: John Ferlan John Thanks for the review. Huaqiang -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 01/18] docs, util: Refactor schemas and virresctrl to support optional cache
On 2018年11月06日 16:18, Huaqiang,Wang wrote:
On 2018年11月05日 23:01, John Ferlan wrote:
On 10/22/18 4:01 AM, Wang Huaqiang wrote:
Refactor schemas and virresctrl to support optional element
in .
Later, the monitor entry will be introduced and to be placed
under . Either cache entry or monitor entry is
an optional element of .
An cachetune has no element is taking the default resource
allocating policy defined in '/sys/fs/resctrl/schemata'.
Signed-off-by: Wang Huaqiang
---
docs/formatdomain.html.in | 4 ++--
docs/schemas/domaincommon.rng | 4 ++--
src/util/virresctrl.c | 28
3 files changed, 32 insertions(+), 4 deletions(-)
[...]
+ /* If the allocation is empty, then the path will be
SYSFS_RESCTRL_PATH */
+ if (virResctrlAllocIsEmpty(alloc)) {
+ if (!alloc->path &&
+ VIR_STRDUP(alloc->path, SYSFS_RESCTRL_PATH) < 0)
+ return -1;
+
+ return 0;
+ }
+
Because of ...
if (!alloc->path &&
virAsprintf(>path, "%s/%s-%s",
SYSFS_RESCTRL_PATH, machinename, alloc->id) < 0)
[...]
@@ -2334,6 +2358,10 @@ virResctrlAllocRemove(virResctrlAllocPtr alloc)
{
int ret = 0;
+ /* No directory have ever been created. Just return */
+ if (virResctrlAllocIsEmpty(alloc))
+ return 0;
... the change to virResctrlAllocDeterminePath to fill in alloc->path
when virResctrlAllocIsEmpty to be a default path, this should be:
if (STREQ_NULLABLE(alloc->path, SYSFS_RESCTRL_PATH))
return 0;
or moved after the next check and the _NULLABLE removed.
Whether the AllocIsEmpty is true or not shouldn't be the bearing on
whether the directory created because of that
Agree with the changes.
"No need to create a directory that has already been created by system."
(SYSFS_RESCTRL_PATH is the created directory).
Should be "no need to destroy a system created directory, the
SYSFS_RESCTRL_PATH.
Directory SYSFS_RESCTRL_PATH is governed by system."
It might also be reasonable to make similar changes for
virResctrlAllocCreate,
because no need to create an already created directory, Right? And
*alloc->path must
be properly assigned, by virResctrlAllocDeterminePath, before a call of
virResctrlAllocCreate.
+
if (!alloc->path)
return 0;
I can adjust for you, let me know; otherwise, things are fine for the
Reviewed-by: John Ferlan
John
Thanks for the review.
Huaqiang
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RESEND] qemu: Process RDMA GID state change event
On 11/05/2018 08:27 PM, Yuval Shaia wrote: > On Mon, Nov 05, 2018 at 05:42:10PM +0100, Michal Privoznik wrote: >> [There is no need to resend your patch just to put yourself onto CC >> list. The review policy is always "reply to all"] > > Hi Michal, > > Actually i posted the "RESEND" as my original mail was not shown up in > https://www.redhat.com/archives/libvir-list so I followed the > recommendations (register to mailing list etc) and re-post. Ah right. Sometimes the list is slow and e-mails don't appear in the archives right away. > >> >> On 11/05/2018 11:50 AM, Yuval Shaia wrote: >>> This event is emitted on the monitor when a GID table in pvrdma device >>> is modified and the change needs to be propagate to the backend RDMA >>> device's GID table. >> >> Not yet, your qemu patches are not merged yet ;-) I will provide review >> anyway, but even if this patch was good as is we couldn't merge it >> unless it's merged into qemu repo first. We have our history with that. > > Yeah, not yet merged but assuming both should be merged at the same time > due to dependency i had to start the reviews in parallel. > > The qemu patches will probably merged in the in the next few weeks (v3.2) > so I'd like to do the best to be ready with this one too. Fair enough. I'm just saying that out loud. Mostly to be fair to new contributors who might not know these little nuances. > > Anyways, appreciate much your review i took all you comments and will post > v2 soon. Please check my answers/questions below. > >> >>> > >> >> Frankly, I don't know about RDMA that much, but the qemu interface looks >> weird to me. If it wants to send an IP address to us it's doing that in >> a cryptic way. > > Can you elaborate more on that? > > Actually the RDMA device does not send IP addresses, from the RDMA device > perspective those uint64_ts (subnet_prefix and interface_id) are called GID > and the only way to add new GID to RDMA device is by adding the > "equivalent" IP(v6) address to the Ethernet function of the device. > > Hope it make sense. Ah, okay. As I've said I know next to nothing about RDMA O:-) Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 01/18] docs, util: Refactor schemas and virresctrl to support optional cache
On 2018年11月05日 23:01, John Ferlan wrote:
On 10/22/18 4:01 AM, Wang Huaqiang wrote:
Refactor schemas and virresctrl to support optional element
in .
Later, the monitor entry will be introduced and to be placed
under . Either cache entry or monitor entry is
an optional element of .
An cachetune has no element is taking the default resource
allocating policy defined in '/sys/fs/resctrl/schemata'.
Signed-off-by: Wang Huaqiang
---
docs/formatdomain.html.in | 4 ++--
docs/schemas/domaincommon.rng | 4 ++--
src/util/virresctrl.c | 28
3 files changed, 32 insertions(+), 4 deletions(-)
[...]
+/* If the allocation is empty, then the path will be SYSFS_RESCTRL_PATH */
+if (virResctrlAllocIsEmpty(alloc)) {
+if (!alloc->path &&
+VIR_STRDUP(alloc->path, SYSFS_RESCTRL_PATH) < 0)
+return -1;
+
+return 0;
+}
+
Because of ...
if (!alloc->path &&
virAsprintf(>path, "%s/%s-%s",
SYSFS_RESCTRL_PATH, machinename, alloc->id) < 0)
[...]
@@ -2334,6 +2358,10 @@ virResctrlAllocRemove(virResctrlAllocPtr alloc)
{
int ret = 0;
+/* No directory have ever been created. Just return */
+if (virResctrlAllocIsEmpty(alloc))
+return 0;
... the change to virResctrlAllocDeterminePath to fill in alloc->path
when virResctrlAllocIsEmpty to be a default path, this should be:
if (STREQ_NULLABLE(alloc->path, SYSFS_RESCTRL_PATH))
return 0;
or moved after the next check and the _NULLABLE removed.
Whether the AllocIsEmpty is true or not shouldn't be the bearing on
whether the directory created because of that
Agree with the changes.
"No need to create a directory that has already been created by system."
(SYSFS_RESCTRL_PATH is the created directory).
+
if (!alloc->path)
return 0;
I can adjust for you, let me know; otherwise, things are fine for the
Reviewed-by: John Ferlan
John
Thanks for the review.
Huaqiang
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
