[libvirt] [PATCH] virsh: Drop dead variables
The helper function virshSnapshotCreate (formerly vshSnapshotCreate) has had dead variables since commit a00c37f2 (Sep 2011). Signed-off-by: Eric Blake --- Pushing under the trivial rule. tools/virsh-snapshot.c | 6 -- 1 file changed, 6 deletions(-) diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index 812fa91333..a4ea959230 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -50,9 +50,6 @@ virshSnapshotCreate(vshControl *ctl, virDomainPtr dom, const char *buffer, bool ret = false; virDomainSnapshotPtr snapshot; bool halt = false; -char *doc = NULL; -xmlDocPtr xml = NULL; -xmlXPathContextPtr ctxt = NULL; const char *name = NULL; snapshot = virDomainSnapshotCreateXML(dom, buffer, flags); @@ -101,10 +98,7 @@ virshSnapshotCreate(vshControl *ctl, virDomainPtr dom, const char *buffer, ret = true; cleanup: -xmlXPathFreeContext(ctxt); -xmlFreeDoc(xml); virshDomainSnapshotFree(snapshot); -VIR_FREE(doc); return ret; } -- 2.17.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 28/28] security_dac: Lock domain metadata
On 08/27/2018 04:08 AM, Michal Privoznik wrote:
You have nothing to say here, wow we got this far and your speechless...
Or your fingers were just really tired from all those patches!
> Signed-off-by: Michal Privoznik
> ---
> src/security/security_dac.c | 52
> +++--
> 1 file changed, 46 insertions(+), 6 deletions(-)
>
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 2115e00e07..818548dd22 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -638,6 +638,8 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr,
> virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
> struct stat sb;
> int rc;
> +bool locked = false;
> +int ret = -1;
>
> if (!path && src && src->path &&
> virStorageSourceIsLocalStorage(src))
> @@ -657,14 +659,28 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr,
> return -1;
> }
>
> +if (!S_ISDIR(sb.st_mode)) {
Seems we leave ourselves open for a few other odd combinations we don't
want... Should this be restricted to certain things like S_ISREG?
> +if (virSecurityManagerMetadataLock(mgr, path) < 0)
> +return -1;
> +locked = true;
> +}
> +
> if (virSecurityDACRememberLabel(priv, path, sb.st_uid, sb.st_gid) <
> 0)
> -return -1;
> +goto cleanup;
> }
>
> VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'",
> NULLSTR(src ? src->path : path), (long)uid, (long)gid);
>
> -return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid);
> +if (virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid) < 0)
> +goto cleanup;
> +
> +ret = 0;
> + cleanup:
> +if (locked &&
> +virSecurityManagerMetadataUnlock(mgr, path) < 0)
> +VIR_WARN("Unable to unlock metadata on %s", path);
> +return ret;
> }
>
>
> @@ -677,6 +693,9 @@
> virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr,
> int rv;
> uid_t uid = 0; /* By default return to root:root */
> gid_t gid = 0;
> +struct stat sb;
> +bool locked;
Coverity says, please initialize this to false since it's used in cleanup
> +int ret = -1;
>
> if (!path && src && src->path &&
> virStorageSourceIsLocalStorage(src))
> @@ -691,17 +710,38 @@
> virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr,
> return 0;
>
> if (path) {
> +if (stat(path, ) < 0) {
> +virReportSystemError(errno, _("unable to stat: %s"), path);
> +return -1;
> +}
> +
> +if (!S_ISDIR(sb.st_mode)) {
Similar S_ISREG
I'll wait for when there's an update before R_By, but I see nothing
inherently wrong that sticks out - I'm just in search of liquid
refreshment right now, so this is as good as it gets.
John
> +if (virSecurityManagerMetadataLock(mgr, path) < 0)
> +return -1;
> +locked = true;
> +}
> +
> rv = virSecurityDACRecallLabel(priv, path, , );
> if (rv < 0)
> -return -1;
> -if (rv > 0)
> -return 0;
> +goto cleanup;
> +if (rv > 0) {
> +ret = 0;
> +goto cleanup;
> +}
> }
>
> VIR_INFO("Restoring DAC user and group on '%s' to %ld:%ld",
> NULLSTR(src ? src->path : path), (long)uid, (long)gid);
>
> -return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid);
> +if (virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid) < 0)
> +goto cleanup;
> +
> +ret = 0;
> + cleanup:
> +if (locked &&
> +virSecurityManagerMetadataUnlock(mgr, path) < 0)
> +VIR_WARN("Unable to unlock metadata on %s", path);
> +return ret;
> }
>
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 27/28] security_dac: Move transaction handling up one level
On 08/27/2018 04:08 AM, Michal Privoznik wrote:
> So far the whole transaction handling is done
> virSecurityDACSetOwnershipInternal(). This needs to change for
> the sake of security label remembering and locking. Otherwise we
> would be locking a path when only appending it to transaction
> list and not when actually relabelling it.
relabeling
>
> Signed-off-by: Michal Privoznik
> ---
> src/security/security_dac.c | 65
> ++---
> 1 file changed, 44 insertions(+), 21 deletions(-)
>
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 7528d8ba7d..2115e00e07 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -77,12 +77,13 @@ struct _virSecurityDACChownItem {
> const virStorageSource *src;
> uid_t uid;
> gid_t gid;
> +bool restore;
> };
>
> typedef struct _virSecurityDACChownList virSecurityDACChownList;
> typedef virSecurityDACChownList *virSecurityDACChownListPtr;
> struct _virSecurityDACChownList {
> -virSecurityDACDataPtr priv;
> +virSecurityManagerPtr manager;
> virSecurityDACChownItemPtr *items;
> size_t nItems;
> };
> @@ -95,7 +96,8 @@ virSecurityDACChownListAppend(virSecurityDACChownListPtr
> list,
>const char *path,
>const virStorageSource *src,
>uid_t uid,
> - gid_t gid)
> + gid_t gid,
> + bool restore)
> {
> int ret = -1;
> char *tmp = NULL;
> @@ -111,6 +113,7 @@ virSecurityDACChownListAppend(virSecurityDACChownListPtr
> list,
> item->src = src;
> item->uid = uid;
> item->gid = gid;
> +item->restore = restore;
>
> if (VIR_APPEND_ELEMENT(list->items, list->nItems, item) < 0)
> goto cleanup;
> @@ -159,25 +162,29 @@ static int
> virSecurityDACTransactionAppend(const char *path,
> const virStorageSource *src,
> uid_t uid,
> -gid_t gid)
> +gid_t gid,
> +bool restore)
> {
> virSecurityDACChownListPtr list = virThreadLocalGet();
> if (!list)
> return 0;
>
> -if (virSecurityDACChownListAppend(list, path, src, uid, gid) < 0)
> +if (virSecurityDACChownListAppend(list, path, src, uid, gid, restore) <
> 0)
> return -1;
>
> return 1;
> }
>
>
> -static int virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv,
> - const virStorageSource *src,
> - const char *path,
> - uid_t uid,
> - gid_t gid);
> +static int virSecurityDACSetOwnership(virSecurityManagerPtr mgr,
> + const virStorageSource *src,
> + const char *path,
> + uid_t uid,
> + gid_t gid);
>
> +static int virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr,
> + const virStorageSource
> *src,
> + const char *path);
> /**
> * virSecurityDACTransactionRun:
> * @pid: process pid
> @@ -201,11 +208,16 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
> virSecurityDACChownItemPtr item = list->items[i];
>
> /* TODO Implement rollback */
^^ Does this need to be removed on the next patch?
> -if (virSecurityDACSetOwnershipInternal(list->priv,
> - item->src,
> - item->path,
> - item->uid,
> - item->gid) < 0)
> +if ((!item->restore &&
> + virSecurityDACSetOwnership(list->manager,
> +item->src,
> +item->path,
> +item->uid,
> +item->gid) < 0) ||
> +(item->restore &&
> + virSecurityDACRestoreFileLabelInternal(list->manager,
> +item->src,
> +item->path) < 0))
Logic is OK, just harder to read this way.
> return -1;
> }
>
> @@ -455,7 +467,6 @@ virSecurityDACPreFork(virSecurityManagerPtr mgr)
> static int
> virSecurityDACTransactionStart(virSecurityManagerPtr mgr)
> {
> -virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
> virSecurityDACChownListPtr list;
>
>
Re: [libvirt] [PATCH v3 24/28] security_dac: Pass virSecurityManagerPtr to virSecurityDACRestoreFileLabelInternal
On 08/27/2018 04:08 AM, Michal Privoznik wrote: > This function is going call security manager APIs and therefore > it needs pointer to it. > > Signed-off-by: Michal Privoznik > --- > src/security/security_dac.c | 43 ++- > 1 file changed, 18 insertions(+), 25 deletions(-) > Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 26/28] security_dac: Fix const correctness
On 08/27/2018 04:08 AM, Michal Privoznik wrote: > These two functions (virSecurityDACSetOwnership and > virSecurityDACRestoreFileLabelInternal) do not really change > @src. Make it const. > > Signed-off-by: Michal Privoznik > --- > src/security/security_dac.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 25/28] security_dac: Fix info messages when chown()-ing
On 08/27/2018 04:08 AM, Michal Privoznik wrote:
> Firstly, the message that says we're setting uid:gid shouldn't be
> called from virSecurityDACSetOwnershipInternal() because
> virSecurityDACRestoreFileLabelInternal() is calling it too.
> Secondly, there are places between us reporting label restore and
> us actually doing it where we can quit. Don't say we're doing
> something until we are actually about to do it.
>
> Signed-off-by: Michal Privoznik
> ---
> src/security/security_dac.c | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 3d0c8d20cb..1be4ead21c 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -563,9 +563,6 @@ virSecurityDACSetOwnershipInternal(const
> virSecurityDACData *priv,
> else if (rc > 0)
> return 0;
>
> -VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'",
> - NULLSTR(src ? src->path : path), (long)uid, (long)gid);
> -
> if (priv && src && priv->chownCallback) {
> rc = priv->chownCallback(src, uid, gid);
> /* here path is used only for error messages */
> @@ -649,6 +646,9 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr,
> return -1;
> }
>
> +VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'",
> + NULLSTR(src ? src->path : path), (long)uid, (long)gid);
> +
Moving this here means the message could be printed before
virSecurityDACTransactionAppend is called (success or failure).
Besides if it fails, then we print this message, but it really didn't
happen.
So maybe this changes to "Preparing to set DAC user and group on ..."
and the cut lines move and are copied to just before the chown and the
chownCallback. That way you know that the chown was done and didn't have
a intermediate failure. Or just before return 0 after the (rc < 0)
checks move the "Setting..." message and change to "Set the..."
> return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid);
> }
>
> @@ -663,9 +663,6 @@
> virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr,
> uid_t uid = 0; /* By default return to root:root */
> gid_t gid = 0;
>
> -VIR_INFO("Restoring DAC user and group on '%s'",
> - NULLSTR(src ? src->path : path));
> -
> if (!path && src && src->path &&
> virStorageSourceIsLocalStorage(src))
> path = src->path;
> @@ -678,6 +675,9 @@
> virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr,
> return 0;
> }
>
> +VIR_INFO("Restoring DAC user and group on '%s' to %ld:%ld",
> + NULLSTR(src ? src->path : path), (long)uid, (long)gid);
> +
This one is fine, but one could change the removed lines to "Preparing
to restore the DAC user and group on %s"... then if *Internal prints,
then you know it happened; otherwise, something prevented it.
John
> return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid);
> }
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 23/28] security_dac: Pass virSecurityManagerPtr to virSecurityDACSetOwnership
On 08/27/2018 04:08 AM, Michal Privoznik wrote: > This function is going call security manager APIs and therefore > it needs pointer to it. > > Signed-off-by: Michal Privoznik > --- > src/security/security_dac.c | 37 +++-- > 1 file changed, 19 insertions(+), 18 deletions(-) > Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 20/28] security_manager: Load lock plugin on init
On 08/27/2018 04:08 AM, Michal Privoznik wrote:
> When creating the security managers stack load the lock plugin
> too. This is done by creating a single object that all secdrivers
> take a reference to. We have to have one shared object so that
> the connection to virlockd can be shared between individual
> secdrivers. It is important that the connection is shared because
> if the connection is closed from one driver while other has a
> file locked, then virtlockd does its job and kills libvirtd.
>
> The cfg.mk change is needed in order to allow syntax-check
> to include lock_manager.h. This is generally safe thing to do as
> this APIs defined there will always exist. However, instead of
> allowing the include for all other drivers (like cpu, network,
> and so on) allow it only for security driver. This will still
> trigger the error if including from other drivers.
>
> Signed-off-by: Michal Privoznik
> ---
> cfg.mk | 4 +-
> src/qemu/qemu_driver.c | 12 --
> src/security/security_manager.c | 81
> -
> src/security/security_manager.h | 3 +-
> tests/testutilsqemu.c | 2 +-
> 5 files changed, 94 insertions(+), 8 deletions(-)
>
> diff --git a/cfg.mk b/cfg.mk
> index 609ae869c2..e0a7b5105a 100644
> --- a/cfg.mk
> +++ b/cfg.mk
> @@ -787,8 +787,10 @@ sc_prohibit_cross_inclusion:
> case $$dir in \
> util/) safe="util";; \
> access/ | conf/) safe="($$dir|conf|util)";; \
> - cpu/| network/| node_device/| rpc/| security/| storage/) \
> + cpu/| network/| node_device/| rpc/| storage/) \
> safe="($$dir|util|conf|storage)";; \
> + security/) \
> + safe="($$dir|util|conf|storage|locking)";; \
> xenapi/ | xenconfig/ ) safe="($$dir|util|conf|xen|cpu)";; \
> *) safe="($$dir|$(mid_dirs)|util)";; \
> esac; \
This I don't really understand - black magic, voodoo stuff ;-)
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index da8c4e8991..e06dee8dfb 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -358,7 +358,9 @@ qemuSecurityInit(virQEMUDriverPtr driver)
> flags)))
> goto error;
> if (!stack) {
> -if (!(stack = qemuSecurityNewStack(mgr)))
> +if (!(stack = qemuSecurityNewStack(mgr,
> +
> cfg->metadataLockManagerName ?
> +
> cfg->metadataLockManagerName : "nop")))
> goto error;
> } else {
> if (qemuSecurityStackAddNested(stack, mgr) < 0)
> @@ -372,7 +374,9 @@ qemuSecurityInit(virQEMUDriverPtr driver)
> QEMU_DRIVER_NAME,
> flags)))
> goto error;
> -if (!(stack = qemuSecurityNewStack(mgr)))
> +if (!(stack = qemuSecurityNewStack(mgr,
> + cfg->metadataLockManagerName ?
> + cfg->metadataLockManagerName :
> "nop")))
> goto error;
> mgr = NULL;
> }
> @@ -389,7 +393,9 @@ qemuSecurityInit(virQEMUDriverPtr driver)
> qemuSecurityChownCallback)))
> goto error;
> if (!stack) {
> -if (!(stack = qemuSecurityNewStack(mgr)))
> +if (!(stack = qemuSecurityNewStack(mgr,
> + cfg->metadataLockManagerName ?
> + cfg->metadataLockManagerName
> : "nop")))
This essentially gets called through the driver state initialize
function, right? But, wouldn't daemon locks be at a different level?
The domain locks would be managed by whatever is managing the domain,
the the daemon locks would be managed by whatever is managing the daemon
locks, wouldn't they?
This also assumes that security_driver is defined/used which wasn't
described in the previous patch (while you understand that, it's not
necessarily that obvious). If I just uncomment metadata_lock_manager,
but not security_driver, then nothing would happen.
I'm trying to figure out the "owner" (so to speak) of this lock. If
multiple daemons can use it, then someone has to own it. This partially
makes it appear that qemu would own it, but I would think virtlockd
would need to own it. As it, when something causes virtlockd to start
so that it's managing locks, that's where initialization takes place.
The fact that qemu has an "interest" in knowing if the running lockd
code is/can handle these metadata locks still would seemingly mean that
the lockmanager would need to have a way to indicate that it is managing
the locks.
Maybe it's just patch order that is causing the blank stare I have.
Maybe the answer lies in a
Re: [libvirt] [PATCH v3 22/28] security_manager: Introduce metadata locking APIs
On 08/27/2018 04:08 AM, Michal Privoznik wrote:
> Expose two APIs to lock and unlock metadata for given path. As
> the comment from the header file says, this is somewhat
> cumbersome, but it does not seem there is a better way.
>
> The idea is that a security driver (like DAC or SELinux) will
> call virSecurityManagerMetadataLock() just before they are about
> to change the label followed by
> virSecurityManagerMetadataUnlock() immediately after.
>
> Now, because we can not make virlockd multithreaded (it uses
> process associated POSIX locks where if one thread holds a lock
> and another one open()+close() the same file it causes the lock
> to be released), we can't have virtlockd to wait for the lock to
> be set. There is just one thread so if that one waits for the
> lock to be set there will not be another one coming to release
> the lock. Therefore we have to implement 'try-set' at libvirtd
> side. This is done by calling virLockManagerAcquire() in a loop
> with possible usleep() until certain timeout is reached. Out of
> thin air, the deadline was chosen to be 10 seconds with the
> maximum sleeping time of 100 ms.
>
> Signed-off-by: Michal Privoznik
> ---
> src/security/security_manager.c | 184
>
> src/security/security_manager.h | 14 +++
> 2 files changed, 198 insertions(+)
>
> diff --git a/src/security/security_manager.c b/src/security/security_manager.c
> index 2238c75a5c..3ab06e0c4a 100644
> --- a/src/security/security_manager.c
> +++ b/src/security/security_manager.c
> @@ -28,7 +28,10 @@
> #include "viralloc.h"
> #include "virobject.h"
> #include "virlog.h"
> +#include "virstring.h"
> #include "locking/lock_manager.h"
> +#include "virrandom.h"
> +#include "virtime.h"
>
> #define VIR_FROM_THIS VIR_FROM_SECURITY
>
> @@ -1389,3 +1392,184 @@
> virSecurityManagerRestoreTPMLabels(virSecurityManagerPtr mgr,
>
> return 0;
> }
> +
> +
> +static virLockManagerPtr
> +virSecurityManagerNewLockManager(virSecurityManagerLockPtr mgrLock)
> +{
> +virLockManagerPtr lock;
> +virLockManagerParam params[] = {
> +{ .type = VIR_LOCK_MANAGER_PARAM_TYPE_UUID,
> +.key = "uuid",
> +},
> +{ .type = VIR_LOCK_MANAGER_PARAM_TYPE_STRING,
> +.key = "name",
> +.value = { .cstr = "libvirtd-sec" },
> +},
> +{ .type = VIR_LOCK_MANAGER_PARAM_TYPE_UINT,
> +.key = "pid",
> +.value = { .iv = getpid() },
> +},
> +};
> +const unsigned int flags = 0;
> +
> +if (virGetHostUUID(params[0].value.uuid) < 0)
> +return NULL;
> +
> +if (!(lock =
> virLockManagerNew(virLockManagerPluginGetDriver(mgrLock->lockPlugin),
> + VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON,
> + ARRAY_CARDINALITY(params),
> + params,
> + flags)))
> +return NULL;
> +
> +return lock;
> +}
> +
> +
> +/* How many miliseconds should we wait for the lock to be
milliseconds
> + * acquired before claiming error. */
> +#define METADATA_LOCK_WAIT_MAX (10 * 1000)
> +
> +/* What is the maximum sleeping time (in miliseconds) between
^^^
consistent at least ;-)
> + * retries. */
> +#define METADATA_LOCK_SLEEP_MAX (100)
or
# define METADATA_LOCK_WAIT_MAX (100 * METADATA_LOCK_SLEEP_MAX)
> +
Could use a few words of wisdom here - it's not necessary self documenting.
> +int
> +virSecurityManagerMetadataLock(virSecurityManagerPtr mgr,
> + const char *path)
> +{
> +virSecurityManagerLockPtr lock = mgr->lock;
> +unsigned long long now;
> +unsigned long long then;
> +int ret = -1;
> +
> +VIR_DEBUG("mgr=%p path=%s lock=%p", mgr, path, lock);
> +
> +if (!lock)
> +return 0;
I'm still wondering how this could be true... If this happens and we
return 0, couldn't the caller have a false sense of security?
> +
> +virObjectLock(lock);
> +
> +while (lock->pathLocked) {
Someone already operating on the thing.
> +if (virCondWait(>cond, >parent.lock) < 0) {
virCondWaitUntil perhaps?
> +virReportSystemError(errno, "%s",
> + _("failed to wait on metadata condition"));
> +goto cleanup;
> +}
If we get here, but considering the previous patch where something else
"force"'d the CondSignal, then patchLocked == false now... So if there
were more than 1 waiter what's going to happen next...
Should this fail? Should that force code set a flag or something to
indicate everyone start walking the plank?
> +}
> +
> +if (!lock->lock &&
> +!(lock->lock = virSecurityManagerNewLockManager(lock)))
> +goto cleanup;
Finally we're getting lock->lock filled in, knew it would happen some day!
> +
> +if (virLockManagerAddResource(lock->lock,
Re: [libvirt] [PATCH v3 21/28] security_manager: Introduce virSecurityManagerLockCloseConn
On 08/27/2018 04:08 AM, Michal Privoznik wrote:
> This is basically just a wrapper over virLockManagerCloseConn()
> so that no connection is left open when it shouldn't be.
>
> Signed-off-by: Michal Privoznik
> ---
> src/security/security_manager.c | 75
> +
> 1 file changed, 75 insertions(+)
>
> diff --git a/src/security/security_manager.c b/src/security/security_manager.c
> index caaff1f703..2238c75a5c 100644
> --- a/src/security/security_manager.c
> +++ b/src/security/security_manager.c
> @@ -91,6 +91,56 @@ virSecurityManagerLockDispose(void *obj)
> }
>
>
> +static void
> +virSecurityManagerLockCloseConnLocked(virSecurityManagerLockPtr lock,
> + bool force)
> +{
> +int rc;
> +
> +if (!lock)
> +return;
Not possible - if this is true you may just want to abort or let the
following code fail miserably. The definition of the API is that it's
called when @lock is locked!
> +
> +while (!force &&
> + lock->pathLocked) {
No need to split && across 2 lines. Still waiting for @pathLocked =
true ;-)
> +if (virCondWait(>cond, >parent.lock) < 0) {
> +VIR_WARN("Unable to wait on metadata condition");
> +return;
> +}
> +}
> +
> +rc = virLockManagerCloseConn(lock->lock, 0);
still haven't filled in lock->lock either
> +if (rc < 0)
> +return;
> +if (rc > 0)
> +lock->lock = NULL;
> +
> +if (force) {
> +/* We've closed the connection. Wake up anybody who might be
s/the/our/
> + * waiting. */
> +lock->pathLocked = false;
> +virCondSignal(>cond);
> +}
> +}
> +
> +
> +static void
> +virSecurityManagerLockCloseConn(virSecurityManagerLockPtr lock)
> +{
> +if (!lock)
> +return;
So this is lock->lock from a few patches ago.
> +
> +virObjectLock(lock);
> +
> +if (!lock->lock)
Still not seeing lock->lock, but maybe the next patch exposes that part.
> +goto cleanup;
> +
> +virSecurityManagerLockCloseConnLocked(lock, false);
> +
> + cleanup:
> +virObjectUnlock(lock);
> +}
> +
> +
I'm staring blankly at the rest of changes wondering, h... what am I
missing.
Beyond that if the mgr->drv->*(mgr) isn't called, then should we call
the CloseConn. I'm not seeing why it's called in the first place!
John
> static int
> virSecurityManagerOnceInit(void)
> {
> @@ -334,6 +384,7 @@ virSecurityManagerTransactionStart(virSecurityManagerPtr
> mgr)
> virObjectLock(mgr);
> if (mgr->drv->transactionStart)
> ret = mgr->drv->transactionStart(mgr);
> +virSecurityManagerLockCloseConn(mgr->lock);
> virObjectUnlock(mgr);
> return ret;
> }
> @@ -362,6 +413,7 @@ virSecurityManagerTransactionCommit(virSecurityManagerPtr
> mgr,
> virObjectLock(mgr);
> if (mgr->drv->transactionCommit)
> ret = mgr->drv->transactionCommit(mgr, pid);
> +virSecurityManagerLockCloseConn(mgr->lock);
> virObjectUnlock(mgr);
> return ret;
> }
> @@ -379,6 +431,7 @@ virSecurityManagerTransactionAbort(virSecurityManagerPtr
> mgr)
> virObjectLock(mgr);
> if (mgr->drv->transactionAbort)
> mgr->drv->transactionAbort(mgr);
> +virSecurityManagerLockCloseConn(mgr->lock);
> virObjectUnlock(mgr);
> }
>
> @@ -487,6 +540,7 @@ virSecurityManagerRestoreDiskLabel(virSecurityManagerPtr
> mgr,
> int ret;
> virObjectLock(mgr);
> ret = mgr->drv->domainRestoreSecurityDiskLabel(mgr, vm, disk);
> +virSecurityManagerLockCloseConn(mgr->lock);
> virObjectUnlock(mgr);
> return ret;
> }
> @@ -515,6 +569,7 @@ virSecurityManagerRestoreImageLabel(virSecurityManagerPtr
> mgr,
> int ret;
> virObjectLock(mgr);
> ret = mgr->drv->domainRestoreSecurityImageLabel(mgr, vm, src);
> +virSecurityManagerLockCloseConn(mgr->lock);
> virObjectUnlock(mgr);
> return ret;
> }
> @@ -532,6 +587,7 @@
> virSecurityManagerSetDaemonSocketLabel(virSecurityManagerPtr mgr,
> int ret;
> virObjectLock(mgr);
> ret = mgr->drv->domainSetSecurityDaemonSocketLabel(mgr, vm);
> +virSecurityManagerLockCloseConn(mgr->lock);
> virObjectUnlock(mgr);
> return ret;
> }
> @@ -549,6 +605,7 @@ virSecurityManagerSetSocketLabel(virSecurityManagerPtr
> mgr,
> int ret;
> virObjectLock(mgr);
> ret = mgr->drv->domainSetSecuritySocketLabel(mgr, vm);
> +virSecurityManagerLockCloseConn(mgr->lock);
> virObjectUnlock(mgr);
> return ret;
> }
> @@ -566,6 +623,7 @@ virSecurityManagerClearSocketLabel(virSecurityManagerPtr
> mgr,
> int ret;
> virObjectLock(mgr);
> ret = mgr->drv->domainClearSecuritySocketLabel(mgr, vm);
> +virSecurityManagerLockCloseConn(mgr->lock);
> virObjectUnlock(mgr);
> return ret;
Re: [libvirt] [PATCH 4/6] conf: Move virDomainPCIAddressAsString() to device_conf
On Fri, 2018-08-31 at 15:29 +0100, Daniel P. Berrangé wrote:
> On Fri, Aug 31, 2018 at 04:25:25PM +0200, Andrea Bolognani wrote:
> > On Fri, 2018-08-31 at 15:05 +0100, Daniel P. Berrangé wrote:
> > > This should really be in src/util/virpci.{c,h}, since that's where the
> > > virPCIDeviceAddressPtr struct is declared. There's nothing XML related
> > > about this string conversion, so doesn't belong in src/conf at all.
> >
> > See the commit message :)
> >
> > I can move this to util/virpci instead of conf/device_conf for
> > the time being if you prefer, but at some point we're going to
> > have to pick a place for *all* functions related to PCI addresses
> > and conf/device_conf is the most sensible option IMHO, seeing as
> > all other address types and related functions are defined there.
>
> The device_conf file is dealing with domain device addresses. The
> virPCIDeviceAddressPtr struct is independant of domain device
> addresses. It is used across domain, node device, network and
> storage drivers.
Yeah, it's a mess. I'll move it to util/virpci instead, but that
won't make the mess go away :)
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/6] conf: Move virDomainPCIAddressAsString() to device_conf
On Fri, Aug 31, 2018 at 04:25:25PM +0200, Andrea Bolognani wrote:
> On Fri, 2018-08-31 at 15:05 +0100, Daniel P. Berrangé wrote:
> > On Fri, Aug 31, 2018 at 04:00:45PM +0200, Andrea Bolognani wrote:
> > > Unfortunately, even after this change functions
> > > handling virPCIDeviceAddress are split pretty much
> > > evenly between conf/device_conf and utils/virpci:
> > > ideally everything would be moved to the former,
> > > including the struct declaration itself, and all the
> > > names would be changed to be consistent with the rest
> > > of the virDomainDevice*Address, but that's a cleanup
> > > for another day.
> [...]
> > > +char *
> > > +virDomainPCIAddressAsString(virPCIDeviceAddressPtr addr)
> > > +{
> > > +char *str;
> > > +
> > > +ignore_value(virAsprintf(, "%.4x:%.2x:%.2x.%.1x",
> > > + addr->domain,
> > > + addr->bus,
> > > + addr->slot,
> > > + addr->function));
> > > +return str;
> > > +}
> >
> > This should really be in src/util/virpci.{c,h}, since that's where the
> > virPCIDeviceAddressPtr struct is declared. There's nothing XML related
> > about this string conversion, so doesn't belong in src/conf at all.
>
> See the commit message :)
>
> I can move this to util/virpci instead of conf/device_conf for
> the time being if you prefer, but at some point we're going to
> have to pick a place for *all* functions related to PCI addresses
> and conf/device_conf is the most sensible option IMHO, seeing as
> all other address types and related functions are defined there.
The device_conf file is dealing with domain device addresses. The
virPCIDeviceAddressPtr struct is independant of domain device
addresses. It is used across domain, node device, network and
storage drivers.
For that matter device_conf should probably be domain_device_conf
as a name.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/6] conf: Move virDomainPCIAddressAsString() to device_conf
On Fri, 2018-08-31 at 15:05 +0100, Daniel P. Berrangé wrote:
> On Fri, Aug 31, 2018 at 04:00:45PM +0200, Andrea Bolognani wrote:
> > Unfortunately, even after this change functions
> > handling virPCIDeviceAddress are split pretty much
> > evenly between conf/device_conf and utils/virpci:
> > ideally everything would be moved to the former,
> > including the struct declaration itself, and all the
> > names would be changed to be consistent with the rest
> > of the virDomainDevice*Address, but that's a cleanup
> > for another day.
[...]
> > +char *
> > +virDomainPCIAddressAsString(virPCIDeviceAddressPtr addr)
> > +{
> > +char *str;
> > +
> > +ignore_value(virAsprintf(, "%.4x:%.2x:%.2x.%.1x",
> > + addr->domain,
> > + addr->bus,
> > + addr->slot,
> > + addr->function));
> > +return str;
> > +}
>
> This should really be in src/util/virpci.{c,h}, since that's where the
> virPCIDeviceAddressPtr struct is declared. There's nothing XML related
> about this string conversion, so doesn't belong in src/conf at all.
See the commit message :)
I can move this to util/virpci instead of conf/device_conf for
the time being if you prefer, but at some point we're going to
have to pick a place for *all* functions related to PCI addresses
and conf/device_conf is the most sensible option IMHO, seeing as
all other address types and related functions are defined there.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] rpc: fix handling of SSH auth failure code
On Fri, Aug 31, 2018 at 04:16:45PM +0200, Andrea Bolognani wrote: > On Fri, 2018-08-31 at 11:15 +0100, Daniel P. Berrangé wrote: > > The result of libssh2_userauth_password is being assigned to 'ret' in > > one branch and 'rc' in the other branch. Checks are all done against the > > 'ret' variable, so one branch never does the correct check. > > > > Signed-off-by: Daniel P. Berrangé > > --- > > src/rpc/virnetsshsession.c | 10 +- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > The fix itself is correct, so: > > Reviewed-by: Andrea Bolognani > > However, see below. > > > /* determine exist status */ > > -if (ret == LIBSSH2_ERROR_AUTHENTICATION_FAILED) > > +if (rc == LIBSSH2_ERROR_AUTHENTICATION_FAILED) > > return 1; > > else > > return -1; > > Since we have a cleanup label right after this, it would make > sense to change both 'return' to 'ret = ': that would ensure > the function has a single return and also prevent 'password' > from leaking when an error which is not AUTHENTICATION_FAILED > is returned by libssh2_userauth_password(). Yes, makes sense. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] rpc: fix handling of SSH auth failure code
On Fri, 2018-08-31 at 11:15 +0100, Daniel P. Berrangé wrote: > The result of libssh2_userauth_password is being assigned to 'ret' in > one branch and 'rc' in the other branch. Checks are all done against the > 'ret' variable, so one branch never does the correct check. > > Signed-off-by: Daniel P. Berrangé > --- > src/rpc/virnetsshsession.c | 10 +- > 1 file changed, 5 insertions(+), 5 deletions(-) The fix itself is correct, so: Reviewed-by: Andrea Bolognani However, see below. > /* determine exist status */ > -if (ret == LIBSSH2_ERROR_AUTHENTICATION_FAILED) > +if (rc == LIBSSH2_ERROR_AUTHENTICATION_FAILED) > return 1; > else > return -1; Since we have a cleanup label right after this, it would make sense to change both 'return' to 'ret = ': that would ensure the function has a single return and also prevent 'password' from leaking when an error which is not AUTHENTICATION_FAILED is returned by libssh2_userauth_password(). -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/6] conf: Move virDomainPCIAddressAsString() to device_conf
On Fri, Aug 31, 2018 at 04:00:45PM +0200, Andrea Bolognani wrote:
> It's a better fit than domain_conf.
>
> Unfortunately, even after this change functions
> handling virPCIDeviceAddress are split pretty much
> evenly between conf/device_conf and utils/virpci:
> ideally everything would be moved to the former,
> including the struct declaration itself, and all the
> names would be changed to be consistent with the rest
> of the virDomainDevice*Address, but that's a cleanup
> for another day.
>
> Signed-off-by: Andrea Bolognani
> ---
> src/conf/device_conf.c | 13 +
> src/conf/device_conf.h | 3 +++
> src/conf/domain_addr.c | 14 --
> src/conf/domain_addr.h | 3 ---
> src/libvirt_private.syms | 2 +-
> 5 files changed, 17 insertions(+), 18 deletions(-)
>
> diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
> index 1565d43fe6..afa06c3cda 100644
> --- a/src/conf/device_conf.c
> +++ b/src/conf/device_conf.c
> @@ -309,6 +309,19 @@ virPCIDeviceAddressFormat(virBufferPtr buf,
> return 0;
> }
>
> +char *
> +virDomainPCIAddressAsString(virPCIDeviceAddressPtr addr)
> +{
> +char *str;
> +
> +ignore_value(virAsprintf(, "%.4x:%.2x:%.2x.%.1x",
> + addr->domain,
> + addr->bus,
> + addr->slot,
> + addr->function));
> +return str;
> +}
This should really be in src/util/virpci.{c,h}, since that's where the
virPCIDeviceAddressPtr struct is declared. There's nothing XML related
about this string conversion, so doesn't belong in src/conf at all.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] qemu: Unify generation of command line for virtio devices
A virtio device such as
will be translated to one of four different QEMU devices
based on the address type. This behavior is the same for
all virtio devices, but unfortunately we have separate
ad-hoc code dealing with each and every one of them: not
only this is pointless duplication, but it turns out
that most of that code is not robust against new address
types being introduced and some of it is outright buggy.
Introduce a new function, qemuBuildVirtioDevStr(), which
deals with the issue in a generic fashion, and rewrite
all existing code to use it.
This fixes a bunch of issues such as virtio-serial-pci
being used with virtio-mmio addresses and virtio-gpu
not being usable at all with virtio-mmio addresses.
It also introduces a couple of minor regressions,
namely no longer erroring out when attempting to
use virtio-balloon and virtio-input devices with
virtio-s390 addresses; that said, virtio-s390 has
been superseded by virtio-ccw such a long time ago
that recent QEMU releases have dropped support for
the former entirely, so re-implementing such
device-specific validation is not worth it.
Signed-off-by: Andrea Bolognani
---
src/qemu/qemu_command.c | 299 ++--
1 file changed, 163 insertions(+), 136 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 8aa20496bc..04554d958b 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1812,6 +1812,62 @@ qemuBuildDriveDevCacheStr(virDomainDiskDefPtr disk,
}
+static char*
+qemuBuildVirtioDevStr(const virDomainDeviceInfo *info,
+ const char *baseName)
+{
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+const char *implName = NULL;
+
+switch ((virDomainDeviceAddressType)info->type) {
+case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI:
+implName = "pci";
+break;
+
+case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO:
+implName = "device";
+break;
+
+case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW:
+implName = "ccw";
+break;
+
+case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390:
+implName = "s390";
+break;
+
+case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE:
+case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL:
+case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCID:
+case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB:
+case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO:
+case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_ISA:
+case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM:
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unexpected address type for %s"), baseName);
+goto error;
+
+case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE:
+case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST:
+default:
+virReportEnumRangeError(virDomainDeviceAddressType,
+info->type);
+goto error;
+}
+
+virBufferAsprintf(, "%s-%s", baseName, implName);
+
+if (virBufferCheckError() < 0)
+goto error;
+
+return virBufferContentAndReset();
+
+ error:
+virBufferFreeAndReset();
+return NULL;
+}
+
+
char *
qemuBuildDiskDeviceStr(const virDomainDef *def,
virDomainDiskDefPtr disk,
@@ -1822,6 +1878,7 @@ qemuBuildDiskDeviceStr(const virDomainDef *def,
const char *bus = virDomainDiskQEMUBusTypeToString(disk->bus);
const char *contAlias;
char *backendAlias = NULL;
+char *devStr = NULL;
int controllerModel;
if (qemuCheckDiskConfig(disk, qemuCaps) < 0)
@@ -2002,21 +2059,19 @@ qemuBuildDiskDeviceStr(const virDomainDef *def,
break;
case VIR_DOMAIN_DISK_BUS_VIRTIO:
-if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
-virBufferAddLit(, "virtio-blk-ccw");
-if (disk->iothread)
-virBufferAsprintf(, ",iothread=iothread%u",
disk->iothread);
-} else if (disk->info.type ==
- VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) {
-virBufferAddLit(, "virtio-blk-s390");
-} else if (disk->info.type ==
- VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO) {
-virBufferAddLit(, "virtio-blk-device");
-} else {
-virBufferAddLit(, "virtio-blk-pci");
-if (disk->iothread)
-virBufferAsprintf(, ",iothread=iothread%u",
disk->iothread);
+
+if (!(devStr = qemuBuildVirtioDevStr(&(disk->info), "virtio-blk")))
+goto error;
+
+virBufferAddStr(, devStr);
+VIR_FREE(devStr);
+
+if (disk->iothread &&
+(disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW ||
+ disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) {
+virBufferAsprintf(, ",iothread=iothread%u", disk->iothread);
}
+
qemuBuildIoEventFdStr(, disk->ioeventfd, qemuCaps);
if (disk->event_idx &&
virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_BLK_EVENT_IDX)) {
@@ -2150,6 +2205,7 @@
[libvirt] [PATCH 1/2] tests: Fix use of virtio-serial for aarch64/virt
virtio-serial is an alias for virtio-serial-pci, which
should not have been used for a PCIe-less aarch64/virt
guest but it ended up being used anyway because the
virtio-mmio capability was missing and the algorithm
is buggy.
Fix the test case so that we can fix the algorithm next.
Signed-off-by: Andrea Bolognani
---
tests/qemuxml2argvdata/mach-virt-console-virtio.args | 2 +-
tests/qemuxml2argvtest.c | 3 ++-
tests/qemuxml2xmloutdata/mach-virt-console-virtio.xml | 4 +++-
tests/qemuxml2xmltest.c | 6 --
4 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/tests/qemuxml2argvdata/mach-virt-console-virtio.args
b/tests/qemuxml2argvdata/mach-virt-console-virtio.args
index d76c5892aa..f4a43ff7c6 100644
--- a/tests/qemuxml2argvdata/mach-virt-console-virtio.args
+++ b/tests/qemuxml2argvdata/mach-virt-console-virtio.args
@@ -20,6 +20,6 @@ server,nowait \
-rtc base=utc \
-no-shutdown \
-no-acpi \
--device virtio-serial,id=virtio-serial0 \
+-device virtio-serial-device,id=virtio-serial0 \
-chardev pty,id=charconsole0 \
-device virtconsole,chardev=charconsole0,id=console0
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 9bfe864597..144e595310 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1976,7 +1976,8 @@ mymain(void)
QEMU_CAPS_DEVICE_USB_SERIAL);
DO_TEST("mach-virt-console-native",
QEMU_CAPS_DEVICE_PL011);
-DO_TEST("mach-virt-console-virtio", NONE);
+DO_TEST("mach-virt-console-virtio",
+QEMU_CAPS_DEVICE_VIRTIO_MMIO);
DO_TEST_PARSE_ERROR("mach-virt-serial-invalid-machine", NONE);
DO_TEST("disk-ide-split",
diff --git a/tests/qemuxml2xmloutdata/mach-virt-console-virtio.xml
b/tests/qemuxml2xmloutdata/mach-virt-console-virtio.xml
index 3e46cd2012..84e5c37ad9 100644
--- a/tests/qemuxml2xmloutdata/mach-virt-console-virtio.xml
+++ b/tests/qemuxml2xmloutdata/mach-virt-console-virtio.xml
@@ -18,7 +18,9 @@
/usr/bin/qemu-system-aarch64
-
+
+
+
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 43fd4e5f0f..47da7a7201 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -644,8 +644,10 @@ mymain(void)
QEMU_CAPS_DEVICE_PCIE_ROOT_PORT,
QEMU_CAPS_DEVICE_QEMU_XHCI,
QEMU_CAPS_DEVICE_USB_SERIAL);
-DO_TEST("mach-virt-console-native", NONE);
-DO_TEST("mach-virt-console-virtio", NONE);
+DO_TEST("mach-virt-console-native",
+QEMU_CAPS_DEVICE_PL011);
+DO_TEST("mach-virt-console-virtio",
+QEMU_CAPS_DEVICE_VIRTIO_MMIO);
DO_TEST("balloon-device-auto", NONE);
DO_TEST("balloon-device-period", NONE);
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] qemu: Unify generation of command line for virtio devices
-device virtio-blurb-pci Andrea Bolognani (2): tests: Fix use of virtio-serial for aarch64/virt qemu: Unify generation of command line for virtio devices src/qemu/qemu_command.c | 299 ++ .../mach-virt-console-virtio.args | 2 +- tests/qemuxml2argvtest.c | 3 +- .../mach-virt-console-virtio.xml | 4 +- tests/qemuxml2xmltest.c | 6 +- 5 files changed, 173 insertions(+), 141 deletions(-) -- 2.17.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/6] conf: Change return type of *AddressIsValid() to bool
These are simple predicates, which makes bool a more
appropriate return type than int.
Signed-off-by: Andrea Bolognani
---
src/conf/device_conf.c | 31 ---
src/conf/device_conf.h | 10 +-
2 files changed, 21 insertions(+), 20 deletions(-)
diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
index 1db92aba4a..1565d43fe6 100644
--- a/src/conf/device_conf.c
+++ b/src/conf/device_conf.c
@@ -160,8 +160,9 @@ virDomainDeviceInfoAddressIsEqual(const virDomainDeviceInfo
*a,
return true;
}
-int virPCIDeviceAddressIsValid(virPCIDeviceAddressPtr addr,
- bool report)
+bool
+virPCIDeviceAddressIsValid(virPCIDeviceAddressPtr addr,
+ bool report)
{
if (addr->domain > 0x) {
if (report)
@@ -169,7 +170,7 @@ int virPCIDeviceAddressIsValid(virPCIDeviceAddressPtr addr,
_("Invalid PCI address domain='0x%x', "
"must be <= 0x"),
addr->domain);
-return 0;
+return false;
}
if (addr->bus > 0xFF) {
if (report)
@@ -177,7 +178,7 @@ int virPCIDeviceAddressIsValid(virPCIDeviceAddressPtr addr,
_("Invalid PCI address bus='0x%x', "
"must be <= 0xFF"),
addr->bus);
-return 0;
+return false;
}
if (addr->slot > 0x1F) {
if (report)
@@ -185,7 +186,7 @@ int virPCIDeviceAddressIsValid(virPCIDeviceAddressPtr addr,
_("Invalid PCI address slot='0x%x', "
"must be <= 0x1F"),
addr->slot);
-return 0;
+return false;
}
if (addr->function > 7) {
if (report)
@@ -193,16 +194,16 @@ int virPCIDeviceAddressIsValid(virPCIDeviceAddressPtr
addr,
_("Invalid PCI address function=0x%x, "
"must be <= 7"),
addr->function);
-return 0;
+return false;
}
if (virPCIDeviceAddressIsEmpty(addr)) {
if (report)
virReportError(VIR_ERR_XML_ERROR, "%s",
_("Invalid PCI address :00:00, at least "
"one of domain, bus, or slot must be > 0"));
-return 0;
+return false;
}
-return 1;
+return true;
}
@@ -321,7 +322,7 @@ virPCIDeviceAddressEqual(virPCIDeviceAddress *addr1,
return false;
}
-int
+bool
virDomainDeviceCCWAddressIsValid(virDomainDeviceCCWAddressPtr addr)
{
return addr->cssid <= VIR_DOMAIN_DEVICE_CCW_MAX_CSSID &&
@@ -329,32 +330,32 @@
virDomainDeviceCCWAddressIsValid(virDomainDeviceCCWAddressPtr addr)
addr->devno <= VIR_DOMAIN_DEVICE_CCW_MAX_DEVNO;
}
-int
+bool
virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info,
int type)
{
if (info->type != type)
-return 0;
+return false;
switch (info->type) {
case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI:
return virPCIDeviceAddressIsValid(>addr.pci, false);
case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE:
-return 1;
+return true;
case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390:
case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO:
-return 1;
+return true;
case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW:
return virDomainDeviceCCWAddressIsValid(>addr.ccw);
case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB:
-return 1;
+return true;
}
-return 0;
+return false;
}
int
diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
index 4a50c3183e..b09d6bcecb 100644
--- a/src/conf/device_conf.h
+++ b/src/conf/device_conf.h
@@ -188,11 +188,11 @@ bool virDomainDeviceInfoAddressIsEqual(const
virDomainDeviceInfo *a,
const virDomainDeviceInfo *b)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
-int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info,
- int type);
+bool virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info,
+ int type);
-int virPCIDeviceAddressIsValid(virPCIDeviceAddressPtr addr,
- bool report);
+bool virPCIDeviceAddressIsValid(virPCIDeviceAddressPtr addr,
+bool report);
bool virPCIDeviceAddressIsEmpty(const virPCIDeviceAddress *addr);
bool virDeviceInfoPCIAddressIsWanted(const virDomainDeviceInfo *info);
@@ -208,7 +208,7 @@ int virPCIDeviceAddressFormat(virBufferPtr buf,
bool virPCIDeviceAddressEqual(virPCIDeviceAddress *addr1,
virPCIDeviceAddress *addr2);
-int virDomainDeviceCCWAddressIsValid(virDomainDeviceCCWAddressPtr addr);
+bool virDomainDeviceCCWAddressIsValid(virDomainDeviceCCWAddressPtr addr);
[libvirt] [PATCH 1/6] conf: Move virDomainDeviceAddressType to device_conf
It's used in virDomainDeviceInfo, which makes
domain_conf the wrong place to declare it.
Signed-off-by: Andrea Bolognani
---
src/conf/device_conf.c | 15 +++
src/conf/device_conf.h | 2 ++
src/conf/domain_conf.c | 14 --
src/conf/domain_conf.h | 1 -
src/libvirt_private.syms | 2 +-
5 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
index 7a8f84e036..dd381f303e 100644
--- a/src/conf/device_conf.c
+++ b/src/conf/device_conf.c
@@ -32,6 +32,21 @@
#define VIR_FROM_THIS VIR_FROM_DEVICE
+VIR_ENUM_IMPL(virDomainDeviceAddress, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST,
+ "none",
+ "pci",
+ "drive",
+ "virtio-serial",
+ "ccid",
+ "usb",
+ "spapr-vio",
+ "virtio-s390",
+ "ccw",
+ "virtio-mmio",
+ "isa",
+ "dimm",
+);
+
int
virDomainDeviceInfoCopy(virDomainDeviceInfoPtr dst,
virDomainDeviceInfoPtr src)
diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
index ff7d6c9d5f..66a999760c 100644
--- a/src/conf/device_conf.h
+++ b/src/conf/device_conf.h
@@ -51,6 +51,8 @@ typedef enum {
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST
} virDomainDeviceAddressType;
+VIR_ENUM_DECL(virDomainDeviceAddress);
+
typedef struct _virDomainDeviceDriveAddress virDomainDeviceDriveAddress;
typedef virDomainDeviceDriveAddress *virDomainDeviceDriveAddressPtr;
struct _virDomainDeviceDriveAddress {
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 38cac07913..2a22978855 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -263,20 +263,6 @@ VIR_ENUM_IMPL(virDomainDevice, VIR_DOMAIN_DEVICE_LAST,
"iommu",
"vsock")
-VIR_ENUM_IMPL(virDomainDeviceAddress, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST,
- "none",
- "pci",
- "drive",
- "virtio-serial",
- "ccid",
- "usb",
- "spapr-vio",
- "virtio-s390",
- "ccw",
- "virtio-mmio",
- "isa",
- "dimm")
-
VIR_ENUM_IMPL(virDomainDiskDevice, VIR_DOMAIN_DISK_DEVICE_LAST,
"disk",
"cdrom",
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 8a3673361a..2a827a093f 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3380,7 +3380,6 @@ VIR_ENUM_DECL(virDomainCapsFeature)
VIR_ENUM_DECL(virDomainLifecycle)
VIR_ENUM_DECL(virDomainLifecycleAction)
VIR_ENUM_DECL(virDomainDevice)
-VIR_ENUM_DECL(virDomainDeviceAddress)
VIR_ENUM_DECL(virDomainDiskDevice)
VIR_ENUM_DECL(virDomainDiskGeometryTrans)
VIR_ENUM_DECL(virDomainDiskBus)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 954ab4b66c..ae0c26ba99 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -95,6 +95,7 @@ virCPUModeTypeToString;
# conf/device_conf.h
virDeviceInfoPCIAddressIsPresent;
virDeviceInfoPCIAddressIsWanted;
+virDomainDeviceAddressTypeToString;
virDomainDeviceInfoAddressIsEqual;
virDomainDeviceInfoCopy;
virInterfaceLinkFormat;
@@ -291,7 +292,6 @@ virDomainDefValidate;
virDomainDefVcpuOrderClear;
virDomainDeleteConfig;
virDomainDeviceAddressIsValid;
-virDomainDeviceAddressTypeToString;
virDomainDeviceAliasIsUserAlias;
virDomainDeviceDefCopy;
virDomainDeviceDefFree;
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 5/6] conf: Rename virDomainPCIAddressAsString()
The struct is called virPCIDeviceAddress and the
functions operating on it should be named accordingly.
Signed-off-by: Andrea Bolognani
---
src/conf/device_conf.c | 2 +-
src/conf/device_conf.h | 2 +-
src/conf/domain_addr.c | 6 +++---
src/libvirt_private.syms | 2 +-
src/qemu/qemu_command.c| 2 +-
src/qemu/qemu_domain_address.c | 6 +++---
6 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
index afa06c3cda..48788540d3 100644
--- a/src/conf/device_conf.c
+++ b/src/conf/device_conf.c
@@ -310,7 +310,7 @@ virPCIDeviceAddressFormat(virBufferPtr buf,
}
char *
-virDomainPCIAddressAsString(virPCIDeviceAddressPtr addr)
+virPCIDeviceAddressAsString(virPCIDeviceAddressPtr addr)
{
char *str;
diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
index 846d12543d..c23a5918b3 100644
--- a/src/conf/device_conf.h
+++ b/src/conf/device_conf.h
@@ -205,7 +205,7 @@ int virPCIDeviceAddressFormat(virBufferPtr buf,
virPCIDeviceAddress addr,
bool includeTypeInAddr);
-char *virDomainPCIAddressAsString(virPCIDeviceAddressPtr addr)
+char *virPCIDeviceAddressAsString(virPCIDeviceAddressPtr addr)
ATTRIBUTE_NONNULL(1);
bool virPCIDeviceAddressEqual(virPCIDeviceAddress *addr1,
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index d2e1142462..442e6aab94 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -604,7 +604,7 @@
virDomainPCIAddressReserveAddrInternal(virDomainPCIAddressSetPtr addrs,
virErrorNumber errType = (fromConfig
? VIR_ERR_XML_ERROR : VIR_ERR_INTERNAL_ERROR);
-if (!(addrStr = virDomainPCIAddressAsString(addr)))
+if (!(addrStr = virPCIDeviceAddressAsString(addr)))
goto cleanup;
/* Add an extra bus if necessary */
@@ -689,7 +689,7 @@ virDomainPCIAddressEnsureAddr(virDomainPCIAddressSetPtr
addrs,
if (!flags)
return 0;
-if (!(addrStr = virDomainPCIAddressAsString(>addr.pci)))
+if (!(addrStr = virPCIDeviceAddressAsString(>addr.pci)))
goto cleanup;
if (virDeviceInfoPCIAddressIsPresent(dev)) {
@@ -770,7 +770,7 @@
virDomainPCIAddressFindUnusedFunctionOnBus(virDomainPCIAddressBusPtr bus,
*found = false;
-if (!(addrStr = virDomainPCIAddressAsString(searchAddr)))
+if (!(addrStr = virPCIDeviceAddressAsString(searchAddr)))
goto cleanup;
if (!virDomainPCIAddressFlagsCompatible(searchAddr, addrStr, bus->flags,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 77f55b3c40..44bf54da81 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -100,9 +100,9 @@ virDomainDeviceAddressTypeToString;
virDomainDeviceCCWAddressIsValid;
virDomainDeviceInfoAddressIsEqual;
virDomainDeviceInfoCopy;
-virDomainPCIAddressAsString;
virInterfaceLinkFormat;
virInterfaceLinkParseXML;
+virPCIDeviceAddressAsString;
virPCIDeviceAddressEqual;
virPCIDeviceAddressFormat;
virPCIDeviceAddressIsEmpty;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 8aa20496bc..b72506f4e1 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -301,7 +301,7 @@ qemuBuildDeviceAddressStr(virBufferPtr buf,
if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
size_t i;
-if (!(devStr = virDomainPCIAddressAsString(>addr.pci)))
+if (!(devStr = virPCIDeviceAddressAsString(>addr.pci)))
goto cleanup;
for (i = 0; i < domainDef->ncontrollers; i++) {
virDomainControllerDefPtr cont = domainDef->controllers[i];
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index dda14adeb3..24fdf12128 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -1306,7 +1306,7 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def
ATTRIBUTE_UNUSED,
* inappropriate address types.
*/
if (!info->pciConnectFlags) {
-char *addrStr = virDomainPCIAddressAsString(>addr.pci);
+char *addrStr = virPCIDeviceAddressAsString(>addr.pci);
VIR_WARN("qemuDomainDeviceCalculatePCIConnectFlags() thinks that the "
"device with PCI address %s should not have a PCI address",
@@ -1554,7 +1554,7 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def,
memset(_addr, 0, sizeof(tmp_addr));
tmp_addr.slot = 2;
-if (!(addrStr = virDomainPCIAddressAsString(_addr)))
+if (!(addrStr = virPCIDeviceAddressAsString(_addr)))
goto cleanup;
if (!virDomainPCIAddressValidate(addrs, _addr,
addrStr, flags, true))
@@ -1743,7 +1743,7 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def,
memset(_addr, 0, sizeof(tmp_addr));
tmp_addr.slot = 1;
-if (!(addrStr =
[libvirt] [PATCH 4/6] conf: Move virDomainPCIAddressAsString() to device_conf
It's a better fit than domain_conf.
Unfortunately, even after this change functions
handling virPCIDeviceAddress are split pretty much
evenly between conf/device_conf and utils/virpci:
ideally everything would be moved to the former,
including the struct declaration itself, and all the
names would be changed to be consistent with the rest
of the virDomainDevice*Address, but that's a cleanup
for another day.
Signed-off-by: Andrea Bolognani
---
src/conf/device_conf.c | 13 +
src/conf/device_conf.h | 3 +++
src/conf/domain_addr.c | 14 --
src/conf/domain_addr.h | 3 ---
src/libvirt_private.syms | 2 +-
5 files changed, 17 insertions(+), 18 deletions(-)
diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
index 1565d43fe6..afa06c3cda 100644
--- a/src/conf/device_conf.c
+++ b/src/conf/device_conf.c
@@ -309,6 +309,19 @@ virPCIDeviceAddressFormat(virBufferPtr buf,
return 0;
}
+char *
+virDomainPCIAddressAsString(virPCIDeviceAddressPtr addr)
+{
+char *str;
+
+ignore_value(virAsprintf(, "%.4x:%.2x:%.2x.%.1x",
+ addr->domain,
+ addr->bus,
+ addr->slot,
+ addr->function));
+return str;
+}
+
bool
virPCIDeviceAddressEqual(virPCIDeviceAddress *addr1,
virPCIDeviceAddress *addr2)
diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
index b09d6bcecb..846d12543d 100644
--- a/src/conf/device_conf.h
+++ b/src/conf/device_conf.h
@@ -205,6 +205,9 @@ int virPCIDeviceAddressFormat(virBufferPtr buf,
virPCIDeviceAddress addr,
bool includeTypeInAddr);
+char *virDomainPCIAddressAsString(virPCIDeviceAddressPtr addr)
+ ATTRIBUTE_NONNULL(1);
+
bool virPCIDeviceAddressEqual(virPCIDeviceAddress *addr1,
virPCIDeviceAddress *addr2);
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index b62fd53c66..d2e1142462 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -573,20 +573,6 @@ virDomainPCIAddressSetGrow(virDomainPCIAddressSetPtr addrs,
}
-char *
-virDomainPCIAddressAsString(virPCIDeviceAddressPtr addr)
-{
-char *str;
-
-ignore_value(virAsprintf(, "%.4x:%.2x:%.2x.%.1x",
- addr->domain,
- addr->bus,
- addr->slot,
- addr->function));
-return str;
-}
-
-
/*
* Check if the PCI slot is used by another device.
*/
diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
index 5ad9d8ef3d..2a9af9c00b 100644
--- a/src/conf/domain_addr.h
+++ b/src/conf/domain_addr.h
@@ -124,9 +124,6 @@ struct _virDomainPCIAddressSet {
typedef struct _virDomainPCIAddressSet virDomainPCIAddressSet;
typedef virDomainPCIAddressSet *virDomainPCIAddressSetPtr;
-char *virDomainPCIAddressAsString(virPCIDeviceAddressPtr addr)
- ATTRIBUTE_NONNULL(1);
-
virDomainPCIAddressSetPtr virDomainPCIAddressSetAlloc(unsigned int nbuses);
void virDomainPCIAddressSetFree(virDomainPCIAddressSetPtr addrs);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 78307885dc..77f55b3c40 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -100,6 +100,7 @@ virDomainDeviceAddressTypeToString;
virDomainDeviceCCWAddressIsValid;
virDomainDeviceInfoAddressIsEqual;
virDomainDeviceInfoCopy;
+virDomainPCIAddressAsString;
virInterfaceLinkFormat;
virInterfaceLinkParseXML;
virPCIDeviceAddressEqual;
@@ -113,7 +114,6 @@ virPCIDeviceAddressParseXML;
virDomainCCWAddressAssign;
virDomainCCWAddressSetCreateFromDomain;
virDomainCCWAddressSetFree;
-virDomainPCIAddressAsString;
virDomainPCIAddressBusIsFullyReserved;
virDomainPCIAddressBusSetModel;
virDomainPCIAddressEnsureAddr;
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/6] conf: Move virDomainDeviceAddressIsValid() to device_conf
The function is called on a virDomainDeviceInfo, so it
should be declared along with it.
Moving this function requires moving and making public
virDomainDeviceCCWAddressIsValid() as well, but that's
perfectly fine since the same reasoning above also
applies to it, due to virDomainDeviceCCWAddress being
(correctly) declared in device_conf.
Signed-off-by: Andrea Bolognani
---
src/conf/device_conf.c | 36
src/conf/device_conf.h | 5 +
src/conf/domain_conf.c | 36
src/conf/domain_conf.h | 2 --
src/libvirt_private.syms | 3 ++-
5 files changed, 43 insertions(+), 39 deletions(-)
diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
index dd381f303e..1db92aba4a 100644
--- a/src/conf/device_conf.c
+++ b/src/conf/device_conf.c
@@ -321,6 +321,42 @@ virPCIDeviceAddressEqual(virPCIDeviceAddress *addr1,
return false;
}
+int
+virDomainDeviceCCWAddressIsValid(virDomainDeviceCCWAddressPtr addr)
+{
+return addr->cssid <= VIR_DOMAIN_DEVICE_CCW_MAX_CSSID &&
+ addr->ssid <= VIR_DOMAIN_DEVICE_CCW_MAX_SSID &&
+ addr->devno <= VIR_DOMAIN_DEVICE_CCW_MAX_DEVNO;
+}
+
+int
+virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info,
+ int type)
+{
+if (info->type != type)
+return 0;
+
+switch (info->type) {
+case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI:
+return virPCIDeviceAddressIsValid(>addr.pci, false);
+
+case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE:
+return 1;
+
+case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390:
+case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO:
+return 1;
+
+case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW:
+return virDomainDeviceCCWAddressIsValid(>addr.ccw);
+
+case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB:
+return 1;
+}
+
+return 0;
+}
+
int
virInterfaceLinkParseXML(xmlNodePtr node,
virNetDevIfLinkPtr lnk)
diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
index 66a999760c..4a50c3183e 100644
--- a/src/conf/device_conf.h
+++ b/src/conf/device_conf.h
@@ -188,6 +188,9 @@ bool virDomainDeviceInfoAddressIsEqual(const
virDomainDeviceInfo *a,
const virDomainDeviceInfo *b)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
+int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info,
+ int type);
+
int virPCIDeviceAddressIsValid(virPCIDeviceAddressPtr addr,
bool report);
bool virPCIDeviceAddressIsEmpty(const virPCIDeviceAddress *addr);
@@ -205,6 +208,8 @@ int virPCIDeviceAddressFormat(virBufferPtr buf,
bool virPCIDeviceAddressEqual(virPCIDeviceAddress *addr1,
virPCIDeviceAddress *addr2);
+int virDomainDeviceCCWAddressIsValid(virDomainDeviceCCWAddressPtr addr);
+
int virInterfaceLinkParseXML(xmlNodePtr node,
virNetDevIfLinkPtr lnk);
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 2a22978855..3cae6b2aeb 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3586,42 +3586,6 @@ virDomainObjGetOneDef(virDomainObjPtr vm,
return virDomainObjGetOneDefState(vm, flags, NULL);
}
-
-static int
-virDomainDeviceCCWAddressIsValid(virDomainDeviceCCWAddressPtr addr)
-{
-return addr->cssid <= VIR_DOMAIN_DEVICE_CCW_MAX_CSSID &&
-addr->ssid <= VIR_DOMAIN_DEVICE_CCW_MAX_SSID &&
-addr->devno <= VIR_DOMAIN_DEVICE_CCW_MAX_DEVNO;
-}
-
-int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info,
- int type)
-{
-if (info->type != type)
-return 0;
-
-switch (info->type) {
-case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI:
-return virPCIDeviceAddressIsValid(>addr.pci, false);
-
-case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE:
-return 1;
-
-case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390:
-case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO:
-return 1;
-
-case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW:
-return virDomainDeviceCCWAddressIsValid(>addr.ccw);
-
-case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB:
-return 1;
-}
-
-return 0;
-}
-
virDomainDeviceInfoPtr
virDomainDeviceGetInfo(virDomainDeviceDefPtr device)
{
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 2a827a093f..3ff6eea117 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2911,8 +2911,6 @@ virDomainDeviceDefPtr
virDomainDeviceDefCopy(virDomainDeviceDefPtr src,
const virDomainDef *def,
virCapsPtr caps,
virDomainXMLOptionPtr xmlopt);
-int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info,
- int type);
virDomainDeviceInfoPtr
[libvirt] [PATCH 6/6] conf: Move *AddressParseXML() to device_conf
The corresponding structs are declared there.
Signed-off-by: Andrea Bolognani
---
src/conf/device_conf.c | 267 ++
src/conf/device_conf.h | 17 +++
src/conf/domain_conf.c | 270 ---
src/libvirt_private.syms | 6 +
4 files changed, 290 insertions(+), 270 deletions(-)
diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
index 48788540d3..18592bbc1d 100644
--- a/src/conf/device_conf.c
+++ b/src/conf/device_conf.c
@@ -343,6 +343,273 @@
virDomainDeviceCCWAddressIsValid(virDomainDeviceCCWAddressPtr addr)
addr->devno <= VIR_DOMAIN_DEVICE_CCW_MAX_DEVNO;
}
+int
+virDomainDeviceCCWAddressParseXML(xmlNodePtr node,
+ virDomainDeviceCCWAddressPtr addr)
+{
+int ret = -1;
+char *cssid;
+char *ssid;
+char *devno;
+
+memset(addr, 0, sizeof(*addr));
+
+cssid = virXMLPropString(node, "cssid");
+ssid = virXMLPropString(node, "ssid");
+devno = virXMLPropString(node, "devno");
+
+if (cssid && ssid && devno) {
+if (cssid &&
+virStrToLong_uip(cssid, NULL, 0, >cssid) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Cannot parse 'cssid' attribute"));
+goto cleanup;
+}
+if (ssid &&
+virStrToLong_uip(ssid, NULL, 0, >ssid) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Cannot parse 'ssid' attribute"));
+goto cleanup;
+}
+if (devno &&
+virStrToLong_uip(devno, NULL, 0, >devno) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Cannot parse 'devno' attribute"));
+goto cleanup;
+}
+if (!virDomainDeviceCCWAddressIsValid(addr)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Invalid specification for virtio ccw"
+ " address: cssid='%s' ssid='%s' devno='%s'"),
+ cssid, ssid, devno);
+goto cleanup;
+}
+addr->assigned = true;
+} else if (cssid || ssid || devno) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Invalid partial specification for virtio ccw"
+ " address"));
+goto cleanup;
+}
+
+ret = 0;
+
+ cleanup:
+VIR_FREE(cssid);
+VIR_FREE(ssid);
+VIR_FREE(devno);
+return ret;
+}
+
+int
+virDomainDeviceDriveAddressParseXML(xmlNodePtr node,
+virDomainDeviceDriveAddressPtr addr)
+{
+char *bus, *unit, *controller, *target;
+int ret = -1;
+
+memset(addr, 0, sizeof(*addr));
+
+controller = virXMLPropString(node, "controller");
+bus = virXMLPropString(node, "bus");
+target = virXMLPropString(node, "target");
+unit = virXMLPropString(node, "unit");
+
+if (controller &&
+virStrToLong_uip(controller, NULL, 10, >controller) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Cannot parse 'controller' attribute"));
+goto cleanup;
+}
+
+if (bus &&
+virStrToLong_uip(bus, NULL, 10, >bus) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Cannot parse 'bus' attribute"));
+goto cleanup;
+}
+
+if (target &&
+virStrToLong_uip(target, NULL, 10, >target) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Cannot parse 'target' attribute"));
+goto cleanup;
+}
+
+if (unit &&
+virStrToLong_uip(unit, NULL, 10, >unit) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Cannot parse 'unit' attribute"));
+goto cleanup;
+}
+
+ret = 0;
+
+ cleanup:
+VIR_FREE(controller);
+VIR_FREE(bus);
+VIR_FREE(target);
+VIR_FREE(unit);
+return ret;
+}
+
+int
+virDomainDeviceVirtioSerialAddressParseXML(xmlNodePtr node,
+
virDomainDeviceVirtioSerialAddressPtr addr)
+{
+char *controller, *bus, *port;
+int ret = -1;
+
+memset(addr, 0, sizeof(*addr));
+
+controller = virXMLPropString(node, "controller");
+bus = virXMLPropString(node, "bus");
+port = virXMLPropString(node, "port");
+
+if (controller &&
+virStrToLong_uip(controller, NULL, 10, >controller) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Cannot parse 'controller' attribute"));
+goto cleanup;
+}
+
+if (bus &&
+virStrToLong_uip(bus, NULL, 10, >bus) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Cannot parse 'bus' attribute"));
+goto cleanup;
+}
+
+if (port &&
+virStrToLong_uip(port, NULL, 10, >port) < 0) {
+
[libvirt] [PATCH 0/6] conf: Move a bunch of stuff to device_conf
These are the times where I wish we were using a language with support for proper classes and modules like, I dunno, PHP or something. Andrea Bolognani (6): conf: Move virDomainDeviceAddressType to device_conf conf: Move virDomainDeviceAddressIsValid() to device_conf conf: Change return type of *AddressIsValid() to bool conf: Move virDomainPCIAddressAsString() to device_conf conf: Rename virDomainPCIAddressAsString() conf: Move *AddressParseXML() to device_conf src/conf/device_conf.c | 348 - src/conf/device_conf.h | 31 ++- src/conf/domain_addr.c | 20 +- src/conf/domain_addr.h | 3 - src/conf/domain_conf.c | 320 -- src/conf/domain_conf.h | 3 - src/libvirt_private.syms | 13 +- src/qemu/qemu_command.c| 2 +- src/qemu/qemu_domain_address.c | 6 +- 9 files changed, 386 insertions(+), 360 deletions(-) -- 2.17.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 19/28] qemu_conf: Introduce metadata_lock_manager
On 08/27/2018 04:08 AM, Michal Privoznik wrote: > This config option allows users to set and enable lock manager > for domain metadata. The lock manager is going to be used by > security drivers to serialize each other when changing a file > ownership or changing the SELinux label. The only supported lock > manager is 'lockd' for now. > > Signed-off-by: Michal Privoznik > --- > src/qemu/libvirtd_qemu.aug | 1 + > src/qemu/qemu.conf | 6 ++ > src/qemu/qemu_conf.c | 13 + > src/qemu/qemu_conf.h | 1 + > src/qemu/test_libvirtd_qemu.aug.in | 1 + > 5 files changed, 22 insertions(+) > > diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug > index ddc4bbfd1d..42e325d4fb 100644 > --- a/src/qemu/libvirtd_qemu.aug > +++ b/src/qemu/libvirtd_qemu.aug > @@ -98,6 +98,7 @@ module Libvirtd_qemu = > | bool_entry "relaxed_acs_check" > | bool_entry "allow_disk_format_probing" > | str_entry "lock_manager" > + | str_entry "metadata_lock_manager" > > let rpc_entry = int_entry "max_queued" > | int_entry "keepalive_interval" > diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf > index cd57b3cc69..06caa39232 100644 > --- a/src/qemu/qemu.conf > +++ b/src/qemu/qemu.conf > @@ -659,6 +659,12 @@ > #lock_manager = "lockd" > > > +# To serialize two daemons trying to change metadata on a file, Just two? ;-) > +# libvirt offers a locking mechanism. Currently, only "lockd" is > +# supported (or no locking at all if unset). > +# > +#metadata_lock_manager = "lockd" > + Should we state that the domain locking is independent of the metadata daemon locking? I know it's obvious to the author (and now the reviewer), but for the first time reader of the config file. And of course that leaves the question on the table for the consumer about what is meant by multiple daemons. As in, which daemons... That little factoid would be lost in the commit message. Assuming some beefed up text, Reviewed-by: John Ferlan John [...] -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 01/10] docs: don't refer to deprecated 'linux' ostype in example
On Thu, Aug 30, 2018 at 03:29:41PM +0100, Daniel P. Berrangé wrote: > On Thu, Aug 30, 2018 at 04:27:06PM +0200, Marek Marczykowski-Górecki wrote: > > On Mon, Aug 27, 2018 at 03:23:16PM -0600, Jim Fehlig wrote: > > > On 08/05/2018 03:48 PM, Marek Marczykowski-Górecki wrote: > > > > Use preferred name: 'xen'. > > > > > > I'd be fine with this change if the actual code used the preferred name > > > too > > > :-). E.g. config containing > > > > > > xen > > > > > > will be shown as > > > > > > linux > > > > > > after virsh define; virsh dumpxml. Also, virsh domxml-from-native will > > > produce the linux variant. > > > > I was thinking about this and decided to left it intact for backward > > compatibility - so config from `virsh dumpxml` will work with very old > > libvirt version. But if that would be acceptable, I'd very much like to > > change that too. What are opinions about that? > > We can't change what we are reporting in the XML here, no matter how > stupid our original choice of terminology was. Ok. What about other patches (not really depending on this one)? -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 18/28] lock_manager: Allow disabling configFile for virLockManagerPluginNew
On 08/27/2018 04:08 AM, Michal Privoznik wrote:
> In some cases we might want to not load the lock driver config.
> Alter virLockManagerPluginNew() and the lock drivers to cope with
> this fact.
No current cases, but sometime in the future the requirement that a
configFile exists will be removed Waiting with baited fingers to see
how it's replaced ;-)
>
> Signed-off-by: Michal Privoznik
> ---
> src/locking/lock_driver_lockd.c | 4 +++-
> src/locking/lock_driver_sanlock.c | 4 +++-
> src/locking/lock_manager.c| 8 ++--
> 3 files changed, 12 insertions(+), 4 deletions(-)
>
I note that virLockDriverInit in src/locking/lock_driver.h doesn't even
document @configFile (or seem to care if it was NULL). In any a
modification there to describe the argument should be added.
> diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c
> index aec768b0df..c0598e6987 100644
> --- a/src/locking/lock_driver_lockd.c
> +++ b/src/locking/lock_driver_lockd.c
> @@ -370,8 +370,10 @@ static int virLockManagerLockDaemonInit(unsigned int
> version,
> driver->requireLeaseForDisks = true;
> driver->autoDiskLease = true;
>
> -if (virLockManagerLockDaemonLoadConfig(configFile) < 0)
> +if (configFile &&
> +virLockManagerLockDaemonLoadConfig(configFile) < 0) {
> goto error;
> +}
>
> if (driver->autoDiskLease) {
> if (driver->fileLockSpaceDir &&
> diff --git a/src/locking/lock_driver_sanlock.c
> b/src/locking/lock_driver_sanlock.c
> index 9393e7d9a2..66953c70d5 100644
> --- a/src/locking/lock_driver_sanlock.c
> +++ b/src/locking/lock_driver_sanlock.c
> @@ -450,8 +450,10 @@ static int virLockManagerSanlockInit(unsigned int
> version,
> goto error;
> }
>
> -if (virLockManagerSanlockLoadConfig(driver, configFile) < 0)
> +if (configFile &&
> +virLockManagerSanlockLoadConfig(driver, configFile) < 0) {
> goto error;
> +}
>
> if (driver->autoDiskLease && !driver->hostID) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> diff --git a/src/locking/lock_manager.c b/src/locking/lock_manager.c
> index 30a0fd996e..c2ff7afb70 100644
> --- a/src/locking/lock_manager.c
> +++ b/src/locking/lock_manager.c
> @@ -105,6 +105,8 @@ static void virLockManagerLogParams(size_t nparams,
> /**
> * virLockManagerPluginNew:
> * @name: the name of the plugin
> + * @driverName: the hypervisor driver that loads the plugin
> + * @configDir: path to dir where config files are stored
> * @flag: optional plugin flags
> *
> * Attempt to load the plugin $(libdir)/libvirt/lock-driver/@name.so
> @@ -132,9 +134,11 @@ virLockManagerPluginPtr virLockManagerPluginNew(const
> char *name,
> VIR_DEBUG("name=%s driverName=%s configDir=%s flags=0x%x",
>name, driverName, configDir, flags);
You'll need to add NULLSTR around @driverName and @configDir then,
right? If they can be NULL?
With those adjustments,
Reviewed-by: John Ferlan
John
>
> -if (virAsprintf(, "%s/%s-%s.conf",
> -configDir, driverName, name) < 0)
> +if (driverName && configDir &&
> +virAsprintf(, "%s/%s-%s.conf",
> +configDir, driverName, name) < 0) {
> return NULL;
> +}
>
> if (STREQ(name, "nop")) {
> driver =
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 00/28] Introduce metadata locking
On Mon, Aug 27, 2018 at 10:08:13AM +0200, Michal Privoznik wrote: > v3 of: > > https://www.redhat.com/archives/libvir-list/2018-August/msg00814.html > > What has changed since v2? A lot. > - The lock manager was moved into security manager (which requires a lot > of preparation which is done in first 8 or so patches). > > - The VIR_LOCK_SPACE_ACQUIRE_WAIT flag (2/7 in v2) is dropped as it > turned out to be harmful. virlockd can't block under any > circumstances. And we can not introduce a thread pool for it. > > - While going through the code I've found couple of bugs which I'm > fixing in first few patches. I've not done a detailed per patch code review, but having looked at the overall design concept across the patches, I think it looks pretty good. Only one conceptual comment > cfg.mk | 4 +- > src/libvirt_private.syms | 2 + > src/locking/lock_daemon.c | 3 + > src/locking/lock_daemon_dispatch.c | 25 +- > src/locking/lock_driver.h | 38 +++ > src/locking/lock_driver_lockd.c| 520 > ++--- > src/locking/lock_driver_lockd.h| 1 + > src/locking/lock_driver_nop.c | 14 + > src/locking/lock_driver_sanlock.c | 50 ++-- > src/locking/lock_manager.c | 31 ++- > src/locking/lock_manager.h | 7 + > src/qemu/libvirtd_qemu.aug | 1 + > src/qemu/qemu.conf | 6 + > src/qemu/qemu_conf.c | 13 + > src/qemu/qemu_conf.h | 1 + > src/qemu/qemu_driver.c | 12 +- > src/qemu/test_libvirtd_qemu.aug.in | 1 + > src/security/security_dac.c| 213 +-- > src/security/security_manager.c| 366 +- > src/security/security_manager.h| 17 +- Why no integration into the security_selinux.c driver ? The apparmor driver probably doesn't need it as it doesn't touchthe files to setup its security profile, but SELinux should need protection. > src/util/virlockspace.c| 15 +- > src/util/virlockspace.h| 4 + > tests/testutilsqemu.c | 2 +- > tests/virlockspacetest.c | 29 ++- > 24 files changed, 1096 insertions(+), 279 deletions(-) Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 17/28] lock_manager: Introduce virLockManagerCloseConn
On 08/27/2018 04:08 AM, Michal Privoznik wrote:
> After the previous commit we have VIR_LOCK_MANAGER_ACQUIRE_KEEP_OPEN
> flag. This is not enough because it will keep connection open for only
> one instance of drvAcquire + drvRelease call. And when starting up a
> domain there will be a lot of such calls as there will be a lot of paths
> to relabel and thus lock. Therfore, VIR_LOCK_MANAGER_RELEASE_KEEP_OPEN
> flag was introduced which allows us to keep connection open even after
s/was/will be/ (or is)
s/which allows us to keep/to allow keeping the/
> the drvAcquire + drvRelease pair. In order to close the connection after
> all locking has been done virLockManagerCloseConn is introduced.
>
> Signed-off-by: Michal Privoznik
> ---
> src/libvirt_private.syms| 1 +
> src/locking/lock_driver.h | 22 ++
> src/locking/lock_driver_lockd.c | 24
> src/locking/lock_driver_nop.c | 8
> src/locking/lock_manager.c | 11 +++
> src/locking/lock_manager.h | 4
> 6 files changed, 70 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 42f15f117e..bca5a51ba0 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1294,6 +1294,7 @@ virDomainLockProcessStart;
> virLockManagerAcquire;
> virLockManagerAddResource;
> virLockManagerClearResources;
> +virLockManagerCloseConn;
> virLockManagerFree;
> virLockManagerInquire;
> virLockManagerNew;
> diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h
> index 7e3ffc58b5..d81767707b 100644
> --- a/src/locking/lock_driver.h
> +++ b/src/locking/lock_driver.h
> @@ -282,6 +282,27 @@ typedef int (*virLockDriverRelease)(virLockManagerPtr
> man,
> char **state,
> unsigned int flags);
>
> +/**
> + * virLockDriverCloseConn:
> + * @man: the lock manager context
> + * @flags: optional flags, currently unused
> + *
> + * Close any connection that was saved via
> + * VIR_LOCK_MANAGER_ACQUIRE_KEEP_OPEN or
> + * VIR_LOCK_MANAGER_RELEASE_KEEP_OPEN flags.
Hmm, a client that forgets to provide KEEP_OPEN on one or the other
would wreak havoc with the algorithm. Upon further thought, the
KEEP_OPEN really only matters for Acquire and it should be saved in
@priv. That way the @priv would manage it not whether the flag was
provided on release.
> + * However, if there is still a resource locked, do not actually
> + * close the connection as it would result in killing the
> + * resource owner. This is similar to refcounting when all
> + * threads call virLockDriverCloseConn() but only the last one
> + * actually closes the connection.
> + *
NB: virObject{Ref|Unlock} uses atomic incr/decr for ref counting.
> + * Returns: 0 on success and connection not actually closed,
> + * 1 on success and connection closed,
> + * -1 otherwise
> + */
> +typedef int (*virLockDriverCloseConn)(virLockManagerPtr man,
> + unsigned int flags);
> +
> /**
> * virLockDriverInquire:
> * @manager: the lock manager context
> @@ -328,6 +349,7 @@ struct _virLockDriver {
>
> virLockDriverAcquire drvAcquire;
> virLockDriverRelease drvRelease;
> +virLockDriverCloseConn drvCloseConn;
> virLockDriverInquire drvInquire;
> };
>
> diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c
> index 14f9eae760..aec768b0df 100644
> --- a/src/locking/lock_driver_lockd.c
> +++ b/src/locking/lock_driver_lockd.c
> @@ -937,6 +937,28 @@ static int
> virLockManagerLockDaemonRelease(virLockManagerPtr lock,
> }
>
>
> +static int virLockManagerLockDaemonCloseConn(virLockManagerPtr lock,
> + unsigned int flags)
static int
virLock...
> +{
> +virLockManagerLockDaemonPrivatePtr priv = lock->privateData;
> +
> +virCheckFlags(0, -1);
> +
> +if (priv->clientRefs)
> +return 0;
> +
> +virNetClientClose(priv->client);
> +virObjectUnref(priv->client);
> +virObjectUnref(priv->program);
> +
> +priv->client = NULL;
> +priv->program = NULL;
> +priv->counter = 0;
> +
> +return 1;
The helper concept from the previous patch still could apply here. I
would say I'm surprised this did anything in testing since clientRefs
wouldn't be 0 if acquire, then release is called w/ the KEEP_OPEN flag.
It'd be -1, I believe.
The rest seems fine.
John
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: mdev: Set vfio device property 'display' to off only if mdev is vfio-pci
S390 is aware of both vfio-pci and vfio-ccw devices, so on S390 the capability QEMU_CAPS_VFIO_PCI_DISPLAY will be available. Add an extra check to make sure we only set the display to off for vfio-pci mediated devices. Otherwise we add display for vfio-ccw device and this breaks vfio-ccw device qemu command line. Fixes: d54e45b6e conf: Introduce new attribute 'display' Signed-off-by: Farhan Ali Reviewed-by: Marc Hartmayer --- src/qemu/qemu_domain.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 22c6d8f..df7e937 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6375,6 +6375,7 @@ qemuDomainHostdevDefMdevPostParse(virDomainHostdevSubsysMediatedDevPtr mdevsrc, /* QEMU 2.12 added support for vfio-pci display type, we default to * 'display=off' to stay safe from future changes */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY) && +mdevsrc->model == VIR_MDEV_MODEL_TYPE_VFIO_PCI && mdevsrc->display == VIR_TRISTATE_SWITCH_ABSENT) mdevsrc->display = VIR_TRISTATE_SWITCH_OFF; -- 2.7.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 16/28] lock_driver: Introduce KEEP_OPEN flags
On 08/27/2018 04:08 AM, Michal Privoznik wrote:
> This flag causes connection to be opened when needed (e.g. when
> calling virLockManagerLockDaemonAcquire for the first time) and
> instead of closing it at the end of such API store it in
> privateData so that it can be reused by later calls.
>
> This is needed because if a resource is acquired and connection
> is closed then virtlockd kills the registered PID (that's what
> virtlockd is designed to do). Therefore we will need the
> connection to open at drvAcquire and close not any sooner than
> drvRelease. However, as we will be locking files step-by-step we
> want to avoid opening new connection for every drvAcquire +
> drvRelease pair, so the connection is going to be shared even
> more than that. But more on that in next commit.
>
> Signed-off-by: Michal Privoznik
> ---
> src/locking/lock_driver.h | 7 +
> src/locking/lock_driver_lockd.c | 68
> +
> 2 files changed, 69 insertions(+), 6 deletions(-)
>
> diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h
> index 59c4c3aac7..7e3ffc58b5 100644
> --- a/src/locking/lock_driver.h
> +++ b/src/locking/lock_driver.h
> @@ -67,8 +67,15 @@ typedef enum {
> VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY = (1 << 0),
> /* Prevent further lock/unlock calls from this process */
> VIR_LOCK_MANAGER_ACQUIRE_RESTRICT = (1 << 1),
> +/* Causes driver to keep connection open and reuse it for further use. */
> +VIR_LOCK_MANAGER_ACQUIRE_KEEP_OPEN = (1 << 2),
> } virLockManagerAcquireFlags;
>
> +typedef enum {
> +/* Reuse previously saved connection. */
> +VIR_LOCK_MANAGER_RELEASE_KEEP_OPEN = (1 << 0),
> +} virLockManagerReleaseFlags;
> +
> typedef enum {
> /* virLockManagerNew called for a freshly started domain */
> VIR_LOCK_MANAGER_NEW_STARTED = (1 << 0),
> diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c
> index 4883e89ac6..14f9eae760 100644
> --- a/src/locking/lock_driver_lockd.c
> +++ b/src/locking/lock_driver_lockd.c
> @@ -76,6 +76,11 @@ struct _virLockManagerLockDaemonPrivate {
>
> size_t nresources;
> virLockManagerLockDaemonResourcePtr resources;
> +
> +int clientRefs;
> +virNetClientPtr client;
> +virNetClientProgramPtr program;
> +int counter;
> };
>
>
> @@ -440,6 +445,13 @@
> virLockManagerLockDaemonPrivateFree(virLockManagerLockDaemonPrivatePtr priv)
> default:
> break;
> }
> +
> +if (priv->client) {
> +virNetClientClose(priv->client);
> +virObjectUnref(priv->client);
> +virObjectUnref(priv->program);
> +}
> +
How about a helper method that would do these?
I wonder now if the @priv should keep track of flags as well?
That way you could "always" use @priv to store the client, program, and
counter and then use helper methods to determine at close whether @flags
had the KEEP_OPEN or not instead of having KEEP_OPEN in various places.
> VIR_FREE(priv);
> }
>
> @@ -770,7 +782,8 @@ static int
> virLockManagerLockDaemonAcquire(virLockManagerPtr lock,
> virLockManagerLockDaemonPrivatePtr priv = lock->privateData;
>
> virCheckFlags(VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY |
> - VIR_LOCK_MANAGER_ACQUIRE_RESTRICT, -1);
> + VIR_LOCK_MANAGER_ACQUIRE_RESTRICT |
> + VIR_LOCK_MANAGER_ACQUIRE_KEEP_OPEN, -1);
>
> if (priv->type == VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN &&
> priv->nresources == 0 &&
> @@ -781,7 +794,14 @@ static int
> virLockManagerLockDaemonAcquire(virLockManagerPtr lock,
> return -1;
> }
>
> -if (!(client = virLockManagerLockDaemonConnect(lock, ,
> )))
> +if (flags & VIR_LOCK_MANAGER_ACQUIRE_KEEP_OPEN) {
> +client = priv->client;
> +program = priv->program;
> +counter = priv->counter;
> +}
> +
> +if (!client &&
> +!(client = virLockManagerLockDaemonConnect(lock, ,
> )))
If "always" storing in @priv this alters to if !priv->client &&
!(priv->client ==...
> goto cleanup;
>
> if (fd &&
> @@ -814,11 +834,25 @@ static int
> virLockManagerLockDaemonAcquire(virLockManagerPtr lock,
> virLockManagerLockDaemonConnectionRestrict(lock, client, program,
> ) < 0)
> goto cleanup;
>
> +if (flags & VIR_LOCK_MANAGER_ACQUIRE_KEEP_OPEN) {
> +VIR_STEAL_PTR(priv->client, client);
> +VIR_STEAL_PTR(priv->program, program);
> +priv->counter = counter;
> +}
> +
If "always" using @priv means this isn't necessary as long as @flags is
also stored.
> rv = 0;
>
> cleanup:
> -if (rv != 0 && fd)
> -VIR_FORCE_CLOSE(*fd);
> +if (rv < 0) {
> +if (fd)
> +VIR_FORCE_CLOSE(*fd);
> +
> +priv->client = NULL;
> +priv->program = NULL;
> +priv->counter = 0;
> +priv->clientRefs = 0;
So one failure this time causes previously stored
Re: [libvirt] [PATCH] qemu: mdev: Set vfio device property 'display' to off only if mdev is vfio-pci
On Thu, Aug 30, 2018 at 01:07:34PM -0400, Farhan Ali wrote: > S390 is aware of both vfio-pci and vfio-ccw devices, so > on S390 the capability QEMU_CAPS_VFIO_PCI_DISPLAY will be > available. Add an extra check to make sure we only set the > display to off for vfio-pci mediated devices. Otherwise we > add display for vfio-ccw device and this breaks vfio-ccw > device qemu command line. > > Fixes: d54e45b6e conf: Introduce new attribute 'display' > Signed-off-by: Farhan Ali > Reviewed-by: Marc Hartmayer > --- > src/qemu/qemu_domain.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > index 22c6d8f..df7e937 100644 > --- a/src/qemu/qemu_domain.c > +++ b/src/qemu/qemu_domain.c > @@ -6375,6 +6375,7 @@ > qemuDomainHostdevDefMdevPostParse(virDomainHostdevSubsysMediatedDevPtr > mdevsrc, > /* QEMU 2.12 added support for vfio-pci display type, we default to > * 'display=off' to stay safe from future changes */ > if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY) && > +mdevsrc->model == VIR_MDEV_MODEL_TYPE_VFIO_PCI && > mdevsrc->display == VIR_TRISTATE_SWITCH_ABSENT) > mdevsrc->display = VIR_TRISTATE_SWITCH_OFF; Reviewed-by: Erik Skultety and safe for freeze... -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Add virNetlinkNewLink for simplifying virNetDev*Create
On Thu, Aug 30, 2018 at 02:27:05PM +0800, Shi Lei wrote:
> Thanks for your comments.
> But I have several questions, please see below...
>
> On 2018-08-29 at 19:43, Erik Skultety wrote:
> >On Thu, Aug 23, 2018 at 12:15:08PM +0800, Shi Lei wrote:
> >> This patch adds virNetlinkNewLink for simplifying virNetDevMacVLanCreate
> >> and virNetDevBridgeCreate.
> >>
> >> Signed-off-by: Shi Lei
> >> ---
> >
> >There are multiple changes happening in this patch so it will need to be
> >split
> >into several patches, see below...
>
> Okay. Then the v2 series should be like:
> [patch v2 0/4]: ... cover ...
> [patch v2 1/4]: Add wrapper macros to make code more readable
> [patch v2 2/4]: Introduce virNetlinkNewLink
2/4 would come before 1/4
> [patch v2 3/4]: Replace the implementation of virNetDevBridgeCreate
> [patch v2 4/4]: Replace the implementation of virNetDevMacVLanCreate
3/4 + 4/4 would be a single patch
...
> >> +static int
> >> +virNetDevMacVLanCreateCallback(struct nl_msg *nl_msg, const void *opaque)
> >> +{
> >> + const uint32_t *mode = (const uint32_t *) opaque;
> >> + if (!nl_msg || !opaque) {
> >> + virReportError(VIR_ERR_INTERNAL_ERROR,
> >> + _("nl_msg %p or opaque %p is NULL"),
> >> + nl_msg, opaque);
> >> + return -1;
> >> + }
> >> +
> >> + if (*mode > 0) {
> >> + struct nlattr *info_data;
> >> + if (!(info_data = nla_nest_start(nl_msg, IFLA_INFO_DATA)))
> >> + goto buffer_too_small;
> >> +
> >> + if (nla_put(nl_msg, IFLA_MACVLAN_MODE, sizeof(*mode), mode) < 0)
> >> + goto buffer_too_small;
> >> +
> >> + nla_nest_end(nl_msg, info_data);
> >> + }
> >> +
> >> + return 0;
> >> +
> >> + buffer_too_small:
> >> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> >> + _("allocated netlink buffer is too small"));
> >> + return -1;
> >> +}
> >
> >Having a callback just to add a nested data into nl_msg doesn't feel like the
> >right approach, I'd expect a callback to do more specific stuff, this should
> >be
> >special-cased in virNetlinkNewLink.
>
> I looked through the code for ip-link in iproute2 which could be a good
> example
> for netlink. And I found that the differences of attributes of various
> net-devices are
> confined to IFLA_INFO_DATA or IFLA_INFO_SLAVE_DATA(this is not used in
> libvirt).
> The code fragment in iplink.c:
>
> if (ulinep && !strcmp(ulinep, "_slave")) {
> ... ...
> lu = get_link_slave_kind(slavebuf);
> iflatype = IFLA_INFO_SLAVE_DATA;
> } else {
> => lu = get_link_kind(type); /* Get pointer to the driver
> for this type */
> => iflatype = IFLA_INFO_DATA; /* NEST will be for IFLA_INFO_DATA
> */
> }
> if (lu && argc) {
> => struct rtattr *data = addattr_nest(, sizeof(req), iflatype);
> /* NEST BEGIN */
>
>
> if (lu->parse_opt &&
> lu->parse_opt(lu, argc, argv, ))
> return -1;
>
> ^The lu points to various types of net-drivers(e.g.
> macvlan/bridge/vxlan/vlan ...)
> which pack all special self-attributes in their parse_opt
> callback
>
> => addattr_nest_end(, data); /* NEST END */
>
> So I think that it's reasonable for us to have a callback to handle it just
> as iproute2
> since iproute2 could almost create all the types of net-devices.
I went through the iproute2 code and it completely makes sense for them to
utilize callbacks here, given the amount of driver-specific options each of the
device type has. While I agree that we could make use of this in libvirt one
day, I don't feel like we need to do it now, since I don't think it brings
any noticeable benefit in its currently proposed form + as I already mentioned,
we can switch to this scheme once there are more than 1 netlink types which
could make use of it.
>
> >
> >> +
> >> +
> >> /**
> >> * virNetDevMacVLanCreate:
> >> *
> >> @@ -307,113 +338,29 @@ virNetDevMacVLanCreate(const char *ifname,
> >> uint32_t macvlan_mode,
> >> int *retry)
> >> {
> >
> >^This replacement would be the last patch in the series.
>
> Okay.
>
> >
> >> - int rc = -1;
> >> - struct nlmsgerr *err;
> >> - struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC };
> >> int ifindex;
> >> - unsigned int recvbuflen;
> >> - struct nl_msg *nl_msg;
> >> - struct nlattr *linkinfo, *info_data;
> >> - char macstr[VIR_MAC_STRING_BUFLEN];
> >> - VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
> >> -
> >> - if (virNetDevGetIndex(srcdev, ) < 0)
> >> - return -1;
> >> -
> >> + int error = 0;
> >> *retry = 0;
> >>
> >> - nl_msg = nlmsg_alloc_simple(RTM_NEWLINK,
> >> - NLM_F_REQUEST | NLM_F_CREATE |
> >>
Re: [libvirt] [PATCH] qga: ignore non present cpus when handling qmp_guest_get_vcpus()
On 08/31/18 10:00, Igor Mammedov wrote:
> On Thu, 30 Aug 2018 17:51:13 +0200
> Laszlo Ersek wrote:
>
>> +Drew
>>
>> On 08/30/18 14:08, Igor Mammedov wrote:
>>> If VM has VCPUs plugged sparselly (for example a VM started with
>>> 3 VCPUs (cpu0, cpu1 and cpu2) and then cpu1 was hotunplugged so
>>> only cpu0 and cpu2 are present), QGA will rise a error
>>> error: internal error: unable to execute QEMU agent command
>>> 'guest-get-vcpus':
>>> open("/sys/devices/system/cpu/cpu1/"): No such file or directory
>>> when
>>> virsh vcpucount FOO --guest
>>> is executed.
>>> Fix it by ignoring non present CPUs when fetching CPUs status from sysfs.
>>>
>>> Signed-off-by: Igor Mammedov
>>> ---
>>> qga/commands-posix.c | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>>> index 37e8a2d..2929872 100644
>>> --- a/qga/commands-posix.c
>>> +++ b/qga/commands-posix.c
>>> @@ -2044,7 +2044,9 @@ static void transfer_vcpu(GuestLogicalProcessor
>>> *vcpu, bool sys2vcpu,
>>>vcpu->logical_id);
>>> dirfd = open(dirpath, O_RDONLY | O_DIRECTORY);
>>> if (dirfd == -1) {
>>> -error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
>>> +if (!(sys2vcpu && errno == ENOENT)) {
>>> +error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
>>> +}
>>> } else {
>>> static const char fn[] = "online";
>>> int fd;
>>>
>>
>> Originally these guest agent commands (both getting and setting) were
>> meant to be used in the absence of real VCPU hot[un]plug, as a fallback
>> / place-holder.
>>
>> If the latter (= real VCPU hot(un)plug) works, then these guest agent
>> commands shouldn't be used at all.
> Technically there isn't reasons for "get" not to work in sparse usecase
> hence the patch.
>
>> Drew, do I remember correctly? ... The related RHBZ is
>> . (It's a private
>> one, and I'm not at liberty to open it up, so my apologies to non-RH folks.)
>>
>> Anyway, given that "set" should be a subset of the "get" return value
>> (as documented in the command schema), if we fix up "get" to work with
>> sparse topologies, then "set" should work at once.
>>
>> However... as far as I understand, this change will allow
>> qmp_guest_get_vcpus() to produce a GuestLogicalProcessor object for the
>> missing (hot-unplugged) VCPU, with the following contents:
>> - @logical-id: populated by the loop,
>> - @online: false (from g_malloc0()),
>> - @can-offline: present (from the loop), and false (from g_malloc0()).
>>
>> The smaller problem with this might be that "online==false &&
>> can-offline==false" is nonsensical and has never been returned before. I
>> don't know how higher level apps will react.
>>
>> The larger problem might be that a higher level app could simply copy
>> this output structure into the next "set" call unchanged, and then that
>> "set" call will fail.
> Libvirt it seems that survives such outrageous output
>
>> I wonder if, instead of this patch, we should rework
>> qmp_guest_get_vcpus(), to silently skip processors for which this
>> dirpath ENOENT condition arises (i.e., return a shorter list of
>> GuestLogicalProcessor objects).
>
>> But, again, I wouldn't mix this guest agent command with real VCPU
>> hot(un)plug in the first place. The latter is much-much better, so if
>> it's available, use that exclusively?
> Agreed,
>
> Maybe we can block invalid usecase on libvirt side with a more clear
> error message as libvirt sort of knows that sparse cpus are supported.
OK -- I see libvir-list is CC'd, so let's hear what they prefer :)
Thanks
Laszlo
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [jenkins-ci PATCH 1/8] lcitool: Add "-r REVISION" argument for build
On Wed, 2018-08-29 at 17:08 +0200, Andrea Bolognani wrote:
> This will allow users to build arbitrary branches from
> arbitrary git repositories, but for the moment all it
> does is parse the argument and pass it down to the
> Ansible playbook.
>
> Signed-off-by: Andrea Bolognani
> ---
> guests/lcitool | 36 ++--
> 1 file changed, 26 insertions(+), 10 deletions(-)
As helpfully pointed out by Erik, this change has the unintended
consequence of making '-r REVISION' mandatory every time a playbook
is invoked, including when using '-a update'.
The patch below fixes the issue; consider it squashed in.
diff --git a/guests/lcitool b/guests/lcitool
index c12143d..0729d55 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -367,13 +367,15 @@ class Application:
ansible_hosts = ",".join(self._inventory.expand_pattern(hosts))
selected_projects = self._projects.expand_pattern(projects)
-if revision is None:
-raise Error("Missing git revision")
-tokens = revision.split('/')
-if len(tokens) < 2:
-raise Error("Invalid git revision '{}'".format(revision))
-git_remote = tokens[0]
-git_branch = '/'.join(tokens[1:])
+if revision is not None:
+tokens = revision.split('/')
+if len(tokens) < 2:
+raise Error("Invalid git revision '{}'".format(revision))
+git_remote = tokens[0]
+git_branch = '/'.join(tokens[1:])
+else:
+git_remote = None
+git_branch = None
ansible_cfg_path = os.path.join(base, "ansible.cfg")
playbook_base = os.path.join(base, "playbooks", playbook)
@@ -494,6 +496,9 @@ class Application:
self._execute_playbook("update", hosts, projects, revision)
def _action_build(self, hosts, projects, revision):
+if revision is None:
+raise Error("Missing git revision")
+
self._execute_playbook("build", hosts, projects, revision)
def _action_dockerfile(self, hosts, projects, _revision):
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] rpc: fix handling of SSH auth failure code
The result of libssh2_userauth_password is being assigned to 'ret' in
one branch and 'rc' in the other branch. Checks are all done against the
'ret' variable, so one branch never does the correct check.
Signed-off-by: Daniel P. Berrangé
---
src/rpc/virnetsshsession.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/rpc/virnetsshsession.c b/src/rpc/virnetsshsession.c
index 35dc6c5356..54c3a6f7aa 100644
--- a/src/rpc/virnetsshsession.c
+++ b/src/rpc/virnetsshsession.c
@@ -706,9 +706,9 @@ virNetSSHAuthenticatePassword(virNetSSHSessionPtr sess,
if (priv->password) {
/* tunelled password authentication */
-if ((ret = libssh2_userauth_password(sess->session,
- priv->username,
- priv->password)) == 0) {
+if ((rc = libssh2_userauth_password(sess->session,
+priv->username,
+priv->password)) == 0) {
ret = 0;
goto cleanup;
}
@@ -737,7 +737,7 @@ virNetSSHAuthenticatePassword(virNetSSHSessionPtr sess,
goto cleanup;
}
-if (ret != LIBSSH2_ERROR_AUTHENTICATION_FAILED)
+if (rc != LIBSSH2_ERROR_AUTHENTICATION_FAILED)
break;
VIR_FREE(password);
@@ -750,7 +750,7 @@ virNetSSHAuthenticatePassword(virNetSSHSessionPtr sess,
_("authentication failed: %s"), errmsg);
/* determine exist status */
-if (ret == LIBSSH2_ERROR_AUTHENTICATION_FAILED)
+if (rc == LIBSSH2_ERROR_AUTHENTICATION_FAILED)
return 1;
else
return -1;
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] tests: pass ULLONG_MAX to qemuMonitorJSONGetBalloonInfo
On Tue, Aug 28, 2018 at 12:22:55PM +0200, Ján Tomko wrote: > Test that we correctly accept 64-bit unsigned numbers for QEMU. > > Signed-off-by: Ján Tomko > --- > tests/qemumonitorjsontest.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/5] Test parsing of large numbers in JSON
On Tue, Aug 28, 2018 at 12:22:54PM +0200, Ján Tomko wrote: > We expect to get numbers as big as ULLONG_MAX from QEMU, > add a test for them. > > Signed-off-by: Ján Tomko > --- > tests/virjsontest.c | 3 +++ > 1 file changed, 3 insertions(+) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] virjsontest: use the test name in AddRemove test
On Tue, Aug 28, 2018 at 12:22:53PM +0200, Ján Tomko wrote: > Instead of printing the whole JSON in error messages, > print just the test name. > > Signed-off-by: Ján Tomko > --- > tests/virjsontest.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/5] virjsontest: use name instead of doc for deflatten test
On Tue, Aug 28, 2018 at 12:22:52PM +0200, Ján Tomko wrote: > This test gets its JSON docs from files. > > Now that we have a 'name' field in testInfo, use it instead > of abusing the 'doc' field. > > Signed-off-by: Ján Tomko > --- > tests/virjsontest.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/5] virjsontest: store name in testInfo
On Tue, Aug 28, 2018 at 12:22:51PM +0200, Ján Tomko wrote: > Give the testing function access to the test name instead of only > passing it to virTestRun. > > Signed-off-by: Ján Tomko > --- > tests/virjsontest.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qga: ignore non present cpus when handling qmp_guest_get_vcpus()
On Thu, 30 Aug 2018 17:51:13 +0200
Laszlo Ersek wrote:
> +Drew
>
> On 08/30/18 14:08, Igor Mammedov wrote:
> > If VM has VCPUs plugged sparselly (for example a VM started with
> > 3 VCPUs (cpu0, cpu1 and cpu2) and then cpu1 was hotunplugged so
> > only cpu0 and cpu2 are present), QGA will rise a error
> > error: internal error: unable to execute QEMU agent command
> > 'guest-get-vcpus':
> > open("/sys/devices/system/cpu/cpu1/"): No such file or directory
> > when
> > virsh vcpucount FOO --guest
> > is executed.
> > Fix it by ignoring non present CPUs when fetching CPUs status from sysfs.
> >
> > Signed-off-by: Igor Mammedov
> > ---
> > qga/commands-posix.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index 37e8a2d..2929872 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> > @@ -2044,7 +2044,9 @@ static void transfer_vcpu(GuestLogicalProcessor
> > *vcpu, bool sys2vcpu,
> >vcpu->logical_id);
> > dirfd = open(dirpath, O_RDONLY | O_DIRECTORY);
> > if (dirfd == -1) {
> > -error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
> > +if (!(sys2vcpu && errno == ENOENT)) {
> > +error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
> > +}
> > } else {
> > static const char fn[] = "online";
> > int fd;
> >
>
> Originally these guest agent commands (both getting and setting) were
> meant to be used in the absence of real VCPU hot[un]plug, as a fallback
> / place-holder.
>
> If the latter (= real VCPU hot(un)plug) works, then these guest agent
> commands shouldn't be used at all.
Technically there isn't reasons for "get" not to work in sparse usecase
hence the patch.
> Drew, do I remember correctly? ... The related RHBZ is
> . (It's a private
> one, and I'm not at liberty to open it up, so my apologies to non-RH folks.)
>
> Anyway, given that "set" should be a subset of the "get" return value
> (as documented in the command schema), if we fix up "get" to work with
> sparse topologies, then "set" should work at once.
>
> However... as far as I understand, this change will allow
> qmp_guest_get_vcpus() to produce a GuestLogicalProcessor object for the
> missing (hot-unplugged) VCPU, with the following contents:
> - @logical-id: populated by the loop,
> - @online: false (from g_malloc0()),
> - @can-offline: present (from the loop), and false (from g_malloc0()).
>
> The smaller problem with this might be that "online==false &&
> can-offline==false" is nonsensical and has never been returned before. I
> don't know how higher level apps will react.
>
> The larger problem might be that a higher level app could simply copy
> this output structure into the next "set" call unchanged, and then that
> "set" call will fail.
Libvirt it seems that survives such outrageous output
> I wonder if, instead of this patch, we should rework
> qmp_guest_get_vcpus(), to silently skip processors for which this
> dirpath ENOENT condition arises (i.e., return a shorter list of
> GuestLogicalProcessor objects).
> But, again, I wouldn't mix this guest agent command with real VCPU
> hot(un)plug in the first place. The latter is much-much better, so if
> it's available, use that exclusively?
Agreed,
Maybe we can block invalid usecase on libvirt side with a more clear
error message as libvirt sort of knows that sparse cpus are supported.
>
> Thanks,
> Laszlo
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] virhostdev: Fix PCI devices are still attatched to stub driver bug
Currently, PCI devices will not be rebound to host drivers but
attached to the stub driver when:
1) use libvirt to start a virtual machine with PCI devices assigned,
then stop libvirtd process and shutdown the virtual machine. Finally,
PCI devices are still bound to the stub driver instead of host drivers
after libvirt start again.
2) use libvirt to shutdown a virtual machine wtih PCI devices assigned,
then stop libvirtd process before libvirt try to rebind PCI devices to
host drivers. Finally, PCI devices are still bound to the stub driver
after libvirt start again.
Notice that the comment on the top of virPCIDeviceDetach as follows:
activeDevs should be a list of all PCI devices currently in use by a
domain.inactiveDevs is a list of all PCI devices that libvirt has
detached from the host driver + attached to the stub driver, but
hasn't yet assigned to a domain.
It's not reasonable that libvirt filter out devices that are either not
active or not used by the current domain and driver. For devices belong
to domains that has been shutdown before libvirt start, we should put
them into inactiveDevs if then meet the condition that PCI devices have
been detached from the host driver + attached to the stub driver.
Moreover, we set orignal states of PCI devices when build PCI devices
list in virHostdevGetPCIHostDeviceList if states has been set in struct
virDomainHostdevDefPtr.
Signed-off-by: Wu Zongyong
---
src/util/virhostdev.c | 20 ++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index ca79c37..ecf95e3 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -235,6 +235,7 @@ virHostdevGetPCIHostDeviceList(virDomainHostdevDefPtr
*hostdevs, int nhostdevs)
for (i = 0; i < nhostdevs; i++) {
virDomainHostdevDefPtr hostdev = hostdevs[i];
virDomainHostdevSubsysPCIPtr pcisrc = >source.subsys.u.pci;
+virDomainHostdevOrigStatesPtr origstates = >origstates;
virPCIDevicePtr pci;
if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
@@ -262,6 +263,10 @@ virHostdevGetPCIHostDeviceList(virDomainHostdevDefPtr
*hostdevs, int nhostdevs)
virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_XEN);
else
virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_KVM);
+
+virPCIDeviceSetUnbindFromStub(pci,
origstates->states.pci.unbind_from_stub);
+virPCIDeviceSetRemoveSlot(pci, origstates->states.pci.remove_slot);
+virPCIDeviceSetReprobe(pci, origstates->states.pci.reprobe);
}
return pcidevs;
@@ -1008,8 +1013,19 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr,
continue;
}
} else {
-virPCIDeviceListDel(pcidevs, pci);
-continue;
+int stub = virPCIDeviceGetStubDriver(pci);
+if (stub > VIR_PCI_STUB_DRIVER_NONE &&
+stub < VIR_PCI_STUB_DRIVER_LAST) {
+/* The device is bound to a known stub driver: add a copy
+ * to the inactive list */
+VIR_DEBUG("Adding PCI device %s to inactive list",
+ virPCIDeviceGetName(pci));
+if (virPCIDeviceListAddCopy(mgr->inactivePCIHostdevs, pci) <
0) {
+VIR_ERROR(_("Failed to add PCI device %s to the inactive
list"),
+ virGetLastErrorMessage());
+virResetLastError();
+}
+}
}
i++;
--
1.9.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
