Re: [libvirt] Ability to add arbitrary data to snapshots
On Sat, Jan 23, 2016 at 03:17:43PM +0300, Alexander Burluka wrote: > Hello! > > Currently our Virtuozzo company is working on some kind of management tool > for libvirt. We encountered some problems with snapshots while development. > So I would like to ask community a few questions: > 1) Would you mind if I extend current libvirt snapshot with ability to add > arbitrary data (blob or some config files)? > 2) Would this feature be useful for your users? > > I see it as something like: > virsh snapshot-create VM1 --blob /path/to/some-file > virsh snapshot-revert VM1 > and /path/to/some-file appeared again. You don't really explain what you would want libvirt to do with this data ? Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Ability to add arbitrary data to snapshots
Just save this data near main snapshot data (e.g. in /var/lib/libvirt/qemu/snapshot//) and restore it on previous or specified path. On 01/25/2016 11:46 AM, Daniel P. Berrange wrote: On Sat, Jan 23, 2016 at 03:17:43PM +0300, Alexander Burluka wrote: Hello! Currently our Virtuozzo company is working on some kind of management tool for libvirt. We encountered some problems with snapshots while development. So I would like to ask community a few questions: 1) Would you mind if I extend current libvirt snapshot with ability to add arbitrary data (blob or some config files)? 2) Would this feature be useful for your users? I see it as something like: virsh snapshot-create VM1 --blob /path/to/some-file virsh snapshot-revert VM1 and /path/to/some-file appeared again. You don't really explain what you would want libvirt to do with this data ? Regards, Daniel -- Regards, Alexander Burluka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: qemuDomainRename and virDomainObjListNumOfDomains ABBA deadlock fix
A pretty nasty deadlock occurs while trying to rename a VM in parallel
with virDomainObjListNumOfDomains.
The short description of the problem is as follows:
Thread #1:
qemuDomainRename:
--> aquires domain lock by qemuDomObjFromDomain
-> waits for domain list lock in any of the listed functions:
- virDomainObjListFindByName
- virDomainObjListRenameAddNew
- virDomainObjListRenameRemove
Thread #2:
virDomainObjListNumOfDomains:
--> aquires domain list lock
-> waits for domain lock in virDomainObjListCount
The patch establishes a single order of taking locks: driver->domains list
first, then a particular VM. This is done by implementing a set of
virDomainObjListXxxLocked functions working with driver->domains that assume
that list lock is already aquired by calling functions.
Signed-off-by: Maxim Nestratov
---
src/conf/virdomainobjlist.c | 74 +
src/conf/virdomainobjlist.h | 9 ++
src/libvirt_private.syms| 4 +++
src/qemu/qemu_driver.c | 40 +---
4 files changed, 110 insertions(+), 17 deletions(-)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index 7568f93..89e28a8 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -132,21 +132,19 @@ virDomainObjPtr
virDomainObjListFindByID(virDomainObjListPtr doms,
static virDomainObjPtr
-virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms,
+virDomainObjListFindByUUIDInternalLocked(virDomainObjListPtr doms,
const unsigned char *uuid,
bool ref)
{
char uuidstr[VIR_UUID_STRING_BUFLEN];
virDomainObjPtr obj;
-virObjectLock(doms);
virUUIDFormat(uuid, uuidstr);
obj = virHashLookup(doms->objs, uuidstr);
-if (ref) {
+if (ref)
virObjectRef(obj);
-virObjectUnlock(doms);
-}
+
if (obj) {
virObjectLock(obj);
if (obj->removing) {
@@ -156,8 +154,19 @@ virDomainObjListFindByUUIDInternal(virDomainObjListPtr
doms,
obj = NULL;
}
}
-if (!ref)
-virObjectUnlock(doms);
+return obj;
+}
+
+static virDomainObjPtr
+virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms,
+ const unsigned char *uuid,
+ bool ref)
+{
+virDomainObjPtr obj;
+
+virObjectLock(doms);
+obj = virDomainObjListFindByUUIDInternalLocked(doms, uuid, ref);
+virObjectUnlock(doms);
return obj;
}
@@ -169,6 +178,13 @@ virDomainObjListFindByUUID(virDomainObjListPtr doms,
return virDomainObjListFindByUUIDInternal(doms, uuid, false);
}
+virDomainObjPtr
+virDomainObjListFindByUUIDRefLocked(virDomainObjListPtr doms,
+ const unsigned char *uuid)
+{
+return virDomainObjListFindByUUIDInternalLocked(doms, uuid, true);
+}
+
virDomainObjPtr
virDomainObjListFindByUUIDRef(virDomainObjListPtr doms,
@@ -177,6 +193,23 @@ virDomainObjListFindByUUIDRef(virDomainObjListPtr doms,
return virDomainObjListFindByUUIDInternal(doms, uuid, true);
}
+virDomainObjPtr virDomainObjListFindByNameLocked(virDomainObjListPtr doms,
+ const char *name)
+{
+virDomainObjPtr obj;
+
+obj = virHashLookup(doms->objsName, name);
+virObjectRef(obj);
+if (obj) {
+virObjectLock(obj);
+if (obj->removing) {
+virObjectUnlock(obj);
+virObjectUnref(obj);
+obj = NULL;
+}
+}
+return obj;
+}
virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms,
const char *name)
@@ -312,14 +345,12 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr
doms,
return ret;
}
-
int
-virDomainObjListRenameAddNew(virDomainObjListPtr doms,
+virDomainObjListRenameAddNewLocked(virDomainObjListPtr doms,
virDomainObjPtr vm,
const char *name)
{
int ret = -1;
-virObjectLock(doms);
/* Add new name into the hash table of domain names. */
if (virHashAddEntry(doms->objsName, name, vm) < 0)
@@ -332,21 +363,38 @@ virDomainObjListRenameAddNew(virDomainObjListPtr doms,
ret = 0;
cleanup:
+return ret;
+}
+
+int
+virDomainObjListRenameAddNew(virDomainObjListPtr doms,
+ virDomainObjPtr vm,
+ const char *name)
+{
+int ret = -1;
+virObjectLock(doms);
+ret = virDomainObjListRenameAddNewLocked(doms, vm, name);
virObjectUnlock(doms);
return ret;
}
+int
+virDomainObjListRenameRemoveLocked(virDomainObjListPtr doms, const char *name)
+{
+virHashRemoveEntry(doms->objsName, name);
+return 0;
+}
int
virDomainObjListRenameRemove(virDomainObjListPtr doms, const char *name)
{
+int ret;
Re: [libvirt] [PATCH 1/4] vz: Split logic of prlsdkAddDomain() into two functions
On 23.01.2016 11:42, Mikhail Feoktistov wrote:
> Add two functions
> 1. prlsdkNewDomain() creates new empty domain in domains list with the
> specific uuid.
> 2. prlsdkSetDomainInstance() add data from VM to domain object.
> ---
> src/vz/vz_sdk.c | 49 +
> src/vz/vz_sdk.h | 8
> 2 files changed, 57 insertions(+)
>
> diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
> index b78c413..c705517 100644
> --- a/src/vz/vz_sdk.c
> +++ b/src/vz/vz_sdk.c
> @@ -1509,6 +1509,55 @@ prlsdkUpdateDomain(vzConnPtr privconn, virDomainObjPtr
> dom)
> return retdom ? 0 : -1;
> }
>
> +int
> +prlsdkSetDomainInstance(vzConnPtr privconn, virDomainObjPtr dom, const
> unsigned char *uuid)
> +{
> +PRL_HANDLE sdkdom = PRL_INVALID_HANDLE;
> +virDomainObjPtr retdom;
> +
> +sdkdom = prlsdkSdkDomainLookupByUUID(privconn, uuid);
> +if (sdkdom == PRL_INVALID_HANDLE)
> +return -1;
> +
> +retdom = prlsdkLoadDomain(privconn, sdkdom, dom);
> +PrlHandle_Free(sdkdom);
> +return retdom ? 0 : -1;
> +}
I suggest name it prlsdkLoadDomainUUID where use pass uuid instead of handle.
Another option is to just inline it. We use it only once.
> +
> +virDomainObjPtr
> +prlsdkNewDomain(vzConnPtr privconn, char *name, const unsigned char *uuid)
> +{
> +virDomainDefPtr def = NULL;
> +virDomainObjPtr dom = NULL;
> +vzDomObjPtr pdom = NULL;
> +
> +if (!(def = virDomainDefNewFull(name, uuid, -1)))
> +goto error;
> +
> +if (VIR_ALLOC(pdom) < 0)
> +goto error;
> +
> +if (STREQ(privconn->drivername, "vz"))
> +def->virtType = VIR_DOMAIN_VIRT_VZ;
> +else
> +def->virtType = VIR_DOMAIN_VIRT_PARALLELS;
> +
> +if (!(dom = virDomainObjListAdd(privconn->domains, def,
> +privconn->xmlopt,
> +0, NULL)))
> +goto error;
> +
> +dom->privateData = pdom;
> +dom->privateDataFreeFunc = prlsdkDomObjFreePrivate;
> +dom->persistent = 1;
> +return dom;
> +
> + error:
> +virDomainDefFree(def);
> +VIR_FREE(pdom);
> +return NULL;
> +}
> +
> static int prlsdkSendEvent(vzConnPtr privconn,
> virDomainObjPtr dom,
> virDomainEventType lvEventType,
> diff --git a/src/vz/vz_sdk.h b/src/vz/vz_sdk.h
> index ff6be07..060635e 100644
> --- a/src/vz/vz_sdk.h
> +++ b/src/vz/vz_sdk.h
> @@ -32,6 +32,14 @@ int
> prlsdkLoadDomains(vzConnPtr privconn);
> virDomainObjPtr
> prlsdkAddDomain(vzConnPtr privconn, const unsigned char *uuid);
> +virDomainObjPtr
> +prlsdkNewDomain(vzConnPtr privconn,
> +char *name,
> +const unsigned char *uuid);
> +int
> +prlsdkSetDomainInstance(vzConnPtr privconn,
> +virDomainObjPtr dom,
> +const unsigned char *uuid);
> int prlsdkUpdateDomain(vzConnPtr privconn, virDomainObjPtr dom);
> int prlsdkSubscribeToPCSEvents(vzConnPtr privconn);
> void prlsdkUnsubscribeFromPCSEvents(vzConnPtr privconn);
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/4] vz: Remove prlsdkAddDomain() and use new functions
You use 'and' in description like this patch have to distinct purposes.
Looks like a candidate for splitting.
Say in first patch you use inroduced functions and thus fix a race. By
the way this could be done in one step with introducing the functions.
In second you refactor prlsdkHandleVmAddedEvent. But I don't like the
way you do it much as you get non-cleanup goto in the new version.
On 23.01.2016 11:42, Mikhail Feoktistov wrote:
> Use prlsdkNewDomain() and prlsdkSetDomainInstance() instead of
> prlsdkAddDomain()
> New algorithm for creating an instance:
> In vzDomainDefineXMLFlags() we add new domain to domain list by calling
> prlsdkNewDomain()
> and only after that we call CreateVm() to create VM.
> It means that we "reserve" domain object with the specific uuid.
> After creation of new VM we add info from this VM
> to reserved domain object by calling sdkSetDomainInstance().
>
> In notification handler prlsdkHandleVmAddedEvent() we check
> the existence of a domain and if it doesn't exist we add new domain by calling
> prlsdkLoadDomain().
> ---
> src/vz/vz_driver.c | 14 --
> src/vz/vz_sdk.c| 38 +++---
> src/vz/vz_sdk.h| 2 --
> 3 files changed, 27 insertions(+), 27 deletions(-)
>
> diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
> index f73f8ef..d1cf8d0 100644
> --- a/src/vz/vz_driver.c
> +++ b/src/vz/vz_driver.c
> @@ -687,6 +687,7 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char
> *xml, unsigned int flags)
> virDomainPtr retdom = NULL;
> virDomainDefPtr def;
> virDomainObjPtr olddom = NULL;
> +virDomainObjPtr newdom = NULL;
> unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE;
>
> virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL);
> @@ -702,6 +703,10 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char
> *xml, unsigned int flags)
> olddom = virDomainObjListFindByUUID(privconn->domains, def->uuid);
> if (olddom == NULL) {
> virResetLastError();
> +newdom = prlsdkNewDomain(privconn, def->name, def->uuid);
> +if (!newdom)
> +goto cleanup;
> +
> if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
> if (prlsdkCreateVm(conn, def))
> goto cleanup;
> @@ -715,8 +720,7 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char
> *xml, unsigned int flags)
> goto cleanup;
> }
>
> -olddom = prlsdkAddDomain(privconn, def->uuid);
> -if (!olddom)
> +if (prlsdkSetDomainInstance(privconn, newdom, def->uuid))
> goto cleanup;
> } else {
> int state, reason;
> @@ -757,6 +761,12 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char
> *xml, unsigned int flags)
> cleanup:
> if (olddom)
> virObjectUnlock(olddom);
> +if (newdom) {
> +if (!retdom)
> + virDomainObjListRemove(privconn->domains, newdom);
> +else
> + virObjectUnlock(newdom);
> +}
> virDomainDefFree(def);
> vzDriverUnlock(privconn);
> return retdom;
> diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
> index c705517..765f5f1 100644
> --- a/src/vz/vz_sdk.c
> +++ b/src/vz/vz_sdk.c
> @@ -1473,27 +1473,6 @@ prlsdkLoadDomains(vzConnPtr privconn)
> return -1;
> }
>
> -virDomainObjPtr
> -prlsdkAddDomain(vzConnPtr privconn, const unsigned char *uuid)
> -{
> -PRL_HANDLE sdkdom = PRL_INVALID_HANDLE;
> -virDomainObjPtr dom;
> -
> -dom = virDomainObjListFindByUUID(privconn->domains, uuid);
> -if (dom) {
> -/* domain is already in the list */
> -return dom;
> -}
> -
> -sdkdom = prlsdkSdkDomainLookupByUUID(privconn, uuid);
> -if (sdkdom == PRL_INVALID_HANDLE)
> -return NULL;
> -
> -dom = prlsdkLoadDomain(privconn, sdkdom, NULL);
> -PrlHandle_Free(sdkdom);
> -return dom;
> -}
> -
> int
> prlsdkUpdateDomain(vzConnPtr privconn, virDomainObjPtr dom)
> {
> @@ -1672,11 +1651,24 @@ prlsdkHandleVmAddedEvent(vzConnPtr privconn,
> unsigned char *uuid)
> {
> virDomainObjPtr dom = NULL;
> +PRL_HANDLE sdkdom = PRL_INVALID_HANDLE;
>
> -dom = prlsdkAddDomain(privconn, uuid);
> -if (dom == NULL)
> +dom = virDomainObjListFindByUUID(privconn->domains, uuid);
> +if (dom) {
> +/* domain is already in the list */
> +goto send_event;
> +}
> +
> +sdkdom = prlsdkSdkDomainLookupByUUID(privconn, uuid);
> +if (sdkdom == PRL_INVALID_HANDLE)
> +return;
> +
> +dom = prlsdkLoadDomain(privconn, sdkdom, NULL);
> +PrlHandle_Free(sdkdom);
> +if (!dom)
> return;
>
> + send_event:
> prlsdkSendEvent(privconn, dom, VIR_DOMAIN_EVENT_DEFINED,
> VIR_DOMAIN_EVENT_DEFINED_ADDED);
>
> diff --git a/src/vz/vz_sdk.h b/src/vz/vz_sdk.h
> index 060635e..19bce88 100644
> --- a/src/vz/vz_sdk.h
> +++ b/src/vz/vz_sdk.h
> @@ -31,8 +31,6 @@ void prlsdkDisconnect(vzConnPtr privc
Re: [libvirt] [PATCH 3/4] vz: remove code duplication from LoadDomain()
prlsdkLoadDomain works in 2 different cases:
1. Called first time for a domain. Then it creates and
initializes it. Then updates it from sdk handle.
2. Called when domain is already in list. In this case it updates domain from
sdk handle.
I think we could end up in a better series if we split
this function into 2: first is to create and initialize,
second update domain from sdk handle (load).
On 23.01.2016 11:42, Mikhail Feoktistov wrote:
> Now we create new domain by calling prlsdkNewDomain().
> In LoadDomain() we just load info from vm instance to domain object.
> So remove code to create domain from LoadDomain().
>
> ---
> src/vz/vz_sdk.c | 55 +--
> 1 file changed, 17 insertions(+), 38 deletions(-)
>
> diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
> index 765f5f1..d9f2127 100644
> --- a/src/vz/vz_sdk.c
> +++ b/src/vz/vz_sdk.c
> @@ -1260,26 +1260,29 @@ prlsdkLoadDomain(vzConnPtr privconn,
> PRL_UINT32 ram;
> PRL_UINT32 envId;
> PRL_VM_AUTOSTART_OPTION autostart;
> +unsigned char uuid[VIR_UUID_BUFLEN];
> +char *name;
>
> virCheckNonNullArgGoto(privconn, error);
> virCheckNonNullArgGoto(sdkdom, error);
>
> -if (!(def = virDomainDefNew()))
> +if (prlsdkGetDomainIds(sdkdom, &name, uuid) < 0)
> goto error;
>
> if (!olddom) {
> -if (VIR_ALLOC(pdom) < 0)
> -goto error;
> +dom = prlsdkNewDomain(privconn, name, uuid);
> +def = dom->def;
> +pdom = dom->privateData;
> } else {
> +if (!(def = virDomainDefNewFull(name, uuid, -1)))
> +goto error;
> pdom = olddom->privateData;
> +if (STREQ(privconn->drivername, "vz"))
> +def->virtType = VIR_DOMAIN_VIRT_VZ;
> +else
> +def->virtType = VIR_DOMAIN_VIRT_PARALLELS;
> }
> -
> -if (STREQ(privconn->drivername, "vz"))
> -def->virtType = VIR_DOMAIN_VIRT_VZ;
> -else
> -def->virtType = VIR_DOMAIN_VIRT_PARALLELS;
> -
> -def->id = -1;
> +VIR_FREE(name);
>
> /* we will remove this field in the near future, so let's set it
> * to NULL temporarily */
> @@ -1292,9 +1295,6 @@ prlsdkLoadDomain(vzConnPtr privconn,
> goto error;
> }
>
> -if (prlsdkGetDomainIds(sdkdom, &def->name, def->uuid) < 0)
> -goto error;
> -
> def->onReboot = VIR_DOMAIN_LIFECYCLE_RESTART;
> def->onPoweroff = VIR_DOMAIN_LIFECYCLE_DESTROY;
> def->onCrash = VIR_DOMAIN_LIFECYCLE_CRASH_DESTROY;
> @@ -1369,23 +1369,10 @@ prlsdkLoadDomain(vzConnPtr privconn,
> }
>
> if (olddom) {
> -/* assign new virDomainDef without any checks */
> -/* we can't use virDomainObjAssignDef, because it checks
> - * for state and domain name */
> dom = olddom;
> virDomainDefFree(dom->def);
> dom->def = def;
> -} else {
> -if (!(dom = virDomainObjListAdd(privconn->domains, def,
> -privconn->xmlopt,
> -0, NULL)))
> -goto error;
> }
> -/* dom is locked here */
> -
> -dom->privateData = pdom;
> -dom->privateDataFreeFunc = prlsdkDomObjFreePrivate;
> -dom->persistent = 1;
>
> switch (autostart) {
> case PAO_VM_START_ON_LOAD:
> @@ -1411,19 +1398,11 @@ prlsdkLoadDomain(vzConnPtr privconn,
>
> return dom;
> error:
> -if (dom && !olddom) {
> -/* Domain isn't persistent means that we haven't yet set
> - * prlsdkDomObjFreePrivate and should call it manually
> - */
> -if (!dom->persistent)
> -prlsdkDomObjFreePrivate(pdom);
> -
> +VIR_FREE(name);
> +if (!olddom && dom)
> virDomainObjListRemove(privconn->domains, dom);
> -}
> -/* Delete newly allocated def only if we haven't assigned it to domain
> - * Otherwise we will end up with domain having invalid def within it
> - */
> -if (!dom)
> +
> +if (olddom && !dom)
> virDomainDefFree(def);
>
> return NULL;
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4] qemu: Connect to guest agent iff needed
On 18.01.2016 11:54, Michal Privoznik wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1293351 > > Since we already have virtio channel events, we know when guest > agent within guest has (dis-)connected. Instead of us blindly > connecting to a socket that no one is listening to, we can just > follow what qemu-ga does. This has a nice benefit that we don't > need to 'guest-ping' the agent just to timeout and find out > nobody is listening. > > The way that this commit is implemented: > - don't connect in qemuProcessStart directly, defer that to event > callback (which already follows the agent). > - after migration is settled, before we resume vCPUs, ask qemu > whether somebody is listening on the socket and if so, connect > to it. > > Signed-off-by: Michal Privoznik > --- > > diff to v3: > -Move cap detection into qemuConnectAgent > > src/qemu/qemu_driver.c| 13 - > src/qemu/qemu_migration.c | 12 > src/qemu/qemu_process.c | 24 > 3 files changed, 48 insertions(+), 1 deletion(-) ping Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virsh: Correctly detect inserted media in change-media command
On 13.01.2016 17:39, Michal Privoznik wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1250331 > > It all works like this. The change-media command dumps domain > XML, finds the corresponding cdrom device we want to change media > in and returns it in the xmlNodePtr form. This way we don't have > to bother with keeping all the subelements or attributes that we > don't care about in the XML that is fed back to libvirt for the > update API. > > Now, the problem is we try to be clever here and detect if disk > already has a source (indicated by subelement). > However, bare fact that the element is there does not mean disk > has source. The element has some attributes and only if @file or > @dev is within them disk has source. Any other attribute is > meaningless for our purpose now. Make our clever check better. > > Signed-off-by: Michal Privoznik > --- > tools/virsh-domain.c | 7 ++- > 1 file changed, 6 insertions(+), 1 deletion(-) ping Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Ability to add arbitrary data to snapshots
On Mon, Jan 25, 2016 at 11:54:55AM +0300, Alexander Burluka wrote: > Just save this data near main snapshot data (e.g. in > /var/lib/libvirt/qemu/snapshot//) > and restore it on previous or specified path. What sort of data is this - we already have ability to record arbitrary metadata in the XML, so it is not particularly attractive to add extra hacks to the snapshot XML for dealing with extra config data, when it could be just pplaced in the metadata block. In general I think everything we deal with should be part of the XML config of the guest, so I don't much like the suggestion to extend the snapshot APis to pass through arbitrary opaque blobs. > On 01/25/2016 11:46 AM, Daniel P. Berrange wrote: > >On Sat, Jan 23, 2016 at 03:17:43PM +0300, Alexander Burluka wrote: > >>Hello! > >> > >>Currently our Virtuozzo company is working on some kind of management tool > >>for libvirt. We encountered some problems with snapshots while development. > >>So I would like to ask community a few questions: > >>1) Would you mind if I extend current libvirt snapshot with ability to add > >>arbitrary data (blob or some config files)? > >>2) Would this feature be useful for your users? > >> > >>I see it as something like: > >>virsh snapshot-create VM1 --blob /path/to/some-file > >>virsh snapshot-revert VM1 > >>and /path/to/some-file appeared again. > >You don't really explain what you would want libvirt to do with this > >data ? Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Ability to add arbitrary data to snapshots
For example, we want to store suspended state of VM. I'm aware that some other careless application dealing with libvirt may erase metadata section and info about additional snapshot data will be lost. On 01/25/2016 02:38 PM, Daniel P. Berrange wrote: On Mon, Jan 25, 2016 at 11:54:55AM +0300, Alexander Burluka wrote: Just save this data near main snapshot data (e.g. in /var/lib/libvirt/qemu/snapshot//) and restore it on previous or specified path. What sort of data is this - we already have ability to record arbitrary metadata in the XML, so it is not particularly attractive to add extra hacks to the snapshot XML for dealing with extra config data, when it could be just pplaced in the metadata block. In general I think everything we deal with should be part of the XML config of the guest, so I don't much like the suggestion to extend the snapshot APis to pass through arbitrary opaque blobs. On 01/25/2016 11:46 AM, Daniel P. Berrange wrote: On Sat, Jan 23, 2016 at 03:17:43PM +0300, Alexander Burluka wrote: Hello! Currently our Virtuozzo company is working on some kind of management tool for libvirt. We encountered some problems with snapshots while development. So I would like to ask community a few questions: 1) Would you mind if I extend current libvirt snapshot with ability to add arbitrary data (blob or some config files)? 2) Would this feature be useful for your users? I see it as something like: virsh snapshot-create VM1 --blob /path/to/some-file virsh snapshot-revert VM1 and /path/to/some-file appeared again. You don't really explain what you would want libvirt to do with this data ? Regards, Daniel -- Regards, Alexander Burluka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2] util: keep/use a bitmap of in-use macvtap devices
On Fri, Jan 22, 2016 at 12:52:27PM -0500, Laine Stump wrote:
> This patch creates two bitmaps, one for macvlan devicenames and one
s/devicenames/device names/
[...]
> diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
[...]
> +/**
> + * virNetDevMacVLanReserveName:
> + *
> + * @name: already-known name of device
> + * @quietFail: don't log an error if this name is already in-use
> + *
> + * Extract the device type and id from a macvtap/macvlan device name
> + * and mark the appropriate position as in-use in the appropriate
> + * bitmap.
> + *
> + * returns 0 on success, -1 on failure, -2 if the name doesn't fit the
> auto-pattern
Long line, would be nice to wrap it. And the return 0 isn't true, because
virNetDevMacVLanReserveID() returns ID on success.
> + */
> +int
> +virNetDevMacVLanReserveName(const char *name, bool quietFail)
> +{
> +unsigned int id;
> +unsigned int flags = 0;
> +const char *idstr = NULL;
> +
> +if (virNetDevMacVLanInitialize() < 0)
> + return -1;
> +
> +if (STRPREFIX(name, MACVTAP_NAME_PREFIX)) {
> +idstr = name + strlen(MACVTAP_NAME_PREFIX);
> +flags |= VIR_NETDEV_MACVLAN_CREATE_WITH_TAP;
> +} else if (STRPREFIX(name, MACVLAN_NAME_PREFIX)) {
> +idstr = name + strlen(MACVLAN_NAME_PREFIX);
> +} else {
> +return -2;
> +}
> +
> +if (virStrToLong_ui(idstr, NULL, 10, &id) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("couldn't get id value from macvtap device name
> %s"),
> + name);
> +return -1;
> +}
> +return virNetDevMacVLanReserveID(id, flags, quietFail, false);
> +}
[...]
> const char *type = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ?
> "macvtap" : "macvlan";
> -const char *prefix = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ?
> -MACVTAP_NAME_PREFIX : MACVLAN_NAME_PREFIX;
Maybe use the MACV(TAP|LAN)_NAME_PREFIX instead of the strings for *type.
> const char *pattern = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ?
> MACVTAP_NAME_PATTERN : MACVLAN_NAME_PATTERN;
> -int c, rc;
> +int rc, reservedID = -1;
> char ifname[IFNAMSIZ];
> int retries, do_retry = 0;
> uint32_t macvtapMode;
> -const char *cr_ifname = NULL;
> +const char *ifnameCreated = NULL;
> int ret;
> int vf = -1;
> bool vnet_hdr = flags & VIR_NETDEV_MACVLAN_VNET_HDR;
[...]
> +retries = MACVLAN_MAX_ID + 1;
> +while (!ifnameCreated && retries) {
> virMutexLock(&virNetDevMacVLanCreateMutex);
> -for (c = 0; c < 8192; c++) {
> -snprintf(ifname, sizeof(ifname), pattern, c);
> -if ((ret = virNetDevExists(ifname)) < 0) {
> -virMutexUnlock(&virNetDevMacVLanCreateMutex);
> +reservedID = virNetDevMacVLanReserveID(reservedID, flags, false,
> true);
> +if (reservedID < 0) {
> +virMutexUnlock(&virNetDevMacVLanCreateMutex);
> +return -1;
> +}
> +snprintf(ifname, sizeof(ifname), pattern, reservedID);
Since you're changing this, snprintf returns -1 in case of error.
ACK with the issues fixed.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virsh: Correctly detect inserted media in change-media command
On Wed, Jan 13, 2016 at 05:39:10PM +0100, Michal Privoznik wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1250331 > > It all works like this. The change-media command dumps domain > XML, finds the corresponding cdrom device we want to change media > in and returns it in the xmlNodePtr form. This way we don't have > to bother with keeping all the subelements or attributes that we > don't care about in the XML that is fed back to libvirt for the > update API. > > Now, the problem is we try to be clever here and detect if disk > already has a source (indicated by subelement). > However, bare fact that the element is there does not mean disk > has source. The element has some attributes and only if @file or > @dev is within them disk has source. Any other attribute is > meaningless for our purpose now. Make our clever check better. That's not true, what about disk type='dir|volume|network'? Those could be also used as cdrom or floppy. The patch looks good, but extend it to detect all possible disk types. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Ability to add arbitrary data to snapshots
On Mon, Jan 25, 2016 at 02:55:30PM +0300, Alexander Burluka wrote: > For example, we want to store suspended state of VM. > I'm aware that some other careless application dealing with libvirt > may erase metadata section and info about additional snapshot data will be > lost. What do you mean by suspended state of the VM ? Are you referring to the VM memory & device state ? If so, I think that something that should be explicitly represented in the API, not a opaque blob. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virsh: Correctly detect inserted media in change-media command
On 25.01.2016 13:45, Pavel Hrdina wrote: > On Wed, Jan 13, 2016 at 05:39:10PM +0100, Michal Privoznik wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1250331 >> >> It all works like this. The change-media command dumps domain >> XML, finds the corresponding cdrom device we want to change media >> in and returns it in the xmlNodePtr form. This way we don't have >> to bother with keeping all the subelements or attributes that we >> don't care about in the XML that is fed back to libvirt for the >> update API. >> >> Now, the problem is we try to be clever here and detect if disk >> already has a source (indicated by subelement). >> However, bare fact that the element is there does not mean disk >> has source. The element has some attributes and only if @file or >> @dev is within them disk has source. Any other attribute is >> meaningless for our purpose now. Make our clever check better. > > That's not true, what about disk type='dir|volume|network'? Those could be > also > used as cdrom or floppy. The patch looks good, but extend it to detect all > possible disk types. > Well, the code doesn't know how to deal with those types anyway. For all types you've pointed out we will change the disk type to file. So in the end from a type='dir|volume|network' disk we will create type='file'. I see no point in producing the following XML then: ... But that's a bigger bug to fix. Meanwhile I think we can use my (incomplete?) fix. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virsh: Correctly detect inserted media in change-media command
On Mon, Jan 25, 2016 at 03:21:14PM +0100, Michal Privoznik wrote: > On 25.01.2016 13:45, Pavel Hrdina wrote: > > On Wed, Jan 13, 2016 at 05:39:10PM +0100, Michal Privoznik wrote: > >> https://bugzilla.redhat.com/show_bug.cgi?id=1250331 > >> > >> It all works like this. The change-media command dumps domain > >> XML, finds the corresponding cdrom device we want to change media > >> in and returns it in the xmlNodePtr form. This way we don't have > >> to bother with keeping all the subelements or attributes that we > >> don't care about in the XML that is fed back to libvirt for the > >> update API. > >> > >> Now, the problem is we try to be clever here and detect if disk > >> already has a source (indicated by subelement). > >> However, bare fact that the element is there does not mean disk > >> has source. The element has some attributes and only if @file or > >> @dev is within them disk has source. Any other attribute is > >> meaningless for our purpose now. Make our clever check better. > > > > That's not true, what about disk type='dir|volume|network'? Those could be > > also > > used as cdrom or floppy. The patch looks good, but extend it to detect all > > possible disk types. > > > > Well, the code doesn't know how to deal with those types anyway. For all > types you've pointed out we will change the disk type to file. So in the > end from a type='dir|volume|network' disk we will create type='file'. I > see no point in producing the following XML then: > > > > ... > > > But that's a bigger bug to fix. Meanwhile I think we can use my > (incomplete?) fix. > > Michal No, I'm not talking about the XML that we will create. Let's say, that you have a domain and there is this device: Your patch would simply ignore the dir source and assumed that there is no media inserted and create a new definition. It would be regression from current behavior. It doesn't meter whether the new cdrom would be block or file, you need to check the existing device and consider all possible source types to detect whether there is a media inserted or not. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virsh: Correctly detect inserted media in change-media command
On 25.01.2016 15:46, Pavel Hrdina wrote:
> On Mon, Jan 25, 2016 at 03:21:14PM +0100, Michal Privoznik wrote:
>> On 25.01.2016 13:45, Pavel Hrdina wrote:
>>> On Wed, Jan 13, 2016 at 05:39:10PM +0100, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1250331
It all works like this. The change-media command dumps domain
XML, finds the corresponding cdrom device we want to change media
in and returns it in the xmlNodePtr form. This way we don't have
to bother with keeping all the subelements or attributes that we
don't care about in the XML that is fed back to libvirt for the
update API.
Now, the problem is we try to be clever here and detect if disk
already has a source (indicated by subelement).
However, bare fact that the element is there does not mean disk
has source. The element has some attributes and only if @file or
@dev is within them disk has source. Any other attribute is
meaningless for our purpose now. Make our clever check better.
>>>
>>> That's not true, what about disk type='dir|volume|network'? Those could be
>>> also
>>> used as cdrom or floppy. The patch looks good, but extend it to detect all
>>> possible disk types.
>>>
>>
>> Well, the code doesn't know how to deal with those types anyway. For all
>> types you've pointed out we will change the disk type to file. So in the
>> end from a type='dir|volume|network' disk we will create type='file'. I
>> see no point in producing the following XML then:
>>
>>
>>
>> ...
>>
>>
>> But that's a bigger bug to fix. Meanwhile I think we can use my
>> (incomplete?) fix.
>>
>> Michal
>
> No, I'm not talking about the XML that we will create. Let's say, that you
> have
> a domain and there is this device:
>
>
>
>
>
>
>
>
>
> Your patch would simply ignore the dir source and assumed that there is no
> media
> inserted and create a new definition. It would be regression from current
> behavior. It doesn't meter whether the new cdrom would be block or file, you
> need to check the existing device and consider all possible source types to
> detect whether there is a media inserted or not.
>
A-ha, now I see the problem. Would squashing this in help?
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index f023e3d..dd41260 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -2,8 +2,11 @@ virshUpdateDiskXML(xmlNodePtr disk_node,
}
if (source) {
-if (!(source_path = virXMLPropString(source, "file")))
-source_path = virXMLPropString(source, "dev");
+if (!(source_path = virXMLPropString(source, "file")) &&
+!(source_path = virXMLPropString(source, "dev")) &&
+!(source_path = virXMLPropString(source, "dir")) &&
+!(source_path = virXMLPropString(source, "pool")))
+source_path = virXMLPropString(source, "name");
if (source_path && type == VIRSH_UPDATE_DISK_XML_INSERT) {
vshError(NULL, _("The disk device '%s' already has media"),
target);
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 22/34] conf: disallow empty cpusets for vcpu pinning when parsing XML
On Mon, Jan 18, 2016 at 12:06:15 -0500, John Ferlan wrote: > > > On 01/14/2016 11:27 AM, Peter Krempa wrote: > > They are disallowed in the pinning API and as default cpuset. > > > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1293241 > > --- > > src/conf/domain_conf.c | 6 ++ > > 1 file changed, 6 insertions(+) > > > > Is this going to be problematic for a running domain? e.g., would it > disappear now? I agree with the failure, just worried about the If you use a bitmap string that ends up empty here, it will be parsed correctly at first and the internal data would be valid. The saved xml would not use the "0,^0" string when written to disk, but the bitmap would format as "". When you attempt to parse such string, the parser code errors out. If you open the bugzilla, the situation is described pretty well in there and the API, as said, does already the check. So in a way a previously accepted config would be rejected in this case but nobody used that since it would vanish. > consequences. Not that anyone should have done this, but seems to be > one of those post parse, pre-run type checks since it's been allowed in > the past. Peter signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virsh: Correctly detect inserted media in change-media command
On Mon, Jan 25, 2016 at 04:03:44PM +0100, Michal Privoznik wrote:
> On 25.01.2016 15:46, Pavel Hrdina wrote:
> > On Mon, Jan 25, 2016 at 03:21:14PM +0100, Michal Privoznik wrote:
> >> On 25.01.2016 13:45, Pavel Hrdina wrote:
> >>> On Wed, Jan 13, 2016 at 05:39:10PM +0100, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1250331
>
> It all works like this. The change-media command dumps domain
> XML, finds the corresponding cdrom device we want to change media
> in and returns it in the xmlNodePtr form. This way we don't have
> to bother with keeping all the subelements or attributes that we
> don't care about in the XML that is fed back to libvirt for the
> update API.
>
> Now, the problem is we try to be clever here and detect if disk
> already has a source (indicated by subelement).
> However, bare fact that the element is there does not mean disk
> has source. The element has some attributes and only if @file or
> @dev is within them disk has source. Any other attribute is
> meaningless for our purpose now. Make our clever check better.
> >>>
> >>> That's not true, what about disk type='dir|volume|network'? Those could
> >>> be also
> >>> used as cdrom or floppy. The patch looks good, but extend it to detect
> >>> all
> >>> possible disk types.
> >>>
> >>
> >> Well, the code doesn't know how to deal with those types anyway. For all
> >> types you've pointed out we will change the disk type to file. So in the
> >> end from a type='dir|volume|network' disk we will create type='file'. I
> >> see no point in producing the following XML then:
> >>
> >>
> >>
> >> ...
> >>
> >>
> >> But that's a bigger bug to fix. Meanwhile I think we can use my
> >> (incomplete?) fix.
> >>
> >> Michal
> >
> > No, I'm not talking about the XML that we will create. Let's say, that you
> > have
> > a domain and there is this device:
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > Your patch would simply ignore the dir source and assumed that there is no
> > media
> > inserted and create a new definition. It would be regression from current
> > behavior. It doesn't meter whether the new cdrom would be block or file,
> > you
> > need to check the existing device and consider all possible source types to
> > detect whether there is a media inserted or not.
> >
>
> A-ha, now I see the problem. Would squashing this in help?
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index f023e3d..dd41260 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -2,8 +2,11 @@ virshUpdateDiskXML(xmlNodePtr disk_node,
> }
>
> if (source) {
> -if (!(source_path = virXMLPropString(source, "file")))
> -source_path = virXMLPropString(source, "dev");
> +if (!(source_path = virXMLPropString(source, "file")) &&
> +!(source_path = virXMLPropString(source, "dev")) &&
> +!(source_path = virXMLPropString(source, "dir")) &&
> +!(source_path = virXMLPropString(source, "pool")))
> +source_path = virXMLPropString(source, "name");
>
> if (source_path && type == VIRSH_UPDATE_DISK_XML_INSERT) {
> vshError(NULL, _("The disk device '%s' already has media"),
> target);
ACK with that :)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] virt-host-validate: Check those CGroups that we actually use
On Mon, Jan 25, 2016 at 08:23:21AM +0100, Michal Privoznik wrote: > Since the introduction of virt-host-validate tool the set of > cgroup controllers we use has changed so the tool is checking for > some cgroups that we don't need (e.g. net_cls, although I doubt > we have ever used that one) and is not checking for those we > actually use (e.g. cpuset). > > Signed-off-by: Michal Privoznik > --- > tools/virt-host-validate-lxc.c | 16 > tools/virt-host-validate-qemu.c | 10 +- > 2 files changed, 13 insertions(+), 13 deletions(-) ACK -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] virt-host-validate: Fix error level for user namespace check
On Mon, Jan 25, 2016 at 08:23:22AM +0100, Michal Privoznik wrote: > >From the code it seems to me that we need user namespace iff s/>From/From/ s/iff/if/ > configured in domain XML. Otherwise we don't use it at all. > However our tool is more strict about that. Fix this discrepancy. > > Signed-off-by: Michal Privoznik > --- > tools/virt-host-validate-lxc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) ACK -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] virsh: improve waiting for block job readiness
Since this has been sitting for a bit, I'll poke the bear, but also CC
Peter since he reviewed V1 and had some specific concerns...
On 01/06/2016 10:03 PM, Michael Chapman wrote:
> After a block job hits 100%, we only need to apply a timeout waiting for
> a block job event if exactly one of the BLOCK_JOB or BLOCK_JOB_2
> callbacks were able to be registered.
>
> If neither callback could be registered, there's clearly no need for a
> timeout.
>
> If both callbacks were registered, then we're guaranteed to eventually
> get one of the events. The path being used by virsh must be exactly the
> source path or target device in the domain's disk definition, and these
> are the respective strings sent back in these two events.
>
> Signed-off-by: Michael Chapman
> ---
>
> v1 discussion at:
> http://www.redhat.com/archives/libvir-list/2016-January/msg00031.html
>
> Changes since v1:
> - Fixed bugs in cb_id/cb_id2 conditionals
> - Consistently used break to exit loop, dropped cleanup label
> - Clarified the logic and behaviour in comments
> - Improved commit message
>
> tools/virsh-domain.c | 60
>
> 1 file changed, 32 insertions(+), 28 deletions(-)
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index edbbc34..853416c 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -1875,14 +1875,17 @@ virshBlockJobWaitFree(virshBlockJobWaitDataPtr data)
> * virshBlockJobWait:
> * @data: private data initialized by virshBlockJobWaitInit
> *
> - * Waits for the block job to complete. This function prefers to get an event
> - * from libvirt but still has fallback means if the device name can't be
> matched
> + * Waits for the block job to complete. This function prefers to wait for a
> + * matching VIR_DOMAIN_EVENT_ID_BLOCK_JOB or VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2
> + * event from libvirt, however it has a fallback mode should either of these
nit: It's usually "...; however, ..."
> + * events not be available.
> *
> - * This function returns values from the virConnectDomainEventBlockJobStatus
> enum
> - * or -1 in case of a internal error. Fallback states if a block job vanishes
> - * without triggering the event is VIR_DOMAIN_BLOCK_JOB_COMPLETED. For two
> phase
> - * jobs after the retry count for waiting for the event expires is
> - * VIR_DOMAIN_BLOCK_JOB_READY.
> + * This function returns values from the virConnectDomainEventBlockJobStatus
> + * enum or -1 in case of a internal error.
nit: "...of an internal..." (usage of an instead of a)
> + *
> + * If the fallback mode is activated the returned event is
> + * VIR_DOMAIN_BLOCK_JOB_COMPLETED if the block job vanishes, or
nit: "...block job vanishes or..." (no need for a comma)
> + * VIR_DOMAIN_BLOCK_JOB_READY if the block job reaches 100%.
> */
> static int
> virshBlockJobWait(virshBlockJobWaitDataPtr data)
> @@ -1932,28 +1935,32 @@ virshBlockJobWait(virshBlockJobWaitDataPtr data)
>
> if (result < 0) {
> vshError(data->ctl, _("failed to query job for disk %s"),
> data->dev);
> -goto cleanup;
> +break;
Usage of 'goto' is a libvirt coding convention, even though perhaps not
a convention of all programming styles. I think use of goto here should
be kept (here and other places that follow in this function).
If you feel strongly about usage of break vs. goto, then this should be
a two patch series - the first to convert "goto" usage into "break"
usage and the second to resolve the issue of continuing with the timeout
logic even if both callbacks could not be registered.
> }
>
> -/* if we've got an event for the device we are waiting for we can end
> - * the waiting loop */
> +/* If either callback could be registered and we've got an event, we
> can
> + * can end the waiting loop */
> if ((data->cb_id >= 0 || data->cb_id2 >= 0) && data->status != -1) {
> ret = data->status;
> -goto cleanup;
> +break;
> }
>
> -/* since virsh can't guarantee that the path provided by the user
> will
> - * later be matched in the event we will need to keep the fallback
> - * approach and claim success if the block job finishes or vanishes.
> */
> -if (result == 0)
> -break;
> +/* Fallback behaviour is only needed if one or both callbacks could
> not
> + * be registered */
> +if (data->cb_id < 0 || data->cb_id2 < 0) {
> +/* If the block job vanishes, synthesize a COMPLETED event */
> +if (result == 0) {
> +ret = VIR_DOMAIN_BLOCK_JOB_COMPLETED;
> +break;
> +}
>
> -/* for two-phase jobs we will try to wait in the synchronized phase
> - * for event arrival since 100% completion doesn't necessarily mean
> that
> - * the block job has finished and can be terminated with succ
[libvirt] [PATCH v2 7/9] hostdev: Update comments
Some of the comments are no longer accurate after the recent changes,
others have been expanded and hopefully improved.
The initial summary of the operations has been removed so that two
separate edits are not needed when making changes.
---
src/util/virhostdev.c | 68 +++
1 file changed, 30 insertions(+), 38 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index ab17547..0892dbf 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -675,11 +675,10 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr
hostdev_mgr,
* must be reset before being marked as active.
*/
-/* Loop 1: validate that non-managed device isn't in use, eg
- * by checking that device is either un-bound, or bound
- * to pci-stub.ko
- */
+/* Detaching devices from the host involves several steps; each of them
+ * is described at length below */
+/* Step 1: perform safety checks, eg. ensure the devices are assignable */
for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
bool strict_acs_check = !!(flags & VIR_HOSTDEV_STRICT_ACS_CHECK);
@@ -710,7 +709,8 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr
hostdev_mgr,
}
}
-/* Loop 2: detach managed devices (i.e. bind to appropriate stub driver) */
+/* Step 2: detach managed devices (i.e. bind to appropriate stub driver).
+ * detachPCIDevices() will also mark devices as inactive */
for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
@@ -718,11 +718,10 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr
hostdev_mgr,
goto reattachdevs;
}
-/* At this point, all devices are attached to the stub driver and have
+/* At this point, devices are attached to the stub driver and have
* been marked as inactive */
-/* Loop 3: Now that all the PCI hostdevs have been detached, we
- * can safely reset them */
+/* Step 3: perform a PCI reset on all devices */
for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
@@ -732,8 +731,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr
hostdev_mgr,
goto reattachdevs;
}
-/* Loop 4: For SRIOV network devices, Now that we have detached the
- * the network device, set the netdev config */
+/* Step 4: set the netdev config for SRIOV network devices */
for (i = 0; i < nhostdevs; i++) {
virDomainHostdevDefPtr hostdev = hostdevs[i];
if (!virHostdevIsPCINetDevice(hostdev))
@@ -762,9 +760,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr
hostdev_mgr,
goto inactivedevs;
}
-/* Loop 7: Now set the used_by_domain of the device in
- * activePCIHostdevs as domain name.
- */
+/* Step 6: set driver and domain information */
for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
virPCIDevicePtr dev, activeDev;
@@ -777,7 +773,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr
hostdev_mgr,
virPCIDeviceSetUsedBy(activeDev, drv_name, dom_name);
}
-/* Loop 8: Now set the original states for hostdev def */
+/* Step 7: set the original states for hostdev def */
for (i = 0; i < nhostdevs; i++) {
virPCIDevicePtr dev;
virPCIDevicePtr pcidev;
@@ -792,10 +788,9 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr
hostdev_mgr,
dev = virPCIDeviceNew(pcisrc->addr.domain, pcisrc->addr.bus,
pcisrc->addr.slot, pcisrc->addr.function);
-/* original states "unbind_from_stub", "remove_slot",
- * "reprobe" were already set by pciDettachDevice in
- * loop 2.
- */
+/* original states for "unbind_from_stub", "remove_slot" and
+ * "reprobe" (used when reattaching) were already set by
+ * detachPCIDevices() in a previous step */
VIR_DEBUG("Saving network configuration of PCI device %s",
virPCIDeviceGetName(dev));
if ((pcidev = virPCIDeviceListFind(pcidevs, dev))) {
@@ -875,16 +870,11 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr
hostdev_mgr,
goto cleanup;
}
-/* Loop through the assigned devices 4 times: 1) delete them all from
- * activePCIHostdevs, 2) restore network config of SRIOV netdevs, 3) Do a
- * PCI reset on each device, 4) reattach the devices to their host drivers
- * (managed) or add them to inactivePCIHostdevs (!managed).
- */
+/* Reattaching devices to the host involves several steps; each of them
+ * is described at length below */
-/*
- * Loop 1: verify that each device in the hostdevs list really was in use
- * by this domain, and remove them all from the activePCIHostdevs list.
- */
+/* Ste
[libvirt] [PATCH v2 4/9] hostdev: Add virHostdevOnlyDetachPCIDevice()
This function mirrors virHostdevOnlyDetachPCIDevice().
The handling of active and inactive devices is updated and made more
explicit, which means virHostdevPreparePCIDevices() has to be
updated as well.
---
src/util/virhostdev.c | 87 ---
1 file changed, 61 insertions(+), 26 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index f40d636..6f14574 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -594,6 +594,56 @@ virHostdevOnlyReattachPCIDevice(virHostdevManagerPtr mgr,
return ret;
}
+/**
+ * virHostdevOnlyDetachPCIDevice:
+ * @mgr: hostdev manager
+ * @pci: PCI device to be detached
+ * @skipUnmanaged: whether to skip unmanaged devices
+ *
+ * Detach a PCI device from the host.
+ *
+ * This function only performs the base detach steps that are required
+ * regardless of whether the device is being attached to a domain or
+ * is simply being detached from the host for later use.
+ *
+ * The PCI related parts of @mgr (inactivePCIHostdevs, activePCIHostdevs)
+ * must have been locked beforehand using virObjectLock().
+ *
+ * Returns: 0 on success, <0 on failure
+ */
+static int
+virHostdevOnlyDetachPCIDevice(virHostdevManagerPtr mgr,
+ virPCIDevicePtr pci,
+ bool skipUnmanaged)
+{
+int ret = -1;
+
+/* Skip unmanaged devices if asked to do so */
+if (!virPCIDeviceGetManaged(pci) && skipUnmanaged) {
+VIR_DEBUG("Not detaching unmanaged PCI device %s",
+ virPCIDeviceGetName(pci));
+ret = 0;
+goto out;
+}
+
+VIR_DEBUG("Detaching PCI device %s", virPCIDeviceGetName(pci));
+if (virPCIDeviceDetach(pci,
+ mgr->activePCIHostdevs,
+ mgr->inactivePCIHostdevs) < 0) {
+virErrorPtr err = virGetLastError();
+VIR_ERROR(_("Failed to detach PCI device %s: %s"),
+ virPCIDeviceGetName(pci),
+ err ? err->message : _("unknown error"));
+virResetError(err);
+goto out;
+}
+
+ret = 0;
+
+ out:
+return ret;
+}
+
int
virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr,
const char *drv_name,
@@ -664,17 +714,8 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr
hostdev_mgr,
for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
-if (virPCIDeviceGetManaged(dev)) {
-VIR_DEBUG("Detaching managed PCI device %s",
- virPCIDeviceGetName(dev));
-if (virPCIDeviceDetach(dev,
- hostdev_mgr->activePCIHostdevs,
- hostdev_mgr->inactivePCIHostdevs) < 0)
-goto reattachdevs;
-} else {
-VIR_DEBUG("Not detaching unmanaged PCI device %s",
- virPCIDeviceGetName(dev));
-}
+if (virHostdevOnlyDetachPCIDevice(hostdev_mgr, dev, true) < 0)
+goto reattachdevs;
}
/* At this point, all devices are attached to the stub driver and have
@@ -704,23 +745,21 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr
hostdev_mgr,
last_processed_hostdev_vf = i;
}
-/* Loop 5: Now mark all the devices as active */
+/* Step 5: move all devices from the inactive list to the active list */
for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
+virPCIDevicePtr actual;
-VIR_DEBUG("Adding PCI device %s to active list",
+VIR_DEBUG("Removing PCI device %s from inactive list",
virPCIDeviceGetName(dev));
-if (virPCIDeviceListAdd(hostdev_mgr->activePCIHostdevs, dev) < 0)
+if (!(actual = virPCIDeviceListSteal(hostdev_mgr->inactivePCIHostdevs,
+ dev)))
goto inactivedevs;
-}
-
-/* Loop 6: Now remove the devices from inactive list. */
-for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
-virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
-VIR_DEBUG("Removing PCI device %s from inactive list",
- virPCIDeviceGetName(dev));
-virPCIDeviceListDel(hostdev_mgr->inactivePCIHostdevs, dev);
+VIR_DEBUG("Adding PCI device %s to active list",
+ virPCIDeviceGetName(actual));
+if (virPCIDeviceListAdd(hostdev_mgr->activePCIHostdevs, actual) < 0)
+goto inactivedevs;
}
/* Loop 7: Now set the used_by_domain of the device in
@@ -771,10 +810,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr
hostdev_mgr,
virPCIDeviceFree(dev);
}
-/* Loop 9: Now steal all the devices from pcidevs */
-while (virPCIDeviceListCount(pcidevs) > 0)
-virPCIDeviceListStealIndex(pcidevs, 0);
-
ret = 0;
goto
[libvirt] [PATCH v2 3/9] hostdev: Use common reattach code in virHostdevPCINodeDeviceReAttach()
This ensures the behavior for reattach is consistent, no matter how it
was triggered (eg. 'virsh nodedev-reattach', 'virsh detach-device' or
shutdown of a domain that is configured to use hostdevs).
---
src/util/virhostdev.c | 33 ++---
1 file changed, 30 insertions(+), 3 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index 586937e..f40d636 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -1637,10 +1637,24 @@ virHostdevPCINodeDeviceDetach(virHostdevManagerPtr
hostdev_mgr,
return ret;
}
+/**
+ * virHostdevPCINodeDeviceReAttach:
+ * @hostdev_mgr: hostdev manager
+ * @pci: PCI device
+ *
+ * Reattach a specific PCI device to the host.
+ *
+ * This function makes sure the device is not in use before reattaching it
+ * to the host; if the device has already been reattached to the host, the
+ * operation is considered successful.
+ *
+ * Returns: 0 on success, <0 on failure
+ */
int
virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr hostdev_mgr,
virPCIDevicePtr pci)
{
+virPCIDevicePtr actual;
struct virHostdevIsPCINodeDeviceUsedData data = {hostdev_mgr, NULL,
false};
int ret = -1;
@@ -1651,10 +1665,23 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr
hostdev_mgr,
if (virHostdevIsPCINodeDeviceUsed(virPCIDeviceGetAddress(pci), &data))
goto out;
-virPCIDeviceReattachInit(pci);
+/* We want this function to be idempotent, so if the device has already
+ * been removed from the inactive list (and is not active either, as
+ * per the check above) just return right away. We also need to retrieve
+ * the actual device from the inactive list because that's the one that
+ * contains state information such as the stub driver */
+if (!(actual = virPCIDeviceListFind(hostdev_mgr->inactivePCIHostdevs,
+pci))) {
+VIR_DEBUG("PCI device %s is already attached to the host",
+ virPCIDeviceGetName(pci));
+ret = 0;
+goto out;
+}
-if (virPCIDeviceReattach(pci, hostdev_mgr->activePCIHostdevs,
- hostdev_mgr->inactivePCIHostdevs) < 0)
+/* Reattach the device. We don't want to skip unmanaged devices in
+ * this case, because the user explicitly asked for the device to
+ * be reattached to the host driver */
+if (virHostdevOnlyReattachPCIDevice(hostdev_mgr, actual, false) < 0)
goto out;
ret = 0;
--
2.5.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 1/9] hostdev: Add virHostdevOnlyReattachPCIDevice()
This function replaces virHostdevReattachPCIDevice() and, unlike it,
does not perform list manipulation, leaving it to the calling function.
This means virHostdevReAttachPCIDevices() had to be updated to cope
with the updated requirements.
---
src/util/virhostdev.c | 136 +-
1 file changed, 90 insertions(+), 46 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index f31ad41..66629b4 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -526,6 +526,74 @@ virHostdevNetConfigRestore(virDomainHostdevDefPtr hostdev,
return ret;
}
+/**
+ * virHostdevOnlyReattachPCIDevice:
+ * @mgr: hostdev manager
+ * @pci: PCI device to be reattached
+ * @skipUnmanaged: whether to skip unmanaged devices
+ *
+ * Reattach a PCI device to the host.
+ *
+ * This function only performs the base reattach steps that are required
+ * regardless of whether the device is being detached from a domain or
+ * had been simply detached from the host earlier.
+ *
+ * @pci must have already been marked as inactive, and the PCI related
+ * parts of @mgr (inactivePCIHostdevs, activePCIHostdevs) must have been
+ * locked beforehand using virObjectLock().
+ *
+ * Returns: 0 on success, <0 on failure
+ */
+static int
+virHostdevOnlyReattachPCIDevice(virHostdevManagerPtr mgr,
+virPCIDevicePtr pci,
+bool skipUnmanaged)
+{
+virPCIDevicePtr actual;
+int ret = -1;
+
+/* Retrieve the actual device from the inactive list */
+if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci))) {
+VIR_DEBUG("PCI device %s is not marked as inactive",
+ virPCIDeviceGetName(pci));
+goto out;
+}
+
+/* Skip unmanaged devices if asked to do so */
+if (!virPCIDeviceGetManaged(actual) && skipUnmanaged) {
+VIR_DEBUG("Not reattaching unmanaged PCI device %s",
+ virPCIDeviceGetName(actual));
+ret = 0;
+goto out;
+}
+
+/* Wait for device cleanup if it is qemu/kvm */
+if (virPCIDeviceGetStubDriver(actual) == VIR_PCI_STUB_DRIVER_KVM) {
+int retries = 100;
+while (virPCIDeviceWaitForCleanup(actual, "kvm_assigned_device")
+ && retries) {
+usleep(100*1000);
+retries--;
+}
+}
+
+VIR_DEBUG("Reattaching PCI device %s", virPCIDeviceGetName(actual));
+if (virPCIDeviceReattach(actual, mgr->activePCIHostdevs,
+ mgr->inactivePCIHostdevs) < 0) {
+virErrorPtr err = virGetLastError();
+VIR_ERROR(_("Failed to reattach PCI device %s: %s"),
+ virPCIDeviceGetName(actual),
+ err ? err->message : _("unknown error"));
+virResetError(err);
+goto out;
+}
+
+ret = 0;
+
+ out:
+return ret;
+}
+
int
virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr,
const char *drv_name,
@@ -753,45 +821,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr
hostdev_mgr,
return ret;
}
-/*
- * Pre-condition: inactivePCIHostdevs & activePCIHostdevs
- * are locked
- */
-static void
-virHostdevReattachPCIDevice(virPCIDevicePtr dev, virHostdevManagerPtr mgr)
-{
-/* If the device is not managed and was attached to guest
- * successfully, it must have been inactive.
- */
-if (!virPCIDeviceGetManaged(dev)) {
-VIR_DEBUG("Adding unmanaged PCI device %s to inactive list",
- virPCIDeviceGetName(dev));
-if (virPCIDeviceListAdd(mgr->inactivePCIHostdevs, dev) < 0)
-virPCIDeviceFree(dev);
-return;
-}
-
-/* Wait for device cleanup if it is qemu/kvm */
-if (virPCIDeviceGetStubDriver(dev) == VIR_PCI_STUB_DRIVER_KVM) {
-int retries = 100;
-while (virPCIDeviceWaitForCleanup(dev, "kvm_assigned_device")
- && retries) {
-usleep(100*1000);
-retries--;
-}
-}
-
-VIR_DEBUG("Reattaching PCI device %s", virPCIDeviceGetName(dev));
-if (virPCIDeviceReattach(dev, mgr->activePCIHostdevs,
- mgr->inactivePCIHostdevs) < 0) {
-virErrorPtr err = virGetLastError();
-VIR_ERROR(_("Failed to re-attach PCI device: %s"),
- err ? err->message : _("unknown error"));
-virResetError(err);
-}
-virPCIDeviceFree(dev);
-}
-
/* @oldStateDir:
* For upgrade purpose: see virHostdevNetConfigRestore
*/
@@ -803,7 +832,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr
hostdev_mgr,
int nhostdevs,
const char *oldStateDir)
{
-virPCIDeviceListPtr pcidevs;
+virPCIDeviceListPtr pcidevs = NULL;
size_t i;
if (!nhostdevs)
@@ -848,11 +877,25 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr
hostdev_mgr,
continue;
}
}
+
[libvirt] [PATCH v2 8/9] pci: Phase out virPCIDeviceReattachInit()
The name is confusing, and the only use left is in a test case.
---
src/libvirt_private.syms | 1 -
src/util/virpci.c| 8
src/util/virpci.h| 1 -
tests/virpcitest.c | 5 -
4 files changed, 4 insertions(+), 11 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 6d221de..3f3dbe9 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1991,7 +1991,6 @@ virPCIDeviceListSteal;
virPCIDeviceListStealIndex;
virPCIDeviceNew;
virPCIDeviceReattach;
-virPCIDeviceReattachInit;
virPCIDeviceReset;
virPCIDeviceSetManaged;
virPCIDeviceSetRemoveSlot;
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 505c1f3..f56dbe6 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1771,14 +1771,6 @@ virPCIDeviceGetUsedBy(virPCIDevicePtr dev,
*dom_name = dev->used_by_domname;
}
-void virPCIDeviceReattachInit(virPCIDevicePtr pci)
-{
-pci->unbind_from_stub = true;
-pci->remove_slot = true;
-pci->reprobe = true;
-}
-
-
virPCIDeviceListPtr
virPCIDeviceListNew(void)
{
diff --git a/src/util/virpci.h b/src/util/virpci.h
index d1ac942..0debd7b 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -119,7 +119,6 @@ void virPCIDeviceSetRemoveSlot(virPCIDevice *dev,
unsigned int virPCIDeviceGetReprobe(virPCIDevicePtr dev);
void virPCIDeviceSetReprobe(virPCIDevice *dev,
bool reprobe);
-void virPCIDeviceReattachInit(virPCIDevice *dev);
virPCIDeviceListPtr virPCIDeviceListNew(void);
diff --git a/tests/virpcitest.c b/tests/virpcitest.c
index fb0c970..6dceef2 100644
--- a/tests/virpcitest.c
+++ b/tests/virpcitest.c
@@ -301,7 +301,10 @@ testVirPCIDeviceReattachSingle(const void *opaque)
if (!dev)
goto cleanup;
-virPCIDeviceReattachInit(dev);
+virPCIDeviceSetUnbindFromStub(dev, true);
+virPCIDeviceSetRemoveSlot(dev, true);
+virPCIDeviceSetReprobe(dev, true);
+
if (virPCIDeviceReattach(dev, NULL, NULL) < 0)
goto cleanup;
--
2.5.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 6/9] hostdev: Minor style adjustments
Mostly labels names and whitespace.
No functional changes.
---
src/util/virhostdev.c | 23 ++-
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index 5ded1e9..ab17547 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -838,9 +838,10 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr
hostdev_mgr,
}
cleanup:
+virObjectUnref(pcidevs);
virObjectUnlock(hostdev_mgr->activePCIHostdevs);
virObjectUnlock(hostdev_mgr->inactivePCIHostdevs);
-virObjectUnref(pcidevs);
+
return ret;
}
@@ -1671,7 +1672,7 @@ virHostdevPCINodeDeviceDetach(virHostdevManagerPtr
hostdev_mgr,
virObjectLock(hostdev_mgr->inactivePCIHostdevs);
if (virHostdevIsPCINodeDeviceUsed(virPCIDeviceGetAddress(pci), &data))
-goto out;
+goto cleanup;
/* We want this function to be idempotent, so if the device has already
* been added to the inactive list (and is not active, as per the check
@@ -1680,19 +1681,21 @@ virHostdevPCINodeDeviceDetach(virHostdevManagerPtr
hostdev_mgr,
VIR_DEBUG("PCI device %s is already detached from the host",
virPCIDeviceGetName(pci));
ret = 0;
-goto out;
+goto cleanup;
}
/* Detach the device. We don't want to skip unmanaged devices in
* this case, because the user explicitly asked for the device to
* be detached from the host driver */
if (virHostdevOnlyDetachPCIDevice(hostdev_mgr, pci, false) < 0)
-goto out;
+goto cleanup;
ret = 0;
- out:
+
+ cleanup:
virObjectUnlock(hostdev_mgr->inactivePCIHostdevs);
virObjectUnlock(hostdev_mgr->activePCIHostdevs);
+
return ret;
}
@@ -1722,7 +1725,7 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr
hostdev_mgr,
virObjectLock(hostdev_mgr->inactivePCIHostdevs);
if (virHostdevIsPCINodeDeviceUsed(virPCIDeviceGetAddress(pci), &data))
-goto out;
+goto cleanup;
/* We want this function to be idempotent, so if the device has already
* been removed from the inactive list (and is not active either, as
@@ -1734,19 +1737,21 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr
hostdev_mgr,
VIR_DEBUG("PCI device %s is already attached to the host",
virPCIDeviceGetName(pci));
ret = 0;
-goto out;
+goto cleanup;
}
/* Reattach the device. We don't want to skip unmanaged devices in
* this case, because the user explicitly asked for the device to
* be reattached to the host driver */
if (virHostdevOnlyReattachPCIDevice(hostdev_mgr, actual, false) < 0)
-goto out;
+goto cleanup;
ret = 0;
- out:
+
+ cleanup:
virObjectUnlock(hostdev_mgr->inactivePCIHostdevs);
virObjectUnlock(hostdev_mgr->activePCIHostdevs);
+
return ret;
}
--
2.5.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 9/9] pci: Add debug messages when unbinding from stub driver
Unbinding a PCI device from the stub driver can require several steps,
and it can be useful for debugging to be able to trace which of these
steps are performed and which are skipped for each device.
---
src/util/virpci.c | 22 ++
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index f56dbe6..51a3f88 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1106,26 +1106,37 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
if (!driver) {
/* The device is not bound to any driver and we are almost done. */
+VIR_DEBUG("PCI device %s is not bound to any driver", dev->name);
goto reprobe;
}
-if (!dev->unbind_from_stub)
+if (!dev->unbind_from_stub) {
+VIR_DEBUG("Unbind from stub skipped for PCI device %s", dev->name);
goto remove_slot;
+}
/* If the device isn't bound to a known stub, skip the unbind. */
if (virPCIStubDriverTypeFromString(driver) < 0 ||
-virPCIStubDriverTypeFromString(driver) == VIR_PCI_STUB_DRIVER_NONE)
+virPCIStubDriverTypeFromString(driver) == VIR_PCI_STUB_DRIVER_NONE) {
+VIR_DEBUG("Unbind from stub skipped for PCI device %s because of "
+ "unknown stub driver", dev->name);
goto remove_slot;
+}
-VIR_DEBUG("Found stub driver %s", driver);
+VIR_DEBUG("Found stub driver %s for PCI device %s", driver, dev->name);
+VIR_DEBUG("Unbinding PCI device %s", dev->name);
if (virPCIDeviceUnbind(dev) < 0)
goto cleanup;
dev->unbind_from_stub = false;
remove_slot:
-if (!dev->remove_slot)
+if (!dev->remove_slot) {
+VIR_DEBUG("Slot removal skipped for PCI device %s", dev->name);
goto reprobe;
+}
+
+VIR_DEBUG("Removing slot for PCI device %s", dev->name);
/* Xen's pciback.ko wants you to use remove_slot on the specific device */
if (!(path = virPCIDriverFile(driver, "remove_slot")))
@@ -1141,10 +1152,13 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
reprobe:
if (!dev->reprobe) {
+VIR_DEBUG("Reprobe skipped for PCI device %s", dev->name);
result = 0;
goto cleanup;
}
+VIR_DEBUG("Reprobing for PCI device %s", dev->name);
+
/* Trigger a re-probe of the device is not in the stub's dynamic
* ID table. If the stub is available, but 'remove_id' isn't
* available, then re-probing would just cause the device to be
--
2.5.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 0/9] PCI hostdev refactoring
Changes in v2: * Internal functions operate on a single device, don't pass around device lists for no reason * Use virHostdev prefix for internal functions * Split off style adjustments and comments updated to separate patches See the cover letter for v1[1] for details about the series. Cheers. [1] https://www.redhat.com/archives/libvir-list/2016-January/msg00835.html Andrea Bolognani (9): hostdev: Add virHostdevOnlyReattachPCIDevice() hostdev: Use common reattach code to rollback prepare errors hostdev: Use common reattach code in virHostdevPCINodeDeviceReAttach() hostdev: Add virHostdevOnlyDetachPCIDevice() hostdev: Use common detach code in virHostdevPCINodeDeviceDetach() hostdev: Minor style adjustments hostdev: Update comments pci: Phase out virPCIDeviceReattachInit() pci: Add debug messages when unbinding from stub driver src/libvirt_private.syms | 1 - src/util/virhostdev.c| 385 ++- src/util/virpci.c| 30 ++-- src/util/virpci.h| 1 - tests/virpcitest.c | 5 +- 5 files changed, 272 insertions(+), 150 deletions(-) -- 2.5.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 2/9] hostdev: Use common reattach code to rollback prepare errors
---
src/util/virhostdev.c | 14 +-
1 file changed, 1 insertion(+), 13 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index 66629b4..586937e 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -799,19 +799,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr
hostdev_mgr,
for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
-if (virPCIDeviceGetManaged(dev)) {
-/* NB: This doesn't actually re-bind to original driver, just
- * unbinds from the stub driver
- */
-VIR_DEBUG("Reattaching managed PCI device %s",
- virPCIDeviceGetName(dev));
-ignore_value(virPCIDeviceReattach(dev,
- hostdev_mgr->activePCIHostdevs,
-
hostdev_mgr->inactivePCIHostdevs));
-} else {
-VIR_DEBUG("Not reattaching unmanaged PCI device %s",
- virPCIDeviceGetName(dev));
-}
+ignore_value(virHostdevOnlyReattachPCIDevice(hostdev_mgr, dev, true));
}
cleanup:
--
2.5.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 5/9] hostdev: Use common detach code in virHostdevPCINodeDeviceDetach()
This ensures the behavior for detach is consistent, no matter how it
was triggered (eg. 'virsh nodedev-detach', 'virsh attach-device' or
startup of a domain that is configured to use hostdevs).
---
src/util/virhostdev.c | 28 ++--
1 file changed, 26 insertions(+), 2 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index 6f14574..5ded1e9 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -1646,6 +1646,19 @@ virHostdevReAttachSCSIDevices(virHostdevManagerPtr
hostdev_mgr,
virObjectUnlock(hostdev_mgr->activeSCSIHostdevs);
}
+/**
+ * virHostdevPCINodeDeviceDetach:
+ * @hostdev_mgr: hostdev manager
+ * @pci: PCI device
+ *
+ * Detach a specific PCI device from the host.
+ *
+ * This function makes sure the device is not in use before detaching it
+ * from the host; if the device has already been detached from the host,
+ * the operation is considered successful.
+ *
+ * Returns: 0 on success, <0 on failure
+ */
int
virHostdevPCINodeDeviceDetach(virHostdevManagerPtr hostdev_mgr,
virPCIDevicePtr pci)
@@ -1660,11 +1673,22 @@ virHostdevPCINodeDeviceDetach(virHostdevManagerPtr
hostdev_mgr,
if (virHostdevIsPCINodeDeviceUsed(virPCIDeviceGetAddress(pci), &data))
goto out;
-if (virPCIDeviceDetach(pci, hostdev_mgr->activePCIHostdevs,
- hostdev_mgr->inactivePCIHostdevs) < 0) {
+/* We want this function to be idempotent, so if the device has already
+ * been added to the inactive list (and is not active, as per the check
+ * above) just return right away */
+if (virPCIDeviceListFind(hostdev_mgr->inactivePCIHostdevs, pci)) {
+VIR_DEBUG("PCI device %s is already detached from the host",
+ virPCIDeviceGetName(pci));
+ret = 0;
goto out;
}
+/* Detach the device. We don't want to skip unmanaged devices in
+ * this case, because the user explicitly asked for the device to
+ * be detached from the host driver */
+if (virHostdevOnlyDetachPCIDevice(hostdev_mgr, pci, false) < 0)
+goto out;
+
ret = 0;
out:
virObjectUnlock(hostdev_mgr->inactivePCIHostdevs);
--
2.5.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Call for mentors and project ideas for Google Summer of Code 2016
The QEMU wiki page for Google Summer of Code 2016 is now available here: http://qemu-project.org/Google_Summer_of_Code_2016 QEMU will apply for Google Summer of Code 2016 (https://g.co/gsoc/). If QEMU is accepted there will be funding for students to work on 12-week full-time open source projects remotely from May to August 2016. QEMU provides a mentor for each student who gives advice and evaluates their progress. If you have a project idea, especially if you are a regular contributor to QEMU and are willing to mentor this summer, please go to this wiki page and fill out the project idea template: http://qemu-project.org/Google_Summer_of_Code_2016 The project ideas list is part of the application so that QEMU can participate in GSoC. It's useful to have your project ideas on the wiki by February 8th 2016. If you have any questions about project ideas or QEMU applying to GSoC, please reply to this thread. Thanks, Stefan -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] virt-host-validate: Fix error level for user namespace check
On Mon, 2016-01-25 at 16:40 +0100, Pavel Hrdina wrote: > On Mon, Jan 25, 2016 at 08:23:22AM +0100, Michal Privoznik wrote: > > > From the code it seems to me that we need user namespace iff > > s/>From/From/ This is fun. The leading ">" is an artifact of escaping, to prevent (poorly written?) MUAs from mistaking a line starting with "From" in the body for the From: header. You would think 'git format-patch', and most importantly 'git am', would be clever enough to take this into account, but alas they don't and just pass the commit message through. > s/iff/if/ Not a typo: "iff" is shortand for "if and only if", and by doing a quick search in the git log you'll find that there are quite a few previous instances. Mostly introduced by Michal :) Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] storage: zfs: enable on Linux
On 01/02/2016 09:25 PM, Roman Bogorodskiy wrote:
> ZFS-on-Linux implementation of ZFS starting with version 0.6.4
> contains all the features we use, so there's no reason to limit
> libvirt ZFS storage backend to FreeBSD only.
>
> There's still one difference between these implementations: ZFS on
> FreeBSD requires to set 'volmode' option to 'dev' when creating
> volumes, while ZFS-on-Linux does not need that.
>
> Handle it by checking if 'volmode' option is needed by parsing usage
> information of the 'zfs get' command.
> ---
> configure.ac | 8 --
> src/storage/storage_backend_zfs.c | 51
> +--
> 2 files changed, 49 insertions(+), 10 deletions(-)
>
Caveat - I know very little about zfs, FreeBSD... But it's languishing
so...
> diff --git a/configure.ac b/configure.ac
> index a566f5b..d768341 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -2005,14 +2005,6 @@ if test "$with_storage_gluster" = "yes"; then
> fi
> AM_CONDITIONAL([WITH_STORAGE_GLUSTER], [test "$with_storage_gluster" =
> "yes"])
>
> -if test "$with_storage_zfs" = "check"; then
> -with_storage_zfs=$with_freebsd
> -fi
> -
> -if test "$with_storage_zfs" = "yes" && test "$with_freebsd" = "no"; then
> -AC_MSG_ERROR([The ZFS storage driver can be enabled on FreeBSD only.])
> -fi
> -
> if test "$with_storage_zfs" = "yes" ||
> test "$with_storage_zfs" = "check"; then
>AC_PATH_PROG([ZFS], [zfs], [], [$PATH:/sbin:/usr/sbin])
So it would seem this hunk and patch 2 would be more related. That is
the virsh WITH_STORAGE_ZFS. The order of the patches is up to you.
> diff --git a/src/storage/storage_backend_zfs.c
> b/src/storage/storage_backend_zfs.c
> index cb2662a..6bf7963 100644
> --- a/src/storage/storage_backend_zfs.c
> +++ b/src/storage/storage_backend_zfs.c
> @@ -39,6 +39,47 @@ VIR_LOG_INIT("storage.storage_backend_zfs");
> * for size, show just a number instead of 2G etc
> */
>
> +/**
> + * virStorageBackendZFSVolModeNeeded:
> + *
> + * Checks if it's necessary to specify 'volmode' (i.e. that
> + * we're working with BSD ZFS implementation).
> + *
> + * Returns 1 if 'volmode' is need, 0 if not needed, -1 on error
> + */
Does volmode only exist in/on one environment? FreeBSD?
> +static int
> +virStorageBackendZFSVolModeNeeded(void)
> +{
> +virCommandPtr cmd = NULL;
> +int ret = -1, exit = -1;
> +char *error = NULL;
> +
> +/* 'zfs get' without arguments prints out
> + * usage information to stderr, including
> + * list of supported options, and exits with
> + * exit code 2
> + */
> +cmd = virCommandNewArgList(ZFS, "get", NULL);
> +virCommandAddEnvString(cmd, "LC_ALL=C");
> +virCommandSetErrorBuffer(cmd, &error);
Why isn't this something like zfs get volmode $target? (examples that
google returns are tank/). And I would assume $target would
be pool->def->source.name & vol->name (from the caller).
It would seem that all that's being checking is whether the variable
exists "somewhere" (in "some" $target) as opposed to the specific one.
Also from what I read volmode can have many settings.
> +
> +ret = virCommandRun(cmd, &exit);
> +if ((ret < 0) || (exit != 2)) {
> +VIR_WARN("Command 'zfs get' either failed "
> + "to run or exited with unexpected status");
> +goto cleanup;
> +}
> +
> +if (strstr(error, " volmode "))
> +ret = 1;
> +else
> +ret = 0;
> +
> + cleanup:
> +virCommandFree(cmd);
> +VIR_FREE(error);
> +return ret;
> +}
>
> static int
> virStorageBackendZFSCheckPool(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED,
> @@ -258,6 +299,7 @@ virStorageBackendZFSCreateVol(virConnectPtr conn
> ATTRIBUTE_UNUSED,
> {
> virCommandPtr cmd = NULL;
> int ret = -1;
> +int volmode_needed = -1;
>
> vol->type = VIR_STORAGE_VOL_BLOCK;
>
> @@ -273,6 +315,9 @@ virStorageBackendZFSCreateVol(virConnectPtr conn
> ATTRIBUTE_UNUSED,
> if (VIR_STRDUP(vol->key, vol->target.path) < 0)
> goto cleanup;
>
> +volmode_needed = virStorageBackendZFSVolModeNeeded();
> +if (volmode_needed < 0)
> +goto cleanup;
> /**
> * $ zfs create -o volmode=dev -V 10240K test/volname
> *
> @@ -281,8 +326,10 @@ virStorageBackendZFSCreateVol(virConnectPtr conn
> ATTRIBUTE_UNUSED,
> * will lookup vfs.zfs.vol.mode sysctl value
> * -V -- tells to create a volume with the specified size
> */
> -cmd = virCommandNewArgList(ZFS, "create", "-o", "volmode=dev",
> - "-V", NULL);
> +cmd = virCommandNewArgList(ZFS, "create", NULL);
Why not just encase this in
#if WITH_FREEBSD
virCommandAddArgList(cmd, "-o", "volmode=dev", NULL);
#endif
Although perhaps there's some that may not like that approach... It just
seems 'safer' than a get command which searches the output for the
parameter being present. Who knows what implica
Re: [libvirt] [PATCH 1/2] storage: zfs: enable on Linux
John Ferlan wrote:
>
>
> On 01/02/2016 09:25 PM, Roman Bogorodskiy wrote:
> > ZFS-on-Linux implementation of ZFS starting with version 0.6.4
> > contains all the features we use, so there's no reason to limit
> > libvirt ZFS storage backend to FreeBSD only.
> >
> > There's still one difference between these implementations: ZFS on
> > FreeBSD requires to set 'volmode' option to 'dev' when creating
> > volumes, while ZFS-on-Linux does not need that.
> >
> > Handle it by checking if 'volmode' option is needed by parsing usage
> > information of the 'zfs get' command.
> > ---
> > configure.ac | 8 --
> > src/storage/storage_backend_zfs.c | 51
> > +--
> > 2 files changed, 49 insertions(+), 10 deletions(-)
> >
>
> Caveat - I know very little about zfs, FreeBSD... But it's languishing
> so...
Heh. Thanks for stepping in! :)
> > diff --git a/configure.ac b/configure.ac
> > index a566f5b..d768341 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -2005,14 +2005,6 @@ if test "$with_storage_gluster" = "yes"; then
> > fi
> > AM_CONDITIONAL([WITH_STORAGE_GLUSTER], [test "$with_storage_gluster" =
> > "yes"])
> >
> > -if test "$with_storage_zfs" = "check"; then
> > -with_storage_zfs=$with_freebsd
> > -fi
> > -
> > -if test "$with_storage_zfs" = "yes" && test "$with_freebsd" = "no"; then
> > -AC_MSG_ERROR([The ZFS storage driver can be enabled on FreeBSD only.])
> > -fi
> > -
> > if test "$with_storage_zfs" = "yes" ||
> > test "$with_storage_zfs" = "check"; then
> >AC_PATH_PROG([ZFS], [zfs], [], [$PATH:/sbin:/usr/sbin])
>
> So it would seem this hunk and patch 2 would be more related. That is
> the virsh WITH_STORAGE_ZFS. The order of the patches is up to you.
Actually, these two patches are unrelated. I should have been added the
virsh part when I added ZFS driver in the first place, but I forgot
about it at that time.
I remembered about it when I was testing my configure.ac changes on
Linux and saw that virsh -V output does not change.
> > diff --git a/src/storage/storage_backend_zfs.c
> > b/src/storage/storage_backend_zfs.c
> > index cb2662a..6bf7963 100644
> > --- a/src/storage/storage_backend_zfs.c
> > +++ b/src/storage/storage_backend_zfs.c
> > @@ -39,6 +39,47 @@ VIR_LOG_INIT("storage.storage_backend_zfs");
> > * for size, show just a number instead of 2G etc
> > */
> >
> > +/**
> > + * virStorageBackendZFSVolModeNeeded:
> > + *
> > + * Checks if it's necessary to specify 'volmode' (i.e. that
> > + * we're working with BSD ZFS implementation).
> > + *
> > + * Returns 1 if 'volmode' is need, 0 if not needed, -1 on error
> > + */
>
> Does volmode only exist in/on one environment? FreeBSD?
>
> > +static int
> > +virStorageBackendZFSVolModeNeeded(void)
> > +{
> > +virCommandPtr cmd = NULL;
> > +int ret = -1, exit = -1;
> > +char *error = NULL;
> > +
> > +/* 'zfs get' without arguments prints out
> > + * usage information to stderr, including
> > + * list of supported options, and exits with
> > + * exit code 2
> > + */
> > +cmd = virCommandNewArgList(ZFS, "get", NULL);
> > +virCommandAddEnvString(cmd, "LC_ALL=C");
> > +virCommandSetErrorBuffer(cmd, &error);
>
> Why isn't this something like zfs get volmode $target? (examples that
> google returns are tank/). And I would assume $target would
> be pool->def->source.name & vol->name (from the caller).
>
> It would seem that all that's being checking is whether the variable
> exists "somewhere" (in "some" $target) as opposed to the specific one.
No, the goal is to check if "volmode" option is supported by the ZFS
implementation we're using. So we don't need any specific information,
just parse usage/help information that 'zfs get' prints out.
In other words, 'zfs get' does not try to receive any information about
the actual pools, volumes etc, its output is always same.
> Also from what I read volmode can have many settings.
Yeah, though if it's available, we always set it to 'dev', and if it's
not available, we just don't care. No need to worry about other possible
settings.
> > +
> > +ret = virCommandRun(cmd, &exit);
> > +if ((ret < 0) || (exit != 2)) {
> > +VIR_WARN("Command 'zfs get' either failed "
> > + "to run or exited with unexpected status");
> > +goto cleanup;
> > +}
> > +
> > +if (strstr(error, " volmode "))
> > +ret = 1;
> > +else
> > +ret = 0;
> > +
> > + cleanup:
> > +virCommandFree(cmd);
> > +VIR_FREE(error);
> > +return ret;
> > +}
> >
> > static int
> > virStorageBackendZFSCheckPool(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED,
> > @@ -258,6 +299,7 @@ virStorageBackendZFSCreateVol(virConnectPtr conn
> > ATTRIBUTE_UNUSED,
> > {
> > virCommandPtr cmd = NULL;
> > int ret = -1;
> > +int volmode_needed = -1;
> >
> > vol->type = VIR_STORAGE_VOL_BLOCK;
> >
> > @@ -273,6 +315,9 @@
Re: [libvirt] [PATCHv2] util: keep/use a bitmap of in-use macvtap devices
On 01/25/2016 07:16 AM, Pavel Hrdina wrote:
On Fri, Jan 22, 2016 at 12:52:27PM -0500, Laine Stump wrote:
This patch creates two bitmaps, one for macvlan devicenames and one
s/devicenames/device names/
[...]
Done.
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
[...]
+/**
+ * virNetDevMacVLanReserveName:
+ *
+ * @name: already-known name of device
+ * @quietFail: don't log an error if this name is already in-use
+ *
+ * Extract the device type and id from a macvtap/macvlan device name
+ * and mark the appropriate position as in-use in the appropriate
+ * bitmap.
+ *
+ * returns 0 on success, -1 on failure, -2 if the name doesn't fit the
auto-pattern
Long line, would be nice to wrap it. And the return 0 isn't true, because
virNetDevMacVLanReserveID() returns ID on success.
The danger of documenting the function when it's initially written,
rather than doing it as an afterthought after everything is wrapped up :-).
+ */
+int
+virNetDevMacVLanReserveName(const char *name, bool quietFail)
+{
+unsigned int id;
+unsigned int flags = 0;
+const char *idstr = NULL;
+
+if (virNetDevMacVLanInitialize() < 0)
+ return -1;
+
+if (STRPREFIX(name, MACVTAP_NAME_PREFIX)) {
+idstr = name + strlen(MACVTAP_NAME_PREFIX);
+flags |= VIR_NETDEV_MACVLAN_CREATE_WITH_TAP;
+} else if (STRPREFIX(name, MACVLAN_NAME_PREFIX)) {
+idstr = name + strlen(MACVLAN_NAME_PREFIX);
+} else {
+return -2;
+}
+
+if (virStrToLong_ui(idstr, NULL, 10, &id) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("couldn't get id value from macvtap device name %s"),
+ name);
+return -1;
+}
+return virNetDevMacVLanReserveID(id, flags, quietFail, false);
+}
[...]
const char *type = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ?
"macvtap" : "macvlan";
-const char *prefix = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ?
-MACVTAP_NAME_PREFIX : MACVLAN_NAME_PREFIX;
Maybe use the MACV(TAP|LAN)_NAME_PREFIX instead of the strings for *type.
Yeah, I wonder why it wasn't like that from the beginning (and for that
matter why they had a separate char* for prefix and type even though
they were identical. Maybe they were anticipating the day when libvirt
might change the names it uses for the devices? Dunno. Anyway, I've
changed it as you suggest.
const char *pattern = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ?
MACVTAP_NAME_PATTERN : MACVLAN_NAME_PATTERN;
-int c, rc;
+int rc, reservedID = -1;
char ifname[IFNAMSIZ];
int retries, do_retry = 0;
uint32_t macvtapMode;
-const char *cr_ifname = NULL;
+const char *ifnameCreated = NULL;
int ret;
int vf = -1;
bool vnet_hdr = flags & VIR_NETDEV_MACVLAN_VNET_HDR;
[...]
+retries = MACVLAN_MAX_ID + 1;
+while (!ifnameCreated && retries) {
virMutexLock(&virNetDevMacVLanCreateMutex);
-for (c = 0; c < 8192; c++) {
-snprintf(ifname, sizeof(ifname), pattern, c);
-if ((ret = virNetDevExists(ifname)) < 0) {
-virMutexUnlock(&virNetDevMacVLanCreateMutex);
+reservedID = virNetDevMacVLanReserveID(reservedID, flags, false, true);
+if (reservedID < 0) {
+virMutexUnlock(&virNetDevMacVLanCreateMutex);
+return -1;
+}
+snprintf(ifname, sizeof(ifname), pattern, reservedID);
Since you're changing this, snprintf returns -1 in case of error.
Well, the man page says -1 is returned by the *printf functions "if an
output error is encountered", which would include an I/O error (moot for
snprintf() since it doesn't do any I/O) or a bad format string (but
we're calling it with a literal format string that only includes a
single %d, so it is known to be good). Checking the returned length is
pointless too, since by definition the longest possible result of
macvtap%d where the %d is guaranteed between 0 and 8192 is
"macvtap8192", which is 12 characters - safely below the limit of
IFNAMSIZ (16).
ACK with the issues fixed.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] virsh: fix cpu-stats command output format issue
After commit 57177f1, the cpu-stats command format change to:
CPU0:
cpu_time 14401.507878990 seconds
vcpu_time14378732785511
vcpu_time is not user friendly. After this patch, it will
change back:
CPU0:
cpu_time 14401.507878990 seconds
vcpu_time14378.732785511 seconds
https://bugzilla.redhat.com/show_bug.cgi?id=1301807
Signed-off-by: Luyao Huang
---
tools/virsh-domain.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 1089a4d..c2146d2 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -7243,6 +7243,7 @@ vshCPUStatsPrintField(vshControl *ctl,
{
vshPrint(ctl, "\t%-12s ", param->field);
if ((STREQ(param->field, VIR_DOMAIN_CPU_STATS_CPUTIME) ||
+ STREQ(param->field, VIR_DOMAIN_CPU_STATS_VCPUTIME) ||
STREQ(param->field, VIR_DOMAIN_CPU_STATS_USERTIME) ||
STREQ(param->field, VIR_DOMAIN_CPU_STATS_SYSTEMTIME)) &&
param->type == VIR_TYPED_PARAM_ULLONG) {
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] libxl: support assignment from a pool of SRIOV VFs
Add codes to support creating domain with network defition of assigning
SRIOV VF from a pool. And fix hot plug and unplug SRIOV VF under this
kind of network defition.
Signed-off-by: Chunyan Liu
---
src/libxl/libxl_conf.c | 5 +++--
src/libxl/libxl_domain.c | 48 +++-
src/libxl/libxl_driver.c | 12
tests/Makefile.am| 3 +++
4 files changed, 65 insertions(+), 3 deletions(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 6320421..f50c68a 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1260,7 +1260,8 @@ libxlMakeNicList(virDomainDefPtr def,
libxl_domain_config *d_config)
return -1;
for (i = 0; i < nnics; i++) {
-if (l_nics[i]->type == VIR_DOMAIN_NET_TYPE_HOSTDEV)
+if (l_nics[i]->type == VIR_DOMAIN_NET_TYPE_HOSTDEV ||
+l_nics[i]->data.network.actual->type ==
VIR_DOMAIN_NET_TYPE_HOSTDEV)
continue;
if (libxlMakeNic(def, l_nics[i], &x_nics[nvnics]))
@@ -1278,7 +1279,7 @@ libxlMakeNicList(virDomainDefPtr def,
libxl_domain_config *d_config)
VIR_SHRINK_N(x_nics, nnics, nnics - nvnics);
d_config->nics = x_nics;
-d_config->num_nics = nnics;
+d_config->num_nics = nvnics;
return 0;
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index cf5c9f6..9bf7a5a 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -35,6 +35,7 @@
#include "virstring.h"
#include "virtime.h"
#include "locking/domain_lock.h"
+#include "network/bridge_driver.h"
#define VIR_FROM_THIS VIR_FROM_LIBXL
@@ -314,7 +315,7 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
virDomainHostdevSubsysPCIPtr pcisrc;
if (dev->type == VIR_DOMAIN_DEVICE_NET)
-hostdev = &(dev->data.net)->data.hostdev.def;
+hostdev = &dev->data.net->data.hostdev.def;
else
hostdev = dev->data.hostdev;
pcisrc = &hostdev->source.subsys.u.pci;
@@ -916,6 +917,48 @@ libxlConsoleCallback(libxl_ctx *ctx, libxl_event *ev, void
*for_callback)
libxl_event_free(ctx, ev);
}
+static int
+libxlNetworkPrepareDevices(virDomainDefPtr def)
+{
+int ret = -1;
+size_t i;
+
+for (i = 0; i < def->nnets; i++) {
+virDomainNetDefPtr net = def->nets[i];
+int actualType;
+
+/* If appropriate, grab a physical device from the configured
+ * network's pool of devices, or resolve bridge device name
+ * to the one defined in the network definition.
+ */
+if (networkAllocateActualDevice(def, net) < 0)
+goto cleanup;
+
+actualType = virDomainNetGetActualType(net);
+if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV &&
+net->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
+/* Each type='hostdev' network device must also have a
+ * corresponding entry in the hostdevs array. For netdevs
+ * that are hardcoded as type='hostdev', this is already
+ * done by the parser, but for those allocated from a
+ * network / determined at runtime, we need to do it
+ * separately.
+ */
+virDomainHostdevDefPtr hostdev = virDomainNetGetActualHostdev(net);
+virDomainHostdevSubsysPCIPtr pcisrc =
&hostdev->source.subsys.u.pci;
+
+if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
+hostdev->source.subsys.type ==
VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
+pcisrc->backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN;
+
+if (virDomainHostdevInsert(def, hostdev) < 0)
+goto cleanup;
+}
+}
+ret = 0;
+ cleanup:
+return ret;
+}
/*
* Start a domain through libxenlight.
@@ -992,6 +1035,9 @@ libxlDomainStart(libxlDriverPrivatePtr driver,
virDomainObjPtr vm,
vm, true) < 0)
goto cleanup;
+if (libxlNetworkPrepareDevices(vm->def) < 0)
+goto cleanup;
+
if (libxlBuildDomainConfig(driver->reservedGraphicsPorts, vm->def,
cfg->ctx, &d_config) < 0)
goto cleanup;
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 4a9134e..6aca042 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -3047,6 +3047,12 @@ libxlDomainAttachHostPCIDevice(libxlDriverPrivatePtr
driver,
libxl_device_pci_init(&pcidev);
+/* For those allocated from a network pool/ determined at runtime, it's
+ * not handled by libxlDomainDeviceDefPostParse as hostdev, we need to
+ * set backend correctly.
+ */
+pcisrc->backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN;
+
if (virDomainHostdevFind(vm->def, hostdev, &found) >= 0) {
virReportError(VIR_ERR_OPERATION_FAILED,
_("target pci device %.4x:%.2x:%.2x.%.1x already
exists"),
@@ -3388,6 +3394,12 @@ libxlDomainDetachHostPCIDev
