Re: [libvirt] [PATCH] network: Add support for configuring dhcp lease time

[Daniel P. Berrange]

On Wed, Sep 28, 2016 at 06:03:21PM +0200, Martin Wilck wrote:
> On Wed, 2016-09-28 at 16:41 +0100, Daniel P. Berrange wrote:
> 
> > Anyone who cares about VMs having the same IP address after resuming
> > from suspend *must* configure persistent DHCP mappings for their
> > guests.
> > Relying on automatic assignment will always fail in the end, may be
> > not immediately, may be not often, but it *will* fail at some point,
> > usually at the worst possible time.
> 
> I believe that setting "dhcp-authoritative" will be a major improvement
> for many setups. Without it, VMs are *never* able to reacquire their
> expired lease. With it, reacquiring the lease would work most of the
> time (as far as I'm concerned, practically always). I reckon the
> dnsmasq man page recommends it for a reason "when dnsmasq is definitely
> the only DHCP server on a network".

That certainly matches our deployment model for dnsmasq, so unless
there's a obvious downside to it, it seems reasonable to add that.


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 4/4] vbox: update rest of the code to for prior changes.

[Dawid Zamirski]

This commit should be squashed to previous one. Keeping it separate for
code-review purposes only as it's mostly noise due to changes of the
struct definitions in previous patch:

* vboxPrivate (aka old vboxGlobalData *data argument) no longer needs
  to be passed around as g_pVBoxGlobalData is the only one that has
  pFuncs reference.

* for vbox 3.x, the even handling code was updated to use
  VirtuaBoxCallback struct (that is modeled after IVirtualBoxCallback)
  but includes additional members to carry contextual info so that
  g_pVBoxGlobalData does not have to be used in the callbacks. The
  additional members keep virConnectPtr reference and callback
  reference counter (taken out from old vboxGlobalData)

P.S. VBox 3.x callback handling code updates were only compile-tested
as VBOX 3 is no longer supported by upstream and does not work on
any reasonably modern linux distro.
---
 src/vbox/vbox_common.c| 101 +++--
 src/vbox/vbox_common.h|  32 +++---
 src/vbox/vbox_network.c   |  31 +++--
 src/vbox/vbox_tmpl.c  | 258 ++
 src/vbox/vbox_uniformed_api.h |  49 
 5 files changed, 206 insertions(+), 265 deletions(-)

diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 1728275..3c98a34 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -520,24 +520,12 @@ vboxDomainSave(virDomainPtr dom, const char *path 
ATTRIBUTE_UNUSED)
 return ret;
 }
 
-static void vboxDriverLock(vboxPrivate *data)
-{
-virMutexLock(>lock);
-}
-
-static void vboxDriverUnlock(vboxPrivate *data)
-{
-virMutexUnlock(>lock);
-}
-
 static int vboxConnectGetVersion(virConnectPtr conn, unsigned long *version)
 {
 vboxPrivate *data = conn->privateData;
 VIR_DEBUG("%s: in vboxGetVersion", conn->driver->name);
 
-vboxDriverLock(data);
 *version = data->version;
-vboxDriverUnlock(data);
 
 return 0;
 }
@@ -600,9 +588,7 @@ static char *vboxConnectGetCapabilities(virConnectPtr conn)
 if (!data->vboxObj)
 return ret;
 
-vboxDriverLock(data);
 ret = virCapabilitiesFormatXML(data->caps);
-vboxDriverUnlock(data);
 
 return ret;
 }
@@ -1712,7 +1698,7 @@ vboxAttachDisplay(virDomainDefPtr def, vboxPrivate *data, 
IMachine *machine)
 }
 
 static void
-vboxAttachUSB(virDomainDefPtr def, vboxPrivate *data, IMachine *machine)
+vboxAttachUSB(virDomainDefPtr def, IMachine *machine)
 {
 IUSBCommon *USBCommon = NULL;
 size_t i = 0;
@@ -1816,7 +1802,7 @@ vboxAttachUSB(virDomainDefPtr def, vboxPrivate *data, 
IMachine *machine)
 }
 
 static void
-vboxAttachSharedFolder(virDomainDefPtr def, vboxPrivate *data, IMachine 
*machine)
+vboxAttachSharedFolder(virDomainDefPtr def, IMachine *machine)
 {
 size_t i;
 PRUnichar *nameUtf16;
@@ -1957,8 +1943,8 @@ vboxDomainDefineXMLFlags(virConnectPtr conn, const char 
*xml, unsigned int flags
 vboxAttachParallel(def, data, machine);
 vboxAttachVideo(def, machine);
 vboxAttachDisplay(def, data, machine);
-vboxAttachUSB(def, data, machine);
-vboxAttachSharedFolder(def, data, machine);
+vboxAttachUSB(def, machine);
+vboxAttachSharedFolder(def, machine);
 
 /* Save the machine settings made till now and close the
  * session. also free up the mchiid variable used.
@@ -2020,7 +2006,7 @@ detachDevices_common(vboxPrivate *data, vboxIIDUnion 
*iidu)
 if (NS_SUCCEEDED(rc)) {
 rc = gVBoxAPI.UISession.GetMachine(data->vboxSession, );
 if (NS_SUCCEEDED(rc) && machine) {
-gVBoxAPI.detachDevices(data, machine, hddcnameUtf16);
+gVBoxAPI.detachDevices(machine, hddcnameUtf16);
 gVBoxAPI.UIMachine.SaveSettings(machine);
 }
 gVBoxAPI.UISession.Close(data->vboxSession);
@@ -2983,7 +2969,7 @@ static int vboxDomainGetMaxVcpus(virDomainPtr dom)
 }
 
 static void
-vboxHostDeviceGetXMLDesc(vboxPrivate *data, virDomainDefPtr def, IMachine 
*machine)
+vboxHostDeviceGetXMLDesc(virDomainDefPtr def, IMachine *machine)
 {
 IUSBCommon *USBCommon = NULL;
 PRBool enabled = PR_FALSE;
@@ -3408,7 +3394,7 @@ vboxDumpDisplay(virDomainDefPtr def, vboxPrivate *data, 
IMachine *machine)
 }
 
 static void
-vboxDumpSharedFolders(virDomainDefPtr def, vboxPrivate *data, IMachine 
*machine)
+vboxDumpSharedFolders(virDomainDefPtr def, IMachine *machine)
 {
 /* shared folders */
 vboxArray sharedFolders = VBOX_ARRAY_INITIALIZER;
@@ -3469,7 +3455,7 @@ vboxDumpSharedFolders(virDomainDefPtr def, vboxPrivate 
*data, IMachine *machine)
 }
 
 static void
-vboxDumpNetwork(virDomainDefPtr def, vboxPrivate *data, IMachine *machine, 
PRUint32 networkAdapterCount)
+vboxDumpNetwork(virDomainDefPtr def, IMachine *machine, PRUint32 
networkAdapterCount)
 {
 PRUint32 netAdpIncCnt = 0;
 size_t i = 0;
@@ -3648,7 +3634,7 @@ vboxDumpAudio(virDomainDefPtr def, vboxPrivate *data 
ATTRIBUTE_UNUSED,
 }
 
 static void
-vboxDumpSerial(virDomainDefPtr def, vboxPrivate *data, 

[libvirt] [PATCH 3/4] vbox: change API (un)initialization logic.

[Dawid Zamirski]

Since VBOX API initialization method (pfnComInitialize) is not
thread-safe and must be called from the primary thread, calling it in
every vboxConnectOpen (as we used to do) leads to segmentation
faults when multiple connections are in use. Therefore the initalization
and unitialization logic has been changed as the following:

* _registerGlobalData allocates g_pVBoxGlobalData (only when not
  already allocated) and calls VBOX's pfnComInitialize to grab
  references to ISession and IVirtualBox objects. This ensures that
  pfnComInitialize is called when the first connection is established.

* _pfnInitialize sets up the virConnectPtr->privateData (aka
  vboxPrivateData) for each connection by copying references to
  ISession and IVirtualBox from g_pVBoxGlobalData so that the rest of
  the driver API can use it without referencing the global. Each time
  this happens, a conntionCount is incremented on g_pVBoxGlobalData to
  keep track of when it's safe to call pfnComUnitialize. One of the
  reasons for existence of per-connection vboxPrivateData rather than
  completely relying on vboxGlobalData, is that more modern VBOX APIs
  provide pfnClientInitialize (since 4.2.20 and 4.3.4) and
  pfnClientThreadInitialize (5.0+) that allow to create multiple
  instances of ISession so if the code ever gets ported to support
  those newer APIs it should create much less diff noise as all API
  implementations are already updated to assume per-connection
  ISession/IVirutalBox instances.

* _pfnUnitilalize decrements connectionCount in g_pVBoxGlobalData and
  once it is down to 0, it calls pfnComUnitialize and
  g_pVBoxGlobalData if free'd. This ensures that pfnComInitialize and
  pfnComUnitialize pair is called only once, even when multiple
  concurrent connections are in use.
---
 src/vbox/vbox_common.c|  32 
 src/vbox/vbox_tmpl.c  | 113 --
 src/vbox/vbox_uniformed_api.h |  57 +++--
 3 files changed, 106 insertions(+), 96 deletions(-)

diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index be6ff2d..1728275 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -294,18 +294,6 @@ static int vboxInitialize(vboxPrivate *data)
 if (gVBoxAPI.domainEventCallbacks && gVBoxAPI.initializeDomainEvent(data) 
!= 0)
 goto cleanup;
 
-if (data->vboxObj == NULL) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("IVirtualBox object is null"));
-goto cleanup;
-}
-
-if (data->vboxSession == NULL) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("ISession object is null"));
-goto cleanup;
-}
-
 return 0;
 
  cleanup:
@@ -382,12 +370,12 @@ static void vboxUninitialize(vboxPrivate *data)
 if (!data)
 return;
 
-gVBoxAPI.UPFN.Uninitialize(data);
-
-virObjectUnref(data->caps);
 virObjectUnref(data->xmlopt);
 if (gVBoxAPI.domainEventCallbacks)
 virObjectEventStateFree(data->domainEvents);
+
+gVBoxAPI.UPFN.Uninitialize(data);
+
 VIR_FREE(data);
 }
 
@@ -398,6 +386,8 @@ vboxConnectOpen(virConnectPtr conn,
 unsigned int flags)
 {
 vboxPrivate *data = NULL;
+vboxGlobalData *globalData = NULL;
+
 uid_t uid = geteuid();
 
 virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR);
@@ -438,8 +428,12 @@ vboxConnectOpen(virConnectPtr conn,
 if (VIR_ALLOC(data) < 0)
 return VIR_DRV_OPEN_ERROR;
 
-if (!(data->caps = vboxCapsInit()) ||
-vboxInitialize(data) < 0 ||
+globalData = gVBoxAPI.registerGlobalData();
+
+if (!globalData || !(globalData->caps = vboxCapsInit()))
+return VIR_DRV_OPEN_ERROR;
+
+if (vboxInitialize(data) < 0 ||
 vboxExtractVersion(data) < 0 ||
 !(data->xmlopt = vboxXMLConfInit())) {
 vboxUninitialize(data);
@@ -451,12 +445,8 @@ vboxConnectOpen(virConnectPtr conn,
 vboxUninitialize(data);
 return VIR_DRV_OPEN_ERROR;
 }
-
-data->conn = conn;
 }
 
-if (gVBoxAPI.hasStaticGlobalData)
-gVBoxAPI.registerGlobalData(data);
 
 conn->privateData = data;
 VIR_DEBUG("in vboxOpen");
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index 13869eb..37dcd3e 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -102,6 +102,8 @@ typedef IMedium IHardDisk;
 
 VIR_LOG_INIT("vbox.vbox_tmpl");
 
+static vboxGlobalData *g_pVBoxGlobalData;
+
 #define vboxUnsupported() \
 VIR_WARN("No %s in current vbox version %d.", __FUNCTION__, 
VBOX_API_VERSION);
 
@@ -167,22 +169,6 @@ if (strUtf16) {\
   (unsigned)(iid)->m3[7]);\
 }\
 
-#if VBOX_API_VERSION > 2002000
-
-/* g_pVBoxGlobalData has to be global variable,
- * there is no other way to make the callbacks
- * work other then having g_pVBoxGlobalData as
- * global, because the functions namely AddRef,
- * Release, etc consider it as global and you
- * can't change the 

[libvirt] [PATCH 2/4] vbox: replace vboxGlobalData with vboxPrivate.

[Dawid Zamirski]

Since those stucts are identical at the moment, this patch basically
does s/vboxGlobalData \*data/vboxPrivate *data/ with type casts in
vboxDriverLock/Unlock calls to keep the code working and take care of
unavoidable diff noise to set the stage for further commits that
actually address the issue for the patch series.
---
 src/vbox/vbox_common.c| 164 +-
 src/vbox/vbox_network.c   |  24 +++
 src/vbox/vbox_storage.c   |  20 +++---
 src/vbox/vbox_tmpl.c  | 146 ++---
 src/vbox/vbox_uniformed_api.h |  54 +++---
 5 files changed, 204 insertions(+), 204 deletions(-)

diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 1472639..be6ff2d 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -57,7 +57,7 @@ VIR_LOG_INIT("vbox.vbox_common");
 /* global vbox API, used for all common codes. */
 static vboxUniformedAPI gVBoxAPI;
 
-static int openSessionForMachine(vboxGlobalData *data, const unsigned char 
*dom_uuid, vboxIIDUnion *iid,
+static int openSessionForMachine(vboxPrivate *data, const unsigned char 
*dom_uuid, vboxIIDUnion *iid,
  IMachine **machine, bool checkflag)
 {
 VBOX_IID_INITIALIZE(iid);
@@ -286,7 +286,7 @@ vboxXMLConfInit(void)
  NULL, NULL);
 }
 
-static int vboxInitialize(vboxGlobalData *data)
+static int vboxInitialize(vboxPrivate *data)
 {
 if (gVBoxAPI.UPFN.Initialize(data) != 0)
 goto cleanup;
@@ -348,7 +348,7 @@ static virCapsPtr vboxCapsInit(void)
 return NULL;
 }
 
-static int vboxExtractVersion(vboxGlobalData *data)
+static int vboxExtractVersion(vboxPrivate *data)
 {
 int ret = -1;
 PRUnichar *versionUtf16 = NULL;
@@ -377,7 +377,7 @@ static int vboxExtractVersion(vboxGlobalData *data)
 return ret;
 }
 
-static void vboxUninitialize(vboxGlobalData *data)
+static void vboxUninitialize(vboxPrivate *data)
 {
 if (!data)
 return;
@@ -397,7 +397,7 @@ vboxConnectOpen(virConnectPtr conn,
 virConfPtr conf ATTRIBUTE_UNUSED,
 unsigned int flags)
 {
-vboxGlobalData *data = NULL;
+vboxPrivate *data = NULL;
 uid_t uid = geteuid();
 
 virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR);
@@ -466,7 +466,7 @@ vboxConnectOpen(virConnectPtr conn,
 
 static int vboxConnectClose(virConnectPtr conn)
 {
-vboxGlobalData *data = conn->privateData;
+vboxPrivate *data = conn->privateData;
 VIR_DEBUG("%s: in vboxClose", conn->driver->name);
 
 vboxUninitialize(data);
@@ -478,7 +478,7 @@ static int vboxConnectClose(virConnectPtr conn)
 static int
 vboxDomainSave(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED)
 {
-vboxGlobalData *data = dom->conn->privateData;
+vboxPrivate *data = dom->conn->privateData;
 IConsole *console = NULL;
 vboxIIDUnion iid;
 IMachine *machine = NULL;
@@ -530,19 +530,19 @@ vboxDomainSave(virDomainPtr dom, const char *path 
ATTRIBUTE_UNUSED)
 return ret;
 }
 
-static void vboxDriverLock(vboxGlobalData *data)
+static void vboxDriverLock(vboxPrivate *data)
 {
 virMutexLock(>lock);
 }
 
-static void vboxDriverUnlock(vboxGlobalData *data)
+static void vboxDriverUnlock(vboxPrivate *data)
 {
 virMutexUnlock(>lock);
 }
 
 static int vboxConnectGetVersion(virConnectPtr conn, unsigned long *version)
 {
-vboxGlobalData *data = conn->privateData;
+vboxPrivate *data = conn->privateData;
 VIR_DEBUG("%s: in vboxGetVersion", conn->driver->name);
 
 vboxDriverLock(data);
@@ -577,7 +577,7 @@ static int vboxConnectIsAlive(virConnectPtr conn 
ATTRIBUTE_UNUSED)
 static int
 vboxConnectGetMaxVcpus(virConnectPtr conn, const char *type ATTRIBUTE_UNUSED)
 {
-vboxGlobalData *data = conn->privateData;
+vboxPrivate *data = conn->privateData;
 PRUint32 maxCPUCount = 0;
 int ret = -1;
 
@@ -604,7 +604,7 @@ vboxConnectGetMaxVcpus(virConnectPtr conn, const char *type 
ATTRIBUTE_UNUSED)
 
 static char *vboxConnectGetCapabilities(virConnectPtr conn)
 {
-vboxGlobalData *data = conn->privateData;
+vboxPrivate *data = conn->privateData;
 char *ret = NULL;
 
 if (!data->vboxObj)
@@ -619,7 +619,7 @@ static char *vboxConnectGetCapabilities(virConnectPtr conn)
 
 static int vboxConnectListDomains(virConnectPtr conn, int *ids, int nids)
 {
-vboxGlobalData *data = conn->privateData;
+vboxPrivate *data = conn->privateData;
 vboxArray machines = VBOX_ARRAY_INITIALIZER;
 PRUint32 state;
 nsresult rc;
@@ -661,7 +661,7 @@ static int vboxConnectListDomains(virConnectPtr conn, int 
*ids, int nids)
 
 static int vboxConnectNumOfDomains(virConnectPtr conn)
 {
-vboxGlobalData *data = conn->privateData;
+vboxPrivate *data = conn->privateData;
 vboxArray machines = VBOX_ARRAY_INITIALIZER;
 PRUint32 state;
 nsresult rc;
@@ -700,7 +700,7 @@ static int vboxConnectNumOfDomains(virConnectPtr conn)
 
 static virDomainPtr 

[libvirt] [PATCH 0/4] vbox: address thread-safety issues.

[Dawid Zamirski]

This patch series solves (at least in my testing) vbox driver
thread-safety issues that were also outlined on libvirt-users ML [1]
and I was affected with. Those patches try to follow the suggestions
made Matthias' [2] in that thread as close as possible. Here's where
my patch differs from that suggesions:

* vboxGlobalData - still needs to keep reference to ISession and
  IVirutalBox session because it's apparently not possible to have
  multiple instances created/destroyed safely with pfnComInitialize
  and pfnComUninitalize calls on per-connection basis.

* as such vboxPrivate (the new struct introduced here) also has
  references to ISession and IVirutalBox (which are just referenes to
  the ones from the global) mainly to immitate ISession instance
  per-connection. Apparently newer VBOX SDKs introduced
  pfnClinetInitialize that can allegedly create multiple ISessions and
  we might want to take advantage of that in the future hopefully
  without making additional changes allover the driver code that this
  patch did.

The gist of the change is in patch 3 and it also contains more
in-depth explanation on how the issue is being resolved. Also, please
note that patch 4 should be squashed into 3 as it was kept separate
only for code-review purposes and 3rd patch won't compile without 4
applied on top.

[1] https://www.redhat.com/archives/libvirt-users/2012-April/msg00122.html
[2] https://www.redhat.com/archives/libvirt-users/2012-April/msg00125.htmlgq

Dawid Zamirski (4):
  vbox: add vboxPrivate struct.
  vbox: replace vboxGlobalData with vboxPrivate.
  vbox: change API (un)initialization logic.
  vbox: update rest of the code to for prior changes.

 src/vbox/vbox_common.c| 275 ---
 src/vbox/vbox_common.h|  32 ++--
 src/vbox/vbox_network.c   |  51 +++--
 src/vbox/vbox_storage.c   |  20 +-
 src/vbox/vbox_tmpl.c  | 433 +-
 src/vbox/vbox_uniformed_api.h | 128 +++--
 6 files changed, 460 insertions(+), 479 deletions(-)

-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/4] vbox: add vboxPrivate struct.

[Dawid Zamirski]

To be passed as per-connection context info instead of using
vboxGlobalData that it will eventually replace in most cases.
---
 src/vbox/vbox_uniformed_api.h | 30 ++
 1 file changed, 30 insertions(+)

diff --git a/src/vbox/vbox_uniformed_api.h b/src/vbox/vbox_uniformed_api.h
index 74e9ac0..6ec5245 100644
--- a/src/vbox/vbox_uniformed_api.h
+++ b/src/vbox/vbox_uniformed_api.h
@@ -110,6 +110,36 @@ typedef struct {
 PCVBOXXPCOM pFuncs;
 
 /* The next is used for domainEvent */
+/* Async event handling */
+virObjectEventStatePtr domainEvents;
+int fdWatch;
+int volatile vboxCallBackRefCount;
+# if defined(VBOX_API_VERSION) && VBOX_API_VERSION > 2002000 && 
VBOX_API_VERSION < 400
+IVirtualBoxCallback *vboxCallback;
+nsIEventQueue *vboxQueue;
+# else /* VBOX_API_VERSION <= 2002000 || VBOX_API_VERSION >= 400 || 
VBOX_API_VERSION undefined */
+void *vboxCallback;
+void *vboxQueue;
+# endif /* VBOX_API_VERSION <= 2002000 || VBOX_API_VERSION >= 400 || 
VBOX_API_VERSION undefined */
+
+/* pointer back to the connection */
+virConnectPtr conn;
+} vboxPrivate;
+
+typedef struct {
+virMutex lock;
+unsigned long version;
+
+virCapsPtr caps;
+virDomainXMLOptionPtr xmlopt;
+
+IVirtualBox *vboxObj;
+ISession *vboxSession;
+
+/** Our version specific API table pointer. */
+PCVBOXXPCOM pFuncs;
+
+/* The next is used for domainEvent */
 # if defined(VBOX_API_VERSION) && VBOX_API_VERSION > 2002000 && 
VBOX_API_VERSION < 400
 
 /* Async event handling */
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] bhyve: chase cpuCompareXML rename

[Roman Bogorodskiy]

In commit 7f127de cpuCompareXML was renamed to virCPUCompareXML,
so change the bhyve driver to use the new function and thus
fix the build.
---
Pushed under the build-breaker rule.

 src/bhyve/bhyve_driver.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
index 51b6301..49b9e1a 100644
--- a/src/bhyve/bhyve_driver.c
+++ b/src/bhyve/bhyve_driver.c
@@ -1436,7 +1436,8 @@ bhyveConnectCompareCPU(virConnectPtr conn,
 ret = VIR_CPU_COMPARE_INCOMPATIBLE;
 }
 } else {
-ret = cpuCompareXML(caps->host.cpu, xmlDesc, failIncompatible);
+ret = virCPUCompareXML(caps->host.arch, caps->host.cpu,
+   xmlDesc, failIncompatible);
 }
 
  cleanup:
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] network: Add support for configuring dhcp lease time

[Martin Wilck]

On Wed, 2016-09-28 at 16:41 +0100, Daniel P. Berrange wrote:

> Anyone who cares about VMs having the same IP address after resuming
> from suspend *must* configure persistent DHCP mappings for their
> guests.
> Relying on automatic assignment will always fail in the end, may be
> not immediately, may be not often, but it *will* fail at some point,
> usually at the worst possible time.

I believe that setting "dhcp-authoritative" will be a major improvement
for many setups. Without it, VMs are *never* able to reacquire their
expired lease. With it, reacquiring the lease would work most of the
time (as far as I'm concerned, practically always). I reckon the
dnsmasq man page recommends it for a reason "when dnsmasq is definitely
the only DHCP server on a network".

Creating persistent DHCP entries using "virsh net-edit" is not exactly
user-friendly today. libvirt-nss is helpful, but other stuff relying on
IP address (NFS, ssh sessions, ...) would still benefit from "dhcp-
authorative". 

Regards
Martin

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC v2] libvirt vGPU QEMU integration

[Alex Williamson]

On Wed, 28 Sep 2016 13:06:31 -0700
Neo Jia  wrote:

> On Wed, Sep 28, 2016 at 01:55:47PM -0600, Alex Williamson wrote:
> > On Wed, 28 Sep 2016 12:22:35 -0700
> > Neo Jia  wrote:
> >   
> > > On Thu, Sep 22, 2016 at 03:26:38PM +0100, Daniel P. Berrange wrote:  
> > > > On Thu, Sep 22, 2016 at 08:19:21AM -0600, Alex Williamson wrote:
> > > > > On Thu, 22 Sep 2016 09:41:20 +0530
> > > > > Kirti Wankhede  wrote:
> > > > > 
> > > > > > > My concern is that a type id seems arbitrary but we're 
> > > > > > > specifying that
> > > > > > > it be unique.  We already have something unique, the name.  
> > > > > > > So why try
> > > > > > > to make the type id unique as well?  A vendor can 
> > > > > > > accidentally create
> > > > > > > their vendor driver so that a given name means something very
> > > > > > > specific.  On the other hand they need to be extremely 
> > > > > > > deliberate to
> > > > > > > coordinate that a type id means a unique thing across all 
> > > > > > > their product
> > > > > > > lines.
> > > > > > >   
> > > > > > 
> > > > > >  Let me clarify, type id should be unique in the list of
> > > > > >  mdev_supported_types. You can't have 2 directories in with 
> > > > > >  same name.
> > > > > > >>>
> > > > > > >>> Of course, but does that mean it's only unique to the machine 
> > > > > > >>> I'm
> > > > > > >>> currently running on?  Let's say I have a Tesla P100 on my 
> > > > > > >>> system and
> > > > > > >>> type-id 11 is named "GRID-M60-0B".  At some point in the future 
> > > > > > >>> I
> > > > > > >>> replace the Tesla P100 with a Q1000 (made up).  Is type-id 11 
> > > > > > >>> on that
> > > > > > >>> new card still going to be a "GRID-M60-0B"?  If not then we've 
> > > > > > >>> based
> > > > > > >>> our XML on the wrong attribute.  If the new device does not 
> > > > > > >>> support
> > > > > > >>> "GRID-M60-0B" then we should generate an error, not simply 
> > > > > > >>> initialize
> > > > > > >>> whatever type-id 11 happens to be on this new card.
> > > > > > >>> 
> > > > > > >>
> > > > > > >> If there are 2 M60 in the system then you would find '11' type 
> > > > > > >> directory
> > > > > > >> in mdev_supported_types of both M60. If you have P100, '11' type 
> > > > > > >> would
> > > > > > >> not be there in its mdev_supported_types, it will have different 
> > > > > > >> types.
> > > > > > >>
> > > > > > >> For example, if you replace M60 with P100, but XML is not 
> > > > > > >> updated. XML
> > > > > > >> have type '11'. When libvirt would try to create mdev device, 
> > > > > > >> libvirt
> > > > > > >> would have to find 'create' file in sysfs in following directory 
> > > > > > >> format:
> > > > > > >>
> > > > > > >>  --- mdev_supported_types
> > > > > > >>  |-- 11
> > > > > > >>  |   |-- create
> > > > > > >>
> > > > > > >> but now for P100, '11' directory is not there, so libvirt should 
> > > > > > >> throw
> > > > > > >> error on not able to find '11' directory.  
> > > > > > > 
> > > > > > > This really seems like an accident waiting to happen.  What 
> > > > > > > happens
> > > > > > > when the user replaces their M60 with an Intel XYZ device that 
> > > > > > > happens
> > > > > > > to expose a type 11 mdev class gpu device?  How is libvirt 
> > > > > > > supposed to
> > > > > > > know that the XML used to refer to a GRID-M60-0B and now it's an
> > > > > > > INTEL-IGD-XYZ?  Doesn't basing the XML entry on the name and 
> > > > > > > removing
> > > > > > > yet another arbitrary requirement that we have some sort of 
> > > > > > > globally
> > > > > > > unique type-id database make a lot of sense?  The same issue 
> > > > > > > applies
> > > > > > > for simple debug-ability, if I'm reviewing the XML for a domain 
> > > > > > > and the
> > > > > > > name is the primary index for the mdev device, I know what it is.
> > > > > > > Seeing type-id='11' is meaningless.
> > > > > > >  
> > > > > > 
> > > > > > Let me clarify again, type '11' is a string that vendor driver would
> > > > > > define (see my previous reply below) it could be "11" or 
> > > > > > "GRID-M60-0B".
> > > > > > If 2 vendors used same string we can't control that. right?
> > > > > > 
> > > > > > 
> > > > > >  Lets remove 'id' from type id in XML if that is the concern. 
> > > > > >  Supported
> > > > > >  types is going to be defined by vendor driver, so let vendor 
> > > > > >  driver
> > > > > >  decide what to use for directory name and same should be used 
> > > > > >  in device
> > > > > >  xml file, it could be '11' or "GRID M60-0B":
> > > > > > 
> > > > > >  
> > > > > >    my-vgpu
> > > > > >    pci__86_00_0
> > > > > >    
> > > > > >  

Re: [libvirt] [RFC v2] libvirt vGPU QEMU integration

[Laine Stump]

On 09/28/2016 03:59 PM, Neo Jia wrote:

On Wed, Sep 28, 2016 at 07:45:38PM +, Tian, Kevin wrote:

From: Neo Jia [mailto:c...@nvidia.com]
Sent: Thursday, September 29, 2016 3:23 AM

On Thu, Sep 22, 2016 at 03:26:38PM +0100, Daniel P. Berrange wrote:

On Thu, Sep 22, 2016 at 08:19:21AM -0600, Alex Williamson wrote:

On Thu, 22 Sep 2016 09:41:20 +0530
Kirti Wankhede  wrote:


My concern is that a type id seems arbitrary but we're specifying that
it be unique.  We already have something unique, the name.  So why try
to make the type id unique as well?  A vendor can accidentally create
their vendor driver so that a given name means something very
specific.  On the other hand they need to be extremely deliberate to
coordinate that a type id means a unique thing across all their product
lines.


Let me clarify, type id should be unique in the list of
mdev_supported_types. You can't have 2 directories in with same name.

Of course, but does that mean it's only unique to the machine I'm
currently running on?  Let's say I have a Tesla P100 on my system and
type-id 11 is named "GRID-M60-0B".  At some point in the future I
replace the Tesla P100 with a Q1000 (made up).  Is type-id 11 on that
new card still going to be a "GRID-M60-0B"?  If not then we've based
our XML on the wrong attribute.  If the new device does not support
"GRID-M60-0B" then we should generate an error, not simply initialize
whatever type-id 11 happens to be on this new card.


If there are 2 M60 in the system then you would find '11' type directory
in mdev_supported_types of both M60. If you have P100, '11' type would
not be there in its mdev_supported_types, it will have different types.

For example, if you replace M60 with P100, but XML is not updated. XML
have type '11'. When libvirt would try to create mdev device, libvirt
would have to find 'create' file in sysfs in following directory format:

  --- mdev_supported_types
  |-- 11
  |   |-- create

but now for P100, '11' directory is not there, so libvirt should throw
error on not able to find '11' directory.

This really seems like an accident waiting to happen.  What happens
when the user replaces their M60 with an Intel XYZ device that happens
to expose a type 11 mdev class gpu device?  How is libvirt supposed to
know that the XML used to refer to a GRID-M60-0B and now it's an
INTEL-IGD-XYZ?  Doesn't basing the XML entry on the name and removing
yet another arbitrary requirement that we have some sort of globally
unique type-id database make a lot of sense?  The same issue applies
for simple debug-ability, if I'm reviewing the XML for a domain and the
name is the primary index for the mdev device, I know what it is.
Seeing type-id='11' is meaningless.


Let me clarify again, type '11' is a string that vendor driver would
define (see my previous reply below) it could be "11" or "GRID-M60-0B".
If 2 vendors used same string we can't control that. right?



Lets remove 'id' from type id in XML if that is the concern. Supported
types is going to be defined by vendor driver, so let vendor driver
decide what to use for directory name and same should be used in device
xml file, it could be '11' or "GRID M60-0B":

 
   my-vgpu
   pci__86_00_0
   
 

Re: [libvirt] [PATCH 3/3] sanlock: Properly init io_timeout

[John Ferlan]

On 09/15/2016 10:35 AM, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1292984
> 
> Hold on to your hats, because this is gonna be wild.
> 
> In bd3e16a3 I've tried to expose sanlock io_timeout. What I had
> not realized (because there is like no documentation for sanlock
> at all) was very unusual way their APIs work. Basically, what we
> do currently is:
> 
> sanlock_add_lockspace_timeout(, io_timeout);
> 
> which adds a lockspace to sanlock daemon. One would expect that
> io_timeout sets the io_timeout for it. Nah! That's where you are
> completely off the tracks. It sets timeout for next lockspace you
> will probably add later. Therefore:
> 
>sanlock_add_lockspace_timeout(, io_timeout = 10);
>/* adds new lockspace with default io_timeout */
> 
>sanlock_add_lockspace_timeout(, io_timeout = 20);
>/* adds new lockspace with io_timeout = 10 */
> 
>sanlock_add_lockspace_timeout(, io_timeout = 40);
>/* adds new lockspace with io_timeout = 20 */
> 
> And so on. You get the picture.
> Fortunately, we don't allow setting io_timeout per domain or per
> domain disk. So we just need to set the default used in the very
> first step and hope for the best (as all the io_timeout-s used
> later will have the same value).
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/locking/lock_driver_sanlock.c | 25 -
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 

Any thoughts about modifying src/locking/sanlock.conf in order add some
text there that indicates support for 'io_timeout' requires at least
sanlock 2.7 (or something similar).

Beyond that things seem reasonable to me

ACK

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/3] lock_driver_sanlock: Avoid global driver variable whenever possible

[John Ferlan]

On 09/15/2016 10:35 AM, Michal Privoznik wrote:
> Global variables are bad, we should avoid using them.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/locking/lock_driver_sanlock.c | 42 
> ---
>  1 file changed, 26 insertions(+), 16 deletions(-)
> 
> diff --git a/src/locking/lock_driver_sanlock.c 
> b/src/locking/lock_driver_sanlock.c
> index 579f696..1ff1abd 100644
> --- a/src/locking/lock_driver_sanlock.c
> +++ b/src/locking/lock_driver_sanlock.c
> @@ -80,7 +80,7 @@ struct _virLockManagerSanlockDriver {
>  gid_t group;
>  };
>  
> -static virLockManagerSanlockDriver *driver;
> +static virLockManagerSanlockDriverPtr sanlockDriver;
>  
>  struct _virLockManagerSanlockPrivate {
>  const char *vm_uri;
> @@ -100,7 +100,8 @@ struct _virLockManagerSanlockPrivate {
>  /*
>   * sanlock plugin for the libvirt virLockManager API
>   */
> -static int virLockManagerSanlockLoadConfig(const char *configFile)
> +static int virLockManagerSanlockLoadConfig(virLockManagerSanlockDriverPtr 
> driver,
> +   const char *configFile)

static int
virLock*

Ironically you did this for the next one...

>  {
>  virConfPtr conf;
>  int ret = -1;
> @@ -161,7 +162,8 @@ static int virLockManagerSanlockLoadConfig(const char 
> *configFile)
>  /* How many times try adding a lockspace? */
>  #define LOCKSPACE_RETRIES 10
>  
> -static int virLockManagerSanlockSetupLockspace(void)
> +static int
> +virLockManagerSanlockSetupLockspace(virLockManagerSanlockDriverPtr driver)
>  {
>  int fd = -1;
>  struct stat st;
> @@ -371,16 +373,20 @@ static int virLockManagerSanlockInit(unsigned int 
> version,
>   const char *configFile,
>   unsigned int flags)
>  {
> +virLockManagerSanlockDriverPtr driver;
> +
>  VIR_DEBUG("version=%u configFile=%s flags=%x",
>version, NULLSTR(configFile), flags);
>  virCheckFlags(0, -1);
>  
> -if (driver)
> +if (sanlockDriver)
>  return 0;
>  
> -if (VIR_ALLOC(driver) < 0)
> +if (VIR_ALLOC(sanlockDriver) < 0)
>  return -1;
>  
> +driver = sanlockDriver;
> +
>  driver->requireLeaseForDisks = true;
>  driver->hostID = 0;
>  driver->autoDiskLease = false;
> @@ -392,7 +398,7 @@ static int virLockManagerSanlockInit(unsigned int version,
>  goto error;
>  }
>  
> -if (virLockManagerSanlockLoadConfig(configFile) < 0)
> +if (virLockManagerSanlockLoadConfig(driver, configFile) < 0)
>  goto error;
>  
>  if (driver->autoDiskLease && !driver->hostID) {
> @@ -402,7 +408,7 @@ static int virLockManagerSanlockInit(unsigned int version,
>  }
>  
>  if (driver->autoDiskLease) {
> -if (virLockManagerSanlockSetupLockspace() < 0)
> +if (virLockManagerSanlockSetupLockspace(driver) < -1)
>  goto error;
>  }
>  
> @@ -415,11 +421,11 @@ static int virLockManagerSanlockInit(unsigned int 
> version,
>  
>  static int virLockManagerSanlockDeinit(void)
>  {
> -if (!driver)
> +if (!sanlockDriver)
>  return 0;
>  
> -VIR_FREE(driver->autoDiskLeasePath);
> -VIR_FREE(driver);
> +VIR_FREE(sanlockDriver->autoDiskLeasePath);
> +VIR_FREE(sanlockDriver);
>  
>  return 0;
>  }
> @@ -438,7 +444,7 @@ static int virLockManagerSanlockNew(virLockManagerPtr 
> lock,
>  
>  virCheckFlags(VIR_LOCK_MANAGER_NEW_STARTED, -1);
>  
> -if (!driver) {
> +if (!sanlockDriver) {

Interesting that this is the one API which checks - every other API
assumes sanlockDriver would be set since they can assume the Init
function was run.

>  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("Sanlock plugin is not initialized"));
>  return -1;
> @@ -566,7 +572,8 @@ static int 
> virLockManagerSanlockAddLease(virLockManagerPtr lock,
>  
>  
>  
> -static int virLockManagerSanlockAddDisk(virLockManagerPtr lock,
> +static int virLockManagerSanlockAddDisk(virLockManagerSanlockDriverPtr 
> driver,
> +virLockManagerPtr lock,

static int
virLock*

>  const char *name,
>  size_t nparams,
>  virLockManagerParamPtr params 
> ATTRIBUTE_UNUSED,
> @@ -631,7 +638,8 @@ static int virLockManagerSanlockAddDisk(virLockManagerPtr 
> lock,
>  }
>  
>  
> -static int virLockManagerSanlockCreateLease(struct sanlk_resource *res)
> +static int virLockManagerSanlockCreateLease(virLockManagerSanlockDriverPtr 
> driver,
> + 

static int
virLock*
   struct sanlk_resource *res)
>  {
>  int fd = -1;
>  struct stat st;
> @@ -719,6 +727,7 @@ static int 
> virLockManagerSanlockAddResource(virLockManagerPtr lock,
>  virLockManagerParamPtr 

Re: [libvirt] [PATCH 2/3] m4: Check for sanlock_write_lockspace

[John Ferlan]

On 09/15/2016 10:35 AM, Michal Privoznik wrote:
> Currently, we are checking for sanlock_add_lockspace_timeout
> which is good for now. But in the next patches we are gonna use

s/the next patches/a subsequent patch/
s/gonna/going to/

> sanlock_write_lockspace (which sets an initial value for io
> timeout for sanlock). Now, there is no reason to check for both
> functions in sanlock library as the sanlock_write_lockspace was
> introduced in 2.7 release and the one we are currently checking
> for in the 2.5 release. Therefore it is safe to assume presence
> of sanlock_add_lockspace_timeout when sanlock_write_lockspace is
> detected.
> 
> Moreover, the macro for conditional compilation is renamed to
> HAVE_SANLOCK_IO_TIMEOUT (as it now encapsulates two functions).
> 
> Signed-off-by: Michal Privoznik 
> ---
>  m4/virt-sanlock.m4| 14 +-
>  src/locking/lock_driver_sanlock.c |  2 +-
>  2 files changed, 10 insertions(+), 6 deletions(-)
> 

ACK (seems reasonable to me, but my m4 knowledge is light)


John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC v2] libvirt vGPU QEMU integration

[Alex Williamson]

On Wed, 28 Sep 2016 12:59:59 -0700
Neo Jia  wrote:

> On Wed, Sep 28, 2016 at 07:45:38PM +, Tian, Kevin wrote:
> > > From: Neo Jia [mailto:c...@nvidia.com]
> > > Sent: Thursday, September 29, 2016 3:23 AM
> > > 
> > > On Thu, Sep 22, 2016 at 03:26:38PM +0100, Daniel P. Berrange wrote:  
> > > > On Thu, Sep 22, 2016 at 08:19:21AM -0600, Alex Williamson wrote:  
> > > > > On Thu, 22 Sep 2016 09:41:20 +0530
> > > > > Kirti Wankhede  wrote:
> > > > >  
> > > > > > > My concern is that a type id seems arbitrary but we're 
> > > > > > > specifying that
> > > > > > > it be unique.  We already have something unique, the name.  
> > > > > > > So why try
> > > > > > > to make the type id unique as well?  A vendor can 
> > > > > > > accidentally create
> > > > > > > their vendor driver so that a given name means something very
> > > > > > > specific.  On the other hand they need to be extremely 
> > > > > > > deliberate to
> > > > > > > coordinate that a type id means a unique thing across all 
> > > > > > > their product
> > > > > > > lines.
> > > > > > >  
> > > > > > 
> > > > > >  Let me clarify, type id should be unique in the list of
> > > > > >  mdev_supported_types. You can't have 2 directories in with 
> > > > > >  same name.  
> > > > > > >>>
> > > > > > >>> Of course, but does that mean it's only unique to the machine 
> > > > > > >>> I'm
> > > > > > >>> currently running on?  Let's say I have a Tesla P100 on my 
> > > > > > >>> system and
> > > > > > >>> type-id 11 is named "GRID-M60-0B".  At some point in the future 
> > > > > > >>> I
> > > > > > >>> replace the Tesla P100 with a Q1000 (made up).  Is type-id 11 
> > > > > > >>> on that
> > > > > > >>> new card still going to be a "GRID-M60-0B"?  If not then we've 
> > > > > > >>> based
> > > > > > >>> our XML on the wrong attribute.  If the new device does not 
> > > > > > >>> support
> > > > > > >>> "GRID-M60-0B" then we should generate an error, not simply 
> > > > > > >>> initialize
> > > > > > >>> whatever type-id 11 happens to be on this new card.
> > > > > > >>>  
> > > > > > >>
> > > > > > >> If there are 2 M60 in the system then you would find '11' type 
> > > > > > >> directory
> > > > > > >> in mdev_supported_types of both M60. If you have P100, '11' type 
> > > > > > >> would
> > > > > > >> not be there in its mdev_supported_types, it will have different 
> > > > > > >> types.
> > > > > > >>
> > > > > > >> For example, if you replace M60 with P100, but XML is not 
> > > > > > >> updated. XML
> > > > > > >> have type '11'. When libvirt would try to create mdev device, 
> > > > > > >> libvirt
> > > > > > >> would have to find 'create' file in sysfs in following directory 
> > > > > > >> format:
> > > > > > >>
> > > > > > >>  --- mdev_supported_types
> > > > > > >>  |-- 11
> > > > > > >>  |   |-- create
> > > > > > >>
> > > > > > >> but now for P100, '11' directory is not there, so libvirt should 
> > > > > > >> throw
> > > > > > >> error on not able to find '11' directory.  
> > > > > > >
> > > > > > > This really seems like an accident waiting to happen.  What 
> > > > > > > happens
> > > > > > > when the user replaces their M60 with an Intel XYZ device that 
> > > > > > > happens
> > > > > > > to expose a type 11 mdev class gpu device?  How is libvirt 
> > > > > > > supposed to
> > > > > > > know that the XML used to refer to a GRID-M60-0B and now it's an
> > > > > > > INTEL-IGD-XYZ?  Doesn't basing the XML entry on the name and 
> > > > > > > removing
> > > > > > > yet another arbitrary requirement that we have some sort of 
> > > > > > > globally
> > > > > > > unique type-id database make a lot of sense?  The same issue 
> > > > > > > applies
> > > > > > > for simple debug-ability, if I'm reviewing the XML for a domain 
> > > > > > > and the
> > > > > > > name is the primary index for the mdev device, I know what it is.
> > > > > > > Seeing type-id='11' is meaningless.
> > > > > > >  
> > > > > >
> > > > > > Let me clarify again, type '11' is a string that vendor driver would
> > > > > > define (see my previous reply below) it could be "11" or 
> > > > > > "GRID-M60-0B".
> > > > > > If 2 vendors used same string we can't control that. right?
> > > > > >
> > > > > >  
> > > > > >  Lets remove 'id' from type id in XML if that is the concern. 
> > > > > >  Supported
> > > > > >  types is going to be defined by vendor driver, so let vendor 
> > > > > >  driver
> > > > > >  decide what to use for directory name and same should be used 
> > > > > >  in device
> > > > > >  xml file, it could be '11' or "GRID M60-0B":
> > > > > > 
> > > > > >  
> > > > > >    my-vgpu
> > > > > >    pci__86_00_0
> > > > > >    
> > > > > >  

Re: [libvirt] [RFC v2] libvirt vGPU QEMU integration

[Tian, Kevin]

> From: Neo Jia [mailto:c...@nvidia.com]
> Sent: Thursday, September 29, 2016 3:23 AM
> 
> On Thu, Sep 22, 2016 at 03:26:38PM +0100, Daniel P. Berrange wrote:
> > On Thu, Sep 22, 2016 at 08:19:21AM -0600, Alex Williamson wrote:
> > > On Thu, 22 Sep 2016 09:41:20 +0530
> > > Kirti Wankhede  wrote:
> > >
> > > > > My concern is that a type id seems arbitrary but we're specifying 
> > > > > that
> > > > > it be unique.  We already have something unique, the name.  So 
> > > > > why try
> > > > > to make the type id unique as well?  A vendor can accidentally 
> > > > > create
> > > > > their vendor driver so that a given name means something very
> > > > > specific.  On the other hand they need to be extremely deliberate 
> > > > > to
> > > > > coordinate that a type id means a unique thing across all their 
> > > > > product
> > > > > lines.
> > > > >
> > > > 
> > > >  Let me clarify, type id should be unique in the list of
> > > >  mdev_supported_types. You can't have 2 directories in with same 
> > > >  name.
> > > > >>>
> > > > >>> Of course, but does that mean it's only unique to the machine I'm
> > > > >>> currently running on?  Let's say I have a Tesla P100 on my system 
> > > > >>> and
> > > > >>> type-id 11 is named "GRID-M60-0B".  At some point in the future I
> > > > >>> replace the Tesla P100 with a Q1000 (made up).  Is type-id 11 on 
> > > > >>> that
> > > > >>> new card still going to be a "GRID-M60-0B"?  If not then we've based
> > > > >>> our XML on the wrong attribute.  If the new device does not support
> > > > >>> "GRID-M60-0B" then we should generate an error, not simply 
> > > > >>> initialize
> > > > >>> whatever type-id 11 happens to be on this new card.
> > > > >>>
> > > > >>
> > > > >> If there are 2 M60 in the system then you would find '11' type 
> > > > >> directory
> > > > >> in mdev_supported_types of both M60. If you have P100, '11' type 
> > > > >> would
> > > > >> not be there in its mdev_supported_types, it will have different 
> > > > >> types.
> > > > >>
> > > > >> For example, if you replace M60 with P100, but XML is not updated. 
> > > > >> XML
> > > > >> have type '11'. When libvirt would try to create mdev device, libvirt
> > > > >> would have to find 'create' file in sysfs in following directory 
> > > > >> format:
> > > > >>
> > > > >>  --- mdev_supported_types
> > > > >>  |-- 11
> > > > >>  |   |-- create
> > > > >>
> > > > >> but now for P100, '11' directory is not there, so libvirt should 
> > > > >> throw
> > > > >> error on not able to find '11' directory.
> > > > >
> > > > > This really seems like an accident waiting to happen.  What happens
> > > > > when the user replaces their M60 with an Intel XYZ device that happens
> > > > > to expose a type 11 mdev class gpu device?  How is libvirt supposed to
> > > > > know that the XML used to refer to a GRID-M60-0B and now it's an
> > > > > INTEL-IGD-XYZ?  Doesn't basing the XML entry on the name and removing
> > > > > yet another arbitrary requirement that we have some sort of globally
> > > > > unique type-id database make a lot of sense?  The same issue applies
> > > > > for simple debug-ability, if I'm reviewing the XML for a domain and 
> > > > > the
> > > > > name is the primary index for the mdev device, I know what it is.
> > > > > Seeing type-id='11' is meaningless.
> > > > >
> > > >
> > > > Let me clarify again, type '11' is a string that vendor driver would
> > > > define (see my previous reply below) it could be "11" or "GRID-M60-0B".
> > > > If 2 vendors used same string we can't control that. right?
> > > >
> > > >
> > > >  Lets remove 'id' from type id in XML if that is the concern. 
> > > >  Supported
> > > >  types is going to be defined by vendor driver, so let vendor driver
> > > >  decide what to use for directory name and same should be used in 
> > > >  device
> > > >  xml file, it could be '11' or "GRID M60-0B":
> > > > 
> > > >  
> > > >    my-vgpu
> > > >    pci__86_00_0
> > > >    
> > > >  

Re: [libvirt] [RFC v2] libvirt vGPU QEMU integration

[Alex Williamson]

On Wed, 28 Sep 2016 12:22:35 -0700
Neo Jia  wrote:

> On Thu, Sep 22, 2016 at 03:26:38PM +0100, Daniel P. Berrange wrote:
> > On Thu, Sep 22, 2016 at 08:19:21AM -0600, Alex Williamson wrote:  
> > > On Thu, 22 Sep 2016 09:41:20 +0530
> > > Kirti Wankhede  wrote:
> > >   
> > > > > My concern is that a type id seems arbitrary but we're specifying 
> > > > > that
> > > > > it be unique.  We already have something unique, the name.  So 
> > > > > why try
> > > > > to make the type id unique as well?  A vendor can accidentally 
> > > > > create
> > > > > their vendor driver so that a given name means something very
> > > > > specific.  On the other hand they need to be extremely deliberate 
> > > > > to
> > > > > coordinate that a type id means a unique thing across all their 
> > > > > product
> > > > > lines.
> > > > > 
> > > > 
> > > >  Let me clarify, type id should be unique in the list of
> > > >  mdev_supported_types. You can't have 2 directories in with same 
> > > >  name.  
> > > > >>>
> > > > >>> Of course, but does that mean it's only unique to the machine I'm
> > > > >>> currently running on?  Let's say I have a Tesla P100 on my system 
> > > > >>> and
> > > > >>> type-id 11 is named "GRID-M60-0B".  At some point in the future I
> > > > >>> replace the Tesla P100 with a Q1000 (made up).  Is type-id 11 on 
> > > > >>> that
> > > > >>> new card still going to be a "GRID-M60-0B"?  If not then we've based
> > > > >>> our XML on the wrong attribute.  If the new device does not support
> > > > >>> "GRID-M60-0B" then we should generate an error, not simply 
> > > > >>> initialize
> > > > >>> whatever type-id 11 happens to be on this new card.
> > > > >>>   
> > > > >>
> > > > >> If there are 2 M60 in the system then you would find '11' type 
> > > > >> directory
> > > > >> in mdev_supported_types of both M60. If you have P100, '11' type 
> > > > >> would
> > > > >> not be there in its mdev_supported_types, it will have different 
> > > > >> types.
> > > > >>
> > > > >> For example, if you replace M60 with P100, but XML is not updated. 
> > > > >> XML
> > > > >> have type '11'. When libvirt would try to create mdev device, libvirt
> > > > >> would have to find 'create' file in sysfs in following directory 
> > > > >> format:
> > > > >>
> > > > >>  --- mdev_supported_types
> > > > >>  |-- 11
> > > > >>  |   |-- create
> > > > >>
> > > > >> but now for P100, '11' directory is not there, so libvirt should 
> > > > >> throw
> > > > >> error on not able to find '11' directory.
> > > > > 
> > > > > This really seems like an accident waiting to happen.  What happens
> > > > > when the user replaces their M60 with an Intel XYZ device that happens
> > > > > to expose a type 11 mdev class gpu device?  How is libvirt supposed to
> > > > > know that the XML used to refer to a GRID-M60-0B and now it's an
> > > > > INTEL-IGD-XYZ?  Doesn't basing the XML entry on the name and removing
> > > > > yet another arbitrary requirement that we have some sort of globally
> > > > > unique type-id database make a lot of sense?  The same issue applies
> > > > > for simple debug-ability, if I'm reviewing the XML for a domain and 
> > > > > the
> > > > > name is the primary index for the mdev device, I know what it is.
> > > > > Seeing type-id='11' is meaningless.
> > > > >
> > > > 
> > > > Let me clarify again, type '11' is a string that vendor driver would
> > > > define (see my previous reply below) it could be "11" or "GRID-M60-0B".
> > > > If 2 vendors used same string we can't control that. right?
> > > > 
> > > >   
> > > >  Lets remove 'id' from type id in XML if that is the concern. 
> > > >  Supported
> > > >  types is going to be defined by vendor driver, so let vendor driver
> > > >  decide what to use for directory name and same should be used in 
> > > >  device
> > > >  xml file, it could be '11' or "GRID M60-0B":
> > > > 
> > > >  
> > > >    my-vgpu
> > > >    pci__86_00_0
> > > >    
> > > >  

[libvirt] [PATCH] vsh: Write out history on "quit" or "exit" in interactive mode

[John Ferlan]

https://bugzilla.redhat.com/show_bug.cgi?id=1379895

Introduced by commit id '834c5720'.

During the code motion and creation of vsh.c, the function 'vshDeinit()'
in the (new) vsh.c was altered from whence it came in virsh.c such that
calling 'vshReadlineDeinit(ctl)' was conditional on "ctl->imode".

This causes a problem for the interactive running if the "quit" and "exit"
commands are used because 'cmdQuit' will clear ctl->imode, thus when the
interactive loop in main() of virsh.c exits because ctl->mode is clear and
virshDeinit is called which calls vshDeinit, the history file is now not
written. Conversely, if one had exited the interactive loop via pressing
D the file would be created because loop control is broken on EOF
and ctl->imode is not set to false.

This patch will remove the conditional call to vshReadlineDeinit and
restore the former behaviour.

Signed-off-by: John Ferlan 
---
 I realize some people don't like the comment I added; however, I'd
 rather be "safe" when purely reading the code than expect someone
 to do a git log -p looking at commit messages for "removed" lines of code.

 An alternative approach would be to create/set a new boolean value
 such as "iquit" (or "iexit") during cmdQuit and then in vshDeinit make
 calling vshReadlineDeinit() conditional on "ctl->imode || ctl->iquit"

 Calling the vshReadlineDeinit during non interactive mode would have no
 affect since vshReadlineInit is only called if ctl->imode is set. The
 Deinit function will essentially do nothing other than a couple of
 VIR_FREE(NULL); and one extra "if"

 tools/vsh.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/vsh.c b/tools/vsh.c
index 041113f..eba3f0f 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -3093,8 +3093,12 @@ vshInitReload(vshControl *ctl)
 void
 vshDeinit(vshControl *ctl)
 {
-if (ctl->imode)
-vshReadlineDeinit(ctl);
+/* Don't make calling vshReadlineDeinit conditional on imode. During
+ * interactive mode when "quit" or "exit" is typed, 'imode' is set
+ * to false so if this were conditional on imode, then history wouldn't
+ * be written when the "quit" or "exit" commands were used instead of
+ * when D is used */
+vshReadlineDeinit(ctl);
 vshCloseLogFile(ctl);
 }
 
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] Don't drop expired lease while reading custom leases file

[Nehal J Wani]

Libvirt, on its own, shouldn't decide whether an expired lease should
stay in the custom leases database or not. It should rather rely on
the 'DEL' event from dnsmasq.
---
 src/util/virlease.c | 8 
 1 file changed, 8 deletions(-)

diff --git a/src/util/virlease.c b/src/util/virlease.c
index 920ebaf..b49105d 100644
--- a/src/util/virlease.c
+++ b/src/util/virlease.c
@@ -57,7 +57,6 @@ virLeaseReadCustomLeaseFile(virJSONValuePtr leases_array_new,
 {
 char *lease_entries = NULL;
 virJSONValuePtr leases_array = NULL;
-long long currtime = 0;
 long long expirytime;
 int custom_lease_file_len = 0;
 virJSONValuePtr lease_tmp = NULL;
@@ -66,8 +65,6 @@ virLeaseReadCustomLeaseFile(virJSONValuePtr leases_array_new,
 size_t i;
 int ret = -1;
 
-currtime = (long long) time(NULL);
-
 /* Read entire contents */
 if ((custom_lease_file_len = virFileReadAll(custom_lease_file,
 
VIR_NETWORK_DHCP_LEASE_FILE_SIZE_MAX,
@@ -109,11 +106,6 @@ virLeaseReadCustomLeaseFile(virJSONValuePtr 
leases_array_new,
_("failed to parse json"));
 goto cleanup;
 }
-/* Check whether lease has expired or not */
-if (expirytime < currtime) {
-i++;
-continue;
-}
 
 /* Check whether lease has to be included or not */
 if (ip_to_delete && STREQ(ip_tmp, ip_to_delete)) {
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC v2] libvirt vGPU QEMU integration

[Neo Jia]

On Wed, Sep 28, 2016 at 07:45:38PM +, Tian, Kevin wrote:
> > From: Neo Jia [mailto:c...@nvidia.com]
> > Sent: Thursday, September 29, 2016 3:23 AM
> > 
> > On Thu, Sep 22, 2016 at 03:26:38PM +0100, Daniel P. Berrange wrote:
> > > On Thu, Sep 22, 2016 at 08:19:21AM -0600, Alex Williamson wrote:
> > > > On Thu, 22 Sep 2016 09:41:20 +0530
> > > > Kirti Wankhede  wrote:
> > > >
> > > > > > My concern is that a type id seems arbitrary but we're 
> > > > > > specifying that
> > > > > > it be unique.  We already have something unique, the name.  So 
> > > > > > why try
> > > > > > to make the type id unique as well?  A vendor can accidentally 
> > > > > > create
> > > > > > their vendor driver so that a given name means something very
> > > > > > specific.  On the other hand they need to be extremely 
> > > > > > deliberate to
> > > > > > coordinate that a type id means a unique thing across all their 
> > > > > > product
> > > > > > lines.
> > > > > >
> > > > > 
> > > > >  Let me clarify, type id should be unique in the list of
> > > > >  mdev_supported_types. You can't have 2 directories in with same 
> > > > >  name.
> > > > > >>>
> > > > > >>> Of course, but does that mean it's only unique to the machine I'm
> > > > > >>> currently running on?  Let's say I have a Tesla P100 on my system 
> > > > > >>> and
> > > > > >>> type-id 11 is named "GRID-M60-0B".  At some point in the future I
> > > > > >>> replace the Tesla P100 with a Q1000 (made up).  Is type-id 11 on 
> > > > > >>> that
> > > > > >>> new card still going to be a "GRID-M60-0B"?  If not then we've 
> > > > > >>> based
> > > > > >>> our XML on the wrong attribute.  If the new device does not 
> > > > > >>> support
> > > > > >>> "GRID-M60-0B" then we should generate an error, not simply 
> > > > > >>> initialize
> > > > > >>> whatever type-id 11 happens to be on this new card.
> > > > > >>>
> > > > > >>
> > > > > >> If there are 2 M60 in the system then you would find '11' type 
> > > > > >> directory
> > > > > >> in mdev_supported_types of both M60. If you have P100, '11' type 
> > > > > >> would
> > > > > >> not be there in its mdev_supported_types, it will have different 
> > > > > >> types.
> > > > > >>
> > > > > >> For example, if you replace M60 with P100, but XML is not updated. 
> > > > > >> XML
> > > > > >> have type '11'. When libvirt would try to create mdev device, 
> > > > > >> libvirt
> > > > > >> would have to find 'create' file in sysfs in following directory 
> > > > > >> format:
> > > > > >>
> > > > > >>  --- mdev_supported_types
> > > > > >>  |-- 11
> > > > > >>  |   |-- create
> > > > > >>
> > > > > >> but now for P100, '11' directory is not there, so libvirt should 
> > > > > >> throw
> > > > > >> error on not able to find '11' directory.
> > > > > >
> > > > > > This really seems like an accident waiting to happen.  What happens
> > > > > > when the user replaces their M60 with an Intel XYZ device that 
> > > > > > happens
> > > > > > to expose a type 11 mdev class gpu device?  How is libvirt supposed 
> > > > > > to
> > > > > > know that the XML used to refer to a GRID-M60-0B and now it's an
> > > > > > INTEL-IGD-XYZ?  Doesn't basing the XML entry on the name and 
> > > > > > removing
> > > > > > yet another arbitrary requirement that we have some sort of globally
> > > > > > unique type-id database make a lot of sense?  The same issue applies
> > > > > > for simple debug-ability, if I'm reviewing the XML for a domain and 
> > > > > > the
> > > > > > name is the primary index for the mdev device, I know what it is.
> > > > > > Seeing type-id='11' is meaningless.
> > > > > >
> > > > >
> > > > > Let me clarify again, type '11' is a string that vendor driver would
> > > > > define (see my previous reply below) it could be "11" or 
> > > > > "GRID-M60-0B".
> > > > > If 2 vendors used same string we can't control that. right?
> > > > >
> > > > >
> > > > >  Lets remove 'id' from type id in XML if that is the concern. 
> > > > >  Supported
> > > > >  types is going to be defined by vendor driver, so let vendor 
> > > > >  driver
> > > > >  decide what to use for directory name and same should be used in 
> > > > >  device
> > > > >  xml file, it could be '11' or "GRID M60-0B":
> > > > > 
> > > > >  
> > > > >    my-vgpu
> > > > >    pci__86_00_0
> > > > >    
> > > > >  

Re: [libvirt] [PATCH 08/12] qemu: Add length for bps/iops throttling parameters to driver

[John Ferlan]

>> +} else if (STREQ(param->field,
>> + 
>> VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH)) {
>> +info.write_iops_sec_max_length = param->value.ul;
>> +set_iops_max_length = true;
>> +if (virTypedParamsAddULLong(, ,
>> +,
>> +
>> VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX_LENGTH,
>> +param->value.ul) < 0)
>> +goto endjob;
> 
> I wonder if we could turn these into something more readable. But that's
> something for a different patch set.
> 

Well since I didn't push before freeze maybe I can insert/add a patch
that will make for fewer lines and push once 2.4 opens along with
adjustments to change from 2.3 to 2.4.

Initially I was scared off by the if-else if logic, but after working
through the conditions, I ended up with 3 macros that build upon the
'bytes' and 'iops' in order to create each of the if/else checks.

Starting with "bytes" or "iops", for each a prefix of "total_", "read_",
or "write_" will be added prior to calling another macro which will
append the postfixes of "_sec", "_sec_max", and "_sec_max_length".

It looks cleaner, but really makes one look closely to see what's
happening.

I'll post a reply here with the patch for consideration

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Qemu-devel] [RFC v2] libvirt vGPU QEMU integration

[Neo Jia]

On Tue, Sep 20, 2016 at 10:47:53AM +0100, Daniel P. Berrange wrote:
> On Tue, Sep 20, 2016 at 02:05:52AM +0530, Kirti Wankhede wrote:
> > 
> > Hi libvirt experts,
> > 
> > Thanks for valuable input on v1 version of RFC.
> > 
> > Quick brief, VFIO based mediated device framework provides a way to
> > virtualize their devices without SR-IOV, like NVIDIA vGPU, Intel KVMGT
> > and IBM's channel IO. This framework reuses VFIO APIs for all the
> > functionalities for mediated devices which are currently being used for
> > pass through devices. This framework introduces a set of new sysfs files
> > for device creation and its life cycle management.
> > 
> > Here is the summary of discussion on v1:
> > 1. Discover mediated device:
> > As part of physical device initialization process, vendor driver will
> > register their physical devices, which will be used to create virtual
> > device (mediated device, aka mdev) to the mediated framework.
> > 
> > Vendor driver should specify mdev_supported_types in directory format.
> > This format is class based, for example, display class directory format
> > should be as below. We need to define such set for each class of devices
> > which would be supported by mediated device framework.
> > 
> >  --- mdev_destroy
> >  --- mdev_supported_types
> >  |-- 11
> >  |   |-- create
> >  |   |-- name
> >  |   |-- fb_length
> >  |   |-- resolution
> >  |   |-- heads
> >  |   |-- max_instances
> >  |   |-- params
> >  |   |-- requires_group
> >  |-- 12
> >  |   |-- create
> >  |   |-- name
> >  |   |-- fb_length
> >  |   |-- resolution
> >  |   |-- heads
> >  |   |-- max_instances
> >  |   |-- params
> >  |   |-- requires_group
> >  |-- 13
> >  |-- create
> >  |-- name
> >  |-- fb_length
> >  |-- resolution
> >  |-- heads
> >  |-- max_instances
> >  |-- params
> >  |-- requires_group
> > 
> > 
> > In the above example directory '11' represents a type id of mdev device.
> > 'name', 'fb_length', 'resolution', 'heads', 'max_instance' and
> > 'requires_group' would be Read-Only files that vendor would provide to
> > describe about that type.
> > 
> > 'create':
> > Write-only file. Mandatory.
> > Accepts string to create mediated device.
> > 
> > 'name':
> > Read-Only file. Mandatory.
> > Returns string, the name of that type id.
> 
> Presumably this is a human-targetted title/description of
> the device.
> 
> > 
> > 'fb_length':
> > Read-only file. Mandatory.
> > Returns {K,M,G}, size of framebuffer.
> > 
> > 'resolution':
> > Read-Only file. Mandatory.
> > Returns 'hres x vres' format. Maximum supported resolution.
> > 
> > 'heads':
> > Read-Only file. Mandatory.
> > Returns integer. Number of maximum heads supported.
> 
> None of these should be mandatory as that makes the mdev
> useless for non-GPU devices.
> 
> I'd expect to see a 'class' or 'type' attribute in the
> directory whcih tells you what kind of mdev it is. A
> valid 'class' value would be 'gpu'. The fb_length,
> resolution, and heads parameters would only be mandatory
> when class==gpu.
> 

Hi Daniel,

Here you are proposing to add a class named "gpu", which will make all those gpu
related attributes mandatory, which libvirt can allow user to better
parse/present a particular mdev configuration?

I am just wondering if there is another option that we just make all those
attributes that a mdev device can have as optional but still meaningful to
libvirt, so libvirt can still parse / recognize them as an class "mdev".

In general, I am just trying to understand the requirement from libvirt and see
how we can fit in this requirement for both Intel and NVIDIA since Intel is also
moving to the type-based interface although they don't have "class" concept yet.

Thanks,
Neo

> > 'max_instance':
> > Read-Only file. Mandatory.
> > Returns integer.  Returns maximum mdev device could be created
> > at the moment when this file is read. This count would be updated by
> > vendor driver. Before creating mdev device of this type, check if
> > max_instance is > 0.
> > 
> > 'params'
> > Write-Only file. Optional.
> > String input. Libvirt would pass the string given in XML file to
> > this file and then create mdev device. Set empty string to clear params.
> > For example, set parameter 'frame_rate_limiter=0' to disable frame rate
> > limiter for performance benchmarking, then create device of type 11. The
> > device created would have that parameter set by vendor driver.
> 
> Nope, libvirt will explicitly *NEVER* allow arbitrary opaque
> passthrough of vendor specific data in this way.
> 
> > The parent device would look like:
> > 
> >
> >  pci__86_00_0
> >  
> >0
> >134
> >0
> >0
> >
> >  
> >  
> >
> >GRID M60-0B
> >512M
> >

Re: [libvirt] [RFC v2] libvirt vGPU QEMU integration

[Neo Jia]

On Wed, Sep 28, 2016 at 01:55:47PM -0600, Alex Williamson wrote:
> On Wed, 28 Sep 2016 12:22:35 -0700
> Neo Jia  wrote:
> 
> > On Thu, Sep 22, 2016 at 03:26:38PM +0100, Daniel P. Berrange wrote:
> > > On Thu, Sep 22, 2016 at 08:19:21AM -0600, Alex Williamson wrote:  
> > > > On Thu, 22 Sep 2016 09:41:20 +0530
> > > > Kirti Wankhede  wrote:
> > > >   
> > > > > > My concern is that a type id seems arbitrary but we're 
> > > > > > specifying that
> > > > > > it be unique.  We already have something unique, the name.  So 
> > > > > > why try
> > > > > > to make the type id unique as well?  A vendor can accidentally 
> > > > > > create
> > > > > > their vendor driver so that a given name means something very
> > > > > > specific.  On the other hand they need to be extremely 
> > > > > > deliberate to
> > > > > > coordinate that a type id means a unique thing across all their 
> > > > > > product
> > > > > > lines.
> > > > > > 
> > > > > 
> > > > >  Let me clarify, type id should be unique in the list of
> > > > >  mdev_supported_types. You can't have 2 directories in with same 
> > > > >  name.  
> > > > > >>>
> > > > > >>> Of course, but does that mean it's only unique to the machine I'm
> > > > > >>> currently running on?  Let's say I have a Tesla P100 on my system 
> > > > > >>> and
> > > > > >>> type-id 11 is named "GRID-M60-0B".  At some point in the future I
> > > > > >>> replace the Tesla P100 with a Q1000 (made up).  Is type-id 11 on 
> > > > > >>> that
> > > > > >>> new card still going to be a "GRID-M60-0B"?  If not then we've 
> > > > > >>> based
> > > > > >>> our XML on the wrong attribute.  If the new device does not 
> > > > > >>> support
> > > > > >>> "GRID-M60-0B" then we should generate an error, not simply 
> > > > > >>> initialize
> > > > > >>> whatever type-id 11 happens to be on this new card.
> > > > > >>>   
> > > > > >>
> > > > > >> If there are 2 M60 in the system then you would find '11' type 
> > > > > >> directory
> > > > > >> in mdev_supported_types of both M60. If you have P100, '11' type 
> > > > > >> would
> > > > > >> not be there in its mdev_supported_types, it will have different 
> > > > > >> types.
> > > > > >>
> > > > > >> For example, if you replace M60 with P100, but XML is not updated. 
> > > > > >> XML
> > > > > >> have type '11'. When libvirt would try to create mdev device, 
> > > > > >> libvirt
> > > > > >> would have to find 'create' file in sysfs in following directory 
> > > > > >> format:
> > > > > >>
> > > > > >>  --- mdev_supported_types
> > > > > >>  |-- 11
> > > > > >>  |   |-- create
> > > > > >>
> > > > > >> but now for P100, '11' directory is not there, so libvirt should 
> > > > > >> throw
> > > > > >> error on not able to find '11' directory.
> > > > > > 
> > > > > > This really seems like an accident waiting to happen.  What happens
> > > > > > when the user replaces their M60 with an Intel XYZ device that 
> > > > > > happens
> > > > > > to expose a type 11 mdev class gpu device?  How is libvirt supposed 
> > > > > > to
> > > > > > know that the XML used to refer to a GRID-M60-0B and now it's an
> > > > > > INTEL-IGD-XYZ?  Doesn't basing the XML entry on the name and 
> > > > > > removing
> > > > > > yet another arbitrary requirement that we have some sort of globally
> > > > > > unique type-id database make a lot of sense?  The same issue applies
> > > > > > for simple debug-ability, if I'm reviewing the XML for a domain and 
> > > > > > the
> > > > > > name is the primary index for the mdev device, I know what it is.
> > > > > > Seeing type-id='11' is meaningless.
> > > > > >
> > > > > 
> > > > > Let me clarify again, type '11' is a string that vendor driver would
> > > > > define (see my previous reply below) it could be "11" or 
> > > > > "GRID-M60-0B".
> > > > > If 2 vendors used same string we can't control that. right?
> > > > > 
> > > > >   
> > > > >  Lets remove 'id' from type id in XML if that is the concern. 
> > > > >  Supported
> > > > >  types is going to be defined by vendor driver, so let vendor 
> > > > >  driver
> > > > >  decide what to use for directory name and same should be used in 
> > > > >  device
> > > > >  xml file, it could be '11' or "GRID M60-0B":
> > > > > 
> > > > >  
> > > > >    my-vgpu
> > > > >    pci__86_00_0
> > > > >    
> > > > >  

Re: [libvirt] [RFC v2] libvirt vGPU QEMU integration

[Neo Jia]

On Wed, Sep 28, 2016 at 04:31:25PM -0400, Laine Stump wrote:
> On 09/28/2016 03:59 PM, Neo Jia wrote:
> > On Wed, Sep 28, 2016 at 07:45:38PM +, Tian, Kevin wrote:
> > > > From: Neo Jia [mailto:c...@nvidia.com]
> > > > Sent: Thursday, September 29, 2016 3:23 AM
> > > > 
> > > > On Thu, Sep 22, 2016 at 03:26:38PM +0100, Daniel P. Berrange wrote:
> > > > > On Thu, Sep 22, 2016 at 08:19:21AM -0600, Alex Williamson wrote:
> > > > > > On Thu, 22 Sep 2016 09:41:20 +0530
> > > > > > Kirti Wankhede  wrote:
> > > > > > 
> > > > > > > > > > > > My concern is that a type id seems arbitrary but we're 
> > > > > > > > > > > > specifying that
> > > > > > > > > > > > it be unique.  We already have something unique, the 
> > > > > > > > > > > > name.  So why try
> > > > > > > > > > > > to make the type id unique as well?  A vendor can 
> > > > > > > > > > > > accidentally create
> > > > > > > > > > > > their vendor driver so that a given name means 
> > > > > > > > > > > > something very
> > > > > > > > > > > > specific.  On the other hand they need to be extremely 
> > > > > > > > > > > > deliberate to
> > > > > > > > > > > > coordinate that a type id means a unique thing across 
> > > > > > > > > > > > all their product
> > > > > > > > > > > > lines.
> > > > > > > > > > > > 
> > > > > > > > > > > Let me clarify, type id should be unique in the list of
> > > > > > > > > > > mdev_supported_types. You can't have 2 directories in 
> > > > > > > > > > > with same name.
> > > > > > > > > > Of course, but does that mean it's only unique to the 
> > > > > > > > > > machine I'm
> > > > > > > > > > currently running on?  Let's say I have a Tesla P100 on my 
> > > > > > > > > > system and
> > > > > > > > > > type-id 11 is named "GRID-M60-0B".  At some point in the 
> > > > > > > > > > future I
> > > > > > > > > > replace the Tesla P100 with a Q1000 (made up).  Is type-id 
> > > > > > > > > > 11 on that
> > > > > > > > > > new card still going to be a "GRID-M60-0B"?  If not then 
> > > > > > > > > > we've based
> > > > > > > > > > our XML on the wrong attribute.  If the new device does not 
> > > > > > > > > > support
> > > > > > > > > > "GRID-M60-0B" then we should generate an error, not simply 
> > > > > > > > > > initialize
> > > > > > > > > > whatever type-id 11 happens to be on this new card.
> > > > > > > > > > 
> > > > > > > > > If there are 2 M60 in the system then you would find '11' 
> > > > > > > > > type directory
> > > > > > > > > in mdev_supported_types of both M60. If you have P100, '11' 
> > > > > > > > > type would
> > > > > > > > > not be there in its mdev_supported_types, it will have 
> > > > > > > > > different types.
> > > > > > > > > 
> > > > > > > > > For example, if you replace M60 with P100, but XML is not 
> > > > > > > > > updated. XML
> > > > > > > > > have type '11'. When libvirt would try to create mdev device, 
> > > > > > > > > libvirt
> > > > > > > > > would have to find 'create' file in sysfs in following 
> > > > > > > > > directory format:
> > > > > > > > > 
> > > > > > > > >   --- mdev_supported_types
> > > > > > > > >   |-- 11
> > > > > > > > >   |   |-- create
> > > > > > > > > 
> > > > > > > > > but now for P100, '11' directory is not there, so libvirt 
> > > > > > > > > should throw
> > > > > > > > > error on not able to find '11' directory.
> > > > > > > > This really seems like an accident waiting to happen.  What 
> > > > > > > > happens
> > > > > > > > when the user replaces their M60 with an Intel XYZ device that 
> > > > > > > > happens
> > > > > > > > to expose a type 11 mdev class gpu device?  How is libvirt 
> > > > > > > > supposed to
> > > > > > > > know that the XML used to refer to a GRID-M60-0B and now it's an
> > > > > > > > INTEL-IGD-XYZ?  Doesn't basing the XML entry on the name and 
> > > > > > > > removing
> > > > > > > > yet another arbitrary requirement that we have some sort of 
> > > > > > > > globally
> > > > > > > > unique type-id database make a lot of sense?  The same issue 
> > > > > > > > applies
> > > > > > > > for simple debug-ability, if I'm reviewing the XML for a domain 
> > > > > > > > and the
> > > > > > > > name is the primary index for the mdev device, I know what it 
> > > > > > > > is.
> > > > > > > > Seeing type-id='11' is meaningless.
> > > > > > > > 
> > > > > > > Let me clarify again, type '11' is a string that vendor driver 
> > > > > > > would
> > > > > > > define (see my previous reply below) it could be "11" or 
> > > > > > > "GRID-M60-0B".
> > > > > > > If 2 vendors used same string we can't control that. right?
> > > > > > > 
> > > > > > > 
> > > > > > > > > > > Lets remove 'id' from type id in XML if that is the 
> > > > > > > > > > > concern. Supported
> > > > > > > > > > > types is going to be defined by vendor driver, so let 
> > > > > > > > > > > vendor driver
> > > > > > > > > > > decide what to use for directory name and same should be 
> > > > > > > > > > > used in device
> > > > > > > > > > > xml 

[libvirt] [PATCH 08.5/12] qemu: Create a set of macros to handle setting block iotune values

[John Ferlan]

Rework the code in a set of 3 macros that will use the "base" of
'bytes' or 'iops' and build up the prefixes of 'total_', 'read_',
and 'write_' before adding the postfixes of '_sec', '_sec_max',
and '_sec_max_length' and making the param->field comparison and
adding of the field.

Signed-off-by: John Ferlan 
---

 NB: Hopefully this applies - my branch is based off of the git head
 which I refreshed prior to sending the patch

 Since I missed 2.3, I figured why not try to make the change.

 src/qemu/qemu_driver.c | 216 +++--
 1 file changed, 45 insertions(+), 171 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2b5b6fc..cbf9483 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17321,6 +17321,41 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
 VIR_DOMAIN_TUNABLE_BLKDEV_DISK, path) < 0)
 goto endjob;
 
+#define SET_IOTUNE_FIELD(FIELD, BOOL, CONST)   
\
+do {   
\
+info.FIELD = params->value.ul; 
\
+BOOL = true;   
\
+if (virTypedParamsAddULLong(, ,   
\
+,   
\
+VIR_DOMAIN_TUNABLE_BLKDEV_##CONST, 
\
+param->value.ul) < 0)  
\
+goto endjob;   
\
+} while (0);
+
+/* Taking a prefix build by CHECK_SET_IOTUNE, add the postfix of "_sec",
+ * "_sec_max", and "_sec_max_length" to each of the variables in order to
+ * in order to check each of the ways one of these can be used. The boolean
+ * doesn't care if "total_", "read_", or "write_" is the prefix. */
+#define CHECK_IOTUNE(FIELD, BOOL, CONST)   
\
+if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_##CONST##_SEC)) {  
\
+SET_IOTUNE_FIELD(FIELD##_sec, set_##BOOL, CONST##_SEC);
\
+} else if (STREQ(param->field, 
\
+ VIR_DOMAIN_BLOCK_IOTUNE_##CONST##_SEC_MAX)) { 
\
+SET_IOTUNE_FIELD(FIELD##_sec_max, set_##BOOL##_max, CONST##_SEC_MAX);  
\
+} else if (STREQ(param->field, 
\
+ VIR_DOMAIN_BLOCK_IOTUNE_##CONST##_SEC_MAX_LENGTH)) {  
\
+SET_IOTUNE_FIELD(FIELD##_sec_max_length, set_##BOOL##_max_length,  
\
+ CONST##_SEC_MAX_LENGTH);  
\
+}
+
+/* For "bytes" and "tune", prepend the "total_", "read_", and "write_" onto
+ * the field/const names, but also passing the "base" field to be used as the
+ * basis of the boolean. */
+#define CHECK_SET_IOTUNE(FIELD, CONST) 
\
+CHECK_IOTUNE(total_##FIELD, FIELD, TOTAL_##CONST); 
\
+CHECK_IOTUNE(read_##FIELD, FIELD, READ_##CONST);   
\
+CHECK_IOTUNE(write_##FIELD, FIELD, WRITE_##CONST);
+
 for (i = 0; i < nparams; i++) {
 virTypedParameterPtr param = [i];
 
@@ -17331,178 +17366,17 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
 goto endjob;
 }
 
-if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC)) {
-info.total_bytes_sec = param->value.ul;
-set_bytes = true;
-if (virTypedParamsAddULLong(, ,
-,
-
VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC,
-param->value.ul) < 0)
-goto endjob;
-} else if (STREQ(param->field,
- VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC)) {
-info.read_bytes_sec = param->value.ul;
-set_bytes = true;
-if (virTypedParamsAddULLong(, ,
-,
-
VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC,
-param->value.ul) < 0)
-goto endjob;
-} else if (STREQ(param->field,
- VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC)) {
-info.write_bytes_sec = param->value.ul;
-set_bytes = true;
-if (virTypedParamsAddULLong(, ,
-,
-
VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC,
-param->value.ul) < 0)
-goto endjob;
-} else if (STREQ(param->field,
- VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC)) {
-

Re: [libvirt] [RFC v2] libvirt vGPU QEMU integration

[Neo Jia]

On Thu, Sep 22, 2016 at 03:26:38PM +0100, Daniel P. Berrange wrote:
> On Thu, Sep 22, 2016 at 08:19:21AM -0600, Alex Williamson wrote:
> > On Thu, 22 Sep 2016 09:41:20 +0530
> > Kirti Wankhede  wrote:
> > 
> > > > My concern is that a type id seems arbitrary but we're specifying 
> > > > that
> > > > it be unique.  We already have something unique, the name.  So why 
> > > > try
> > > > to make the type id unique as well?  A vendor can accidentally 
> > > > create
> > > > their vendor driver so that a given name means something very
> > > > specific.  On the other hand they need to be extremely deliberate to
> > > > coordinate that a type id means a unique thing across all their 
> > > > product
> > > > lines.
> > > >   
> > > 
> > >  Let me clarify, type id should be unique in the list of
> > >  mdev_supported_types. You can't have 2 directories in with same 
> > >  name.
> > > >>>
> > > >>> Of course, but does that mean it's only unique to the machine I'm
> > > >>> currently running on?  Let's say I have a Tesla P100 on my system and
> > > >>> type-id 11 is named "GRID-M60-0B".  At some point in the future I
> > > >>> replace the Tesla P100 with a Q1000 (made up).  Is type-id 11 on that
> > > >>> new card still going to be a "GRID-M60-0B"?  If not then we've based
> > > >>> our XML on the wrong attribute.  If the new device does not support
> > > >>> "GRID-M60-0B" then we should generate an error, not simply initialize
> > > >>> whatever type-id 11 happens to be on this new card.
> > > >>> 
> > > >>
> > > >> If there are 2 M60 in the system then you would find '11' type 
> > > >> directory
> > > >> in mdev_supported_types of both M60. If you have P100, '11' type would
> > > >> not be there in its mdev_supported_types, it will have different types.
> > > >>
> > > >> For example, if you replace M60 with P100, but XML is not updated. XML
> > > >> have type '11'. When libvirt would try to create mdev device, libvirt
> > > >> would have to find 'create' file in sysfs in following directory 
> > > >> format:
> > > >>
> > > >>  --- mdev_supported_types
> > > >>  |-- 11
> > > >>  |   |-- create
> > > >>
> > > >> but now for P100, '11' directory is not there, so libvirt should throw
> > > >> error on not able to find '11' directory.  
> > > > 
> > > > This really seems like an accident waiting to happen.  What happens
> > > > when the user replaces their M60 with an Intel XYZ device that happens
> > > > to expose a type 11 mdev class gpu device?  How is libvirt supposed to
> > > > know that the XML used to refer to a GRID-M60-0B and now it's an
> > > > INTEL-IGD-XYZ?  Doesn't basing the XML entry on the name and removing
> > > > yet another arbitrary requirement that we have some sort of globally
> > > > unique type-id database make a lot of sense?  The same issue applies
> > > > for simple debug-ability, if I'm reviewing the XML for a domain and the
> > > > name is the primary index for the mdev device, I know what it is.
> > > > Seeing type-id='11' is meaningless.
> > > >  
> > > 
> > > Let me clarify again, type '11' is a string that vendor driver would
> > > define (see my previous reply below) it could be "11" or "GRID-M60-0B".
> > > If 2 vendors used same string we can't control that. right?
> > > 
> > > 
> > >  Lets remove 'id' from type id in XML if that is the concern. 
> > >  Supported
> > >  types is going to be defined by vendor driver, so let vendor driver
> > >  decide what to use for directory name and same should be used in 
> > >  device
> > >  xml file, it could be '11' or "GRID M60-0B":
> > > 
> > >  
> > >    my-vgpu
> > >    pci__86_00_0
> > >    
> > >  

[libvirt] [PATCH] NSS: Add explicit check to not report expired lease

[Nehal J Wani]

The NSS module shouldn't rely on custom leases database to not have
entries for leases which have expired.
---
 tools/nss/libvirt_nss.c | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c
index 54c4a2a..57ba473 100644
--- a/tools/nss/libvirt_nss.c
+++ b/tools/nss/libvirt_nss.c
@@ -42,6 +42,7 @@
 #include "virlease.h"
 #include "viralloc.h"
 #include "virfile.h"
+#include "virtime.h"
 #include "virerror.h"
 #include "virstring.h"
 #include "virsocketaddr.h"
@@ -114,6 +115,8 @@ findLease(const char *name,
 ssize_t i, nleases;
 leaseAddress *tmpAddress = NULL;
 size_t ntmpAddress = 0;
+long long currtime = 0;
+long long expirytime = -1;
 
 *address = NULL;
 *naddress = 0;
@@ -161,6 +164,8 @@ findLease(const char *name,
 nleases = virJSONValueArraySize(leases_array);
 DEBUG("Read %zd leases", nleases);
 
+currtime = (long long) time(NULL);
+
 for (i = 0; i < nleases; i++) {
 virJSONValuePtr lease;
 const char *lease_name;
@@ -181,6 +186,16 @@ findLease(const char *name,
 if (STRNEQ_NULLABLE(name, lease_name))
 continue;
 
+if (virJSONValueObjectGetNumberLong(lease, "expiry-time", ) 
< 0) {
+/* A lease cannot be present without expiry-time */
+ERROR("expiry-time field missing for %s", name);
+goto cleanup;
+}
+
+/* Do not report expired lease */
+if (expirytime < currtime)
+continue;
+
 DEBUG("Found record for %s", lease_name);
 *found = true;
 
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] network: Add support for configuring dhcp lease time

[Daniel P. Berrange]

On Tue, Sep 27, 2016 at 09:15:18AM +0530, Nehal J Wani wrote:
> 
> I did another experiment. My network xml limited the range to:
> 
>   
> 
>   
> 
>   
> 
> So, we can safely assume that, if, at any given point of time, there
> are two virtual machines with different mac addresses, connected to
> this network, they should have been leased different IP addresses.
> 
> VM1 is spawned with -net nic,macaddr=52:54:00:69:09:b6 -net bridge,br=virbr1
> 
> VM1 is assigned 192.168.123.4 with lease time of 2 mins.
> 
> Sep 27 08:53:32 lenovo dnsmasq-dhcp[12156]: DHCPDISCOVER(virbr1)
> 52:54:00:69:09:b6
> Sep 27 08:53:32 lenovo dnsmasq-dhcp[12156]: DHCPOFFER(virbr1)
> 192.168.123.4 52:54:00:69:09:b6
> Sep 27 08:53:32 lenovo dnsmasq-dhcp[12156]: DHCPREQUEST(virbr1)
> 192.168.123.4 52:54:00:69:09:b6
> Sep 27 08:53:32 lenovo dnsmasq-dhcp[12156]: DHCPACK(virbr1)
> 192.168.123.4 52:54:00:69:09:b6
> 
> VM1 is paused.
> 
> VM1's lease expires.
> 
> VM2 is spawned with -net nic,macaddr=52:54:00:69:09:b1 -net bridge,br=virbr1
> 
> VM2 is assigned 192.168.123.4 with lease time of 2 mins. :-'(

That is totally valid for dnsmasq todo. Once VM1's lease expires there
is no rule that it must not be given to VM2.

Once VM1 is unpaused it will try to renew its original lease
and will get a NACK from dnsmasq, at which point it will have
to request a new lease and get a different IP address.

There's nothing really special about VMs here either. The same would
happen with physical laptop if you close the lid and re-open it some
time later after your lease expired - any other laptop could have got
your lease in the meantime.

Anyone who cares about VMs having the same IP address after resuming
from suspend *must* configure persistent DHCP mappings for their guests.
Relying on automatic assignment will always fail in the end, may be
not immediately, may be not often, but it *will* fail at some point,
usually at the worst possible time.

Or if you enable the libvirt nss module, then you can avoid needing to
care about specific PI addresses at all and just use the VM names
as a hostname

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] network: Add support for configuring dhcp lease time

[Daniel P. Berrange]

On Wed, Sep 28, 2016 at 04:23:32PM +0200, Martin Wilck wrote:
> On Tue, 2016-09-27 at 09:15 +0530, Nehal J Wani wrote:
> > 
> > Seems like the previous experiment involved a happy accident indeed.
> > 
> 
> So, what are we going to do about libvirt+dnsmasq's handling of expired
> leases now?

IMHO nothing. Once a lease has expired all bets are off, so any behaviour
is valid and you have to expect you'll get a different IP address at some
point.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/3] qemu: store guest visible disk size from qemu monitor block info

[Nikolay Shirokovskiy]

On 28.09.2016 00:53, John Ferlan wrote:
> 
> 
> On 09/27/2016 04:17 AM, Nikolay Shirokovskiy wrote:
>>
>>
>> On 26.09.2016 23:07, John Ferlan wrote:
>>>
>>>
>>> On 09/07/2016 05:24 AM, Nikolay Shirokovskiy wrote:
 ---
  src/qemu/qemu_domain.h   |  1 +
  src/qemu/qemu_monitor_json.c | 17 +
  tests/qemumonitorjsontest.c  | 36 
  3 files changed, 54 insertions(+)

 diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
 index 13c0372..ea57111 100644
 --- a/src/qemu/qemu_domain.h
 +++ b/src/qemu/qemu_domain.h
 @@ -337,6 +337,7 @@ struct qemuDomainDiskInfo {
  bool tray_open;
  bool empty;
  int io_status;
 +unsigned long long guest_size;
>>>
>>> a/k/a: 'virtual-size' in qemu and 'capacity' in libvirt
>>>
>>> I probably would stick with capacity or something that says "disk"
>>> rather than "guest".
>>>
  };
  
>>>
>>> Trying to determine/decide whether this patch should be "separated" and
>>> perhaps made usable with/for the existing callers or whether patch 3
>>> should use the block stats (qemuDomainGetStatsBlock) which already
>>> handles some details that could be missing here (at least w/r/t backing
>>> chains).
>>
>> I donno, does backing chain changes anything in this case? I need virtual
>> size of top image only and anyway is it possible for images of backing
>> chain to have different virtual sizes? It does not make much sense.
> 
> I agree... I don't have in depth knowledge of how the backing chains
> work, so it's more of a question. Hopefully someone that understands
> that logic (pkrempa? eblake?) would be able to be more definitive.
> 
> I just wouldn't want something to be missed if backing chains were required.
> 
>>
>> qemuDomainGetStatsBlock (and more specifically 
>> qemuMonitorBlockStatsUpdateCapacity
>> which is more suited to get just size) has rather inconviniet arguments
>> more suited for many disks requests.
>>
> 
> There's lots of similarities that I see in that code... I spent some
> time today working through a mechanism to make one "query-block" call
> that 3 consumers (BlockInfo, BlockStats, and DiskNameLookup) could use
> (I have it working nominally at least). I would think this could then be
> used to more easily add a new lookup function that's only returning the
> data you want rather than putting stats data in an BlockInfo...
> 
>>>
>>> Another option is adjusting the qemuMonitorJSONDiskNameLookup code to be
>>> more multi-purpose since it's using the "query-block" for
>>> *BlockPullCommon (from Rebase and Pull) as well as from *BlockCommit and
>>> it already checks/expects both "inserted" and "image" to be present in
>>> order to return that name.
>>
>> I would not make function with such specific name provide additionally size 
>> info...
>> I think it is perfectily fine to have many monitor commands that internally
>> use same 'query-block' qemu command because this command is really profound)
>>
> 
> Note that I didn't say add a new param/return - I said make it more
> multi-purpose with a subtly implied inference that perhaps a function
> name change and a different return value (or local/static structure)
> could work as well (and hence why I tried to combine things today to see
> what was possible).
> 
>>>
>>>
  typedef struct _qemuDomainHostdevPrivate qemuDomainHostdevPrivate;
 diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
 index 3d0a120..7903b47 100644
 --- a/src/qemu/qemu_monitor_json.c
 +++ b/src/qemu/qemu_monitor_json.c
 @@ -1800,6 +1800,8 @@ int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon,
  
  for (i = 0; i < virJSONValueArraySize(devices); i++) {
  virJSONValuePtr dev = virJSONValueArrayGet(devices, i);
 +virJSONValuePtr inserted;
 +virJSONValuePtr image;
  struct qemuDomainDiskInfo *info;
  const char *thisdev;
  const char *status;
 @@ -1854,6 +1856,21 @@ int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon,
  if (info->io_status < 0)
  goto cleanup;
  }
 +
 +if ((inserted = virJSONValueObjectGetObject(dev, "inserted"))) {
 +if (!(image = virJSONValueObjectGetObject(inserted, 
 "image"))) {
 +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
 +   _("cannot read 'inserted/image' value"));
 +goto cleanup;
 +}
>>>
>>> Other code that checks "inserted" and "image" doesn't fail - it just
>>> ignores. If there's only going to be one consumer, then I don't believe
>>> we want a failure such as this to affect other callers that may not
>>> care. That could mean having some sort of "marker" in the returned
>>> buffer that the fetch of "virtual-size" did occur (or just using not
>>> zero). It would be a shame to have 

Re: [libvirt] [PATCH 3/3] introduce pull backup

[Nikolay Shirokovskiy]

On 27.09.2016 01:04, John Ferlan wrote:
> 
> 
> On 09/07/2016 05:24 AM, Nikolay Shirokovskiy wrote:
>> Add API to start/stop exporting disks for a backup and add qemu
>> implementation.
>>
>> The latter is not complete yet. At least backup disks are not
>> cleaned up and libvirt restart is not handled.
>> ---
>>  examples/object-events/event-test.c |   3 +
>>  include/libvirt/libvirt-domain-backup.h |  45 +++
>>  include/libvirt/libvirt-domain.h|   3 +
>>  include/libvirt/libvirt.h   |   1 +
>>  include/libvirt/virterror.h |   1 +
>>  po/POTFILES.in  |   2 +
>>  src/Makefile.am |   3 +
>>  src/access/viraccessperm.c  |   3 +-
>>  src/access/viraccessperm.h  |   6 +
>>  src/conf/backup_conf.c  | 295 ++
>>  src/conf/backup_conf.h  |  85 
>>  src/conf/domain_conf.c  |   2 +-
>>  src/driver-hypervisor.h |  11 +
>>  src/libvirt-domain-backup.c |  86 
>>  src/libvirt_private.syms|   6 +
>>  src/libvirt_public.syms |   2 +
>>  src/qemu/qemu_blockjob.c|   2 +
>>  src/qemu/qemu_conf.h|   1 +
>>  src/qemu/qemu_domain.h  |   4 +
>>  src/qemu/qemu_driver.c  | 684 
>> 
>>  src/qemu/qemu_monitor.c |  32 ++
>>  src/qemu/qemu_monitor.h |  12 +
>>  src/qemu/qemu_monitor_json.c| 105 +
>>  src/qemu/qemu_monitor_json.h|  16 +
>>  src/remote/remote_driver.c  |   2 +
>>  src/remote/remote_protocol.x|  33 +-
>>  src/util/virerror.c |   1 +
>>  tools/Makefile.am   |   1 +
>>  tools/virsh-backup.c| 150 +++
>>  tools/virsh-backup.h|  28 ++
>>  tools/virsh-domain.c|   3 +-
>>  tools/virsh.c   |   2 +
>>  tools/virsh.h   |   1 +
>>  33 files changed, 1627 insertions(+), 4 deletions(-)
>>  create mode 100644 include/libvirt/libvirt-domain-backup.h
>>  create mode 100644 src/conf/backup_conf.c
>>  create mode 100644 src/conf/backup_conf.h
>>  create mode 100644 src/libvirt-domain-backup.c
>>  create mode 100644 tools/virsh-backup.c
>>  create mode 100644 tools/virsh-backup.h
>>
> 
> This patch fails make check/syntax-check :
> 
>   GEN  check-augeas-virtlogd
> --- remote_protocol-structs   2016-09-26 08:01:11.645962427 -0400
> +++ remote_protocol-struct-t3 2016-09-26 08:59:57.386094561 -0400
> @@ -2080,6 +2080,21 @@
>  remote_nonnull_domain_snapshot snap;
>  u_int  flags;
>  };
> +struct remote_domain_backup_start_args {
> +remote_nonnull_domain  dom;
> +remote_nonnull_string  xml_desc;
> +u_int  flags;
> +};
> +struct remote_domain_backup_start_ret {
> +intresult;
> +};
> +struct remote_domain_backup_stop_args {
> +remote_nonnull_domain  dom;
> +u_int  flags;
> +};
> +struct remote_domain_backup_stop_ret {
> +intresult;
> +};
>  struct remote_domain_open_console_args {
>  remote_nonnull_domain  dom;
>  remote_string  dev_name;
> @@ -3169,4 +3184,6 @@
>  REMOTE_PROC_CONNECT_NODE_DEVICE_EVENT_DEREGISTER_ANY = 375,
>  REMOTE_PROC_NODE_DEVICE_EVENT_LIFECYCLE = 376,
>  REMOTE_PROC_NODE_DEVICE_EVENT_UPDATE = 377,
> +REMOTE_PROC_DOMAIN_BACKUP_START = 378,
> +REMOTE_PROC_DOMAIN_BACKUP_STOP = 379,
>  };
> Makefile:11101: recipe for target 'remote_protocol-struct' failed
> 
> 
> This also needs to be split up and perhaps regenerated as an RFC so that
> debate can be had on your cover/.0 description.
> 
> Because it's also dependent upon an x-blockdev-del, it cannot be pushed
> upstream to libvirt. I know qemu work continues w/r/t blockdev-add and
> backups, but I don't follow it in detail (not enough time in the day!).

Ok, at least the patch can be some kind of candidate to be pushed upstream
as soon as blockdev-del drops x- prefix.

> 
> I'll scan the rest to provide a few comments...  I really didn't dig
> into algorithms too much.
> 
>> diff --git a/examples/object-events/event-test.c 
>> b/examples/object-events/event-test.c
>> index 730cb8b..08490bb 100644
>> --- a/examples/object-events/event-test.c
>> +++ b/examples/object-events/event-test.c
>> @@ -829,6 +829,9 @@ blockJobTypeToStr(int type)
>>  
>>  case VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT:
>>  return "active layer block commit";
>> +
>> +case VIR_DOMAIN_BLOCK_JOB_TYPE_BACKUP:
>> +return "block backup";
>>  }
>>  
>>  return "unknown";
>> diff --git a/include/libvirt/libvirt-domain-backup.h 
>> 

[libvirt] [PATCH] qemu_process: add pid of vm in domain log

[Chen Hanxiao]

From: Chen Hanxiao 

Add pid of VM in domain log.
We used to show this info in debug log.

For example:
If a process send SIGKILL to a qemu process,
we could find something in audit logs.
Then the pid of VM in domain log will be helpful.

Signed-off-by: Chen Hanxiao 
---
 src/qemu/qemu_process.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 27d04a4..8510a89 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5392,11 +5392,14 @@ qemuProcessLaunch(virConnectPtr conn,
_("Domain %s didn't show up"), vm->def->name);
 rv = -1;
 }
-VIR_DEBUG("QEMU vm=%p name=%s running with pid=%llu",
-  vm, vm->def->name, (unsigned long long)vm->pid);
+qemuDomainLogContextWrite(logCtxt,
+  "QEMU vm=%p name=%s running with pid=%llu\n",
+  vm,
+  vm->def->name,
+  (unsigned long long)vm->pid);
 } else {
-VIR_DEBUG("QEMU vm=%p name=%s failed to spawn",
-  vm, vm->def->name);
+qemuDomainLogContextWrite(logCtxt, "QEMU vm=%p name=%s failed to 
spawn",
+  vm, vm->def->name);
 }
 
 VIR_DEBUG("Writing early domain status to disk");
-- 
1.8.3.1


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] libxl: fix param assignment in domainGetSchedulerParameters

[John Ferlan]

On 09/28/2016 12:33 AM, Jim Fehlig wrote:
> Due to a copy and paste error, the scheduler 'cap' parameter
> was over-writing the 'weight' parameter when preparing the
> return parameters in libxlDomainGetSchedulerParametersFlags.
> As a result, the scheduler weight was never shown when getting
> schedinfo and setting the weight failed as well
> 
> virsh  schedinfo testvm
> Scheduler  : credit
> cap: 0
> 
> virsh  schedinfo testvm --cap 50 --weight 500
> Scheduler  : credit
> error: invalid scheduler option: weight
> 
> The obvious fix is to assign the 'caps' parameter to the correct
> item in the parameter list.
> 
> Reported-by: Volo M. 
> Signed-off-by: Jim Fehlig 
> ---
>  src/libxl/libxl_driver.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

ACK (and safe)

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] doc: fix note about Xen credit scheduler

[John Ferlan]

On 09/28/2016 12:50 AM, Jim Fehlig wrote:
> Commit 6c504d6a added a note to the virsh man page about the
> deprecation of 'cap' and 'weight' settings for the credit
> scheduler. To this day, the default scheduler in Xen is credit
> and it supports setting 'cap' and 'weight'. Remove the deprecation
> notice from the note on the Xen credit scheduler.
> 
> Reported-by: Volo M. 
> Signed-off-by: Jim Fehlig 
> ---
>  tools/virsh.pod | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

ACK (and safe)

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt-php][PATCH 00/11] Resolve reverse include order

[Vasiliy Tolstov]

2016-09-27 16:11 GMT+03:00 Michal Privoznik :
> These are not pushed yet as they might be somewhat controversial.
> I'll wait if somebody wants to review them.
>
> The ultimate goal is to, well drop libvirt-php.h completely. It
> is needless. But before going there we must distribute the
> interesting parts from it around. Therefore I'm introducing some
> modules (like it should have been done from the start).


I like this idea to move extension helper to external files.

-- 
Vasiliy Tolstov,
e-mail: v.tols...@selfip.ru

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2] virsh domdisplay: introduce '--all' for showing all possible graphical display

[Chen Hanxiao]

From: Chen Hanxiao 

For one VM, it could had more than one graphical display.
Such as we coud add both vnc and spice display to a VM.

This patch introduces '--all' for showing all
possible graphical display of a active VM.

Signed-off-by: Chen Hanxiao 
---
v2: VIR_FREE befor use in loops
add descriptions in virsh.pod

 tools/virsh-domain.c | 15 ++-
 tools/virsh.pod  |  3 ++-
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 3829b17..a6124b6 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -10648,6 +10648,10 @@ static const vshCmdOptDef opts_domdisplay[] = {
  .help = N_("select particular graphical display "
 "(e.g. \"vnc\", \"spice\", \"rdp\")")
 },
+{.name = "all",
+ .type = VSH_OT_BOOL,
+ .help = N_("show all possible graphical displays")
+},
 {.name = NULL}
 };
 
@@ -10671,6 +10675,7 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
 int tmp;
 int flags = 0;
 bool params = false;
+bool all = vshCommandOptBool(cmd, "all");
 const char *xpath_fmt = "string(/domain/devices/graphics[@type='%s']/%s)";
 virSocketAddr addr;
 
@@ -10701,6 +10706,7 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
 continue;
 
 /* Create our XPATH lookup for the current display's port */
+VIR_FREE(xpath);
 if (virAsprintf(, xpath_fmt, scheme[iter], "@port") < 0)
 goto cleanup;
 
@@ -10733,6 +10739,7 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
 
 /* Attempt to get the listening addr if set for the current
  * graphics scheme */
+VIR_FREE(listen_addr);
 listen_addr = virXPathString(xpath, ctxt);
 VIR_FREE(xpath);
 
@@ -10788,6 +10795,7 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
 goto cleanup;
 
 /* Attempt to get the password */
+VIR_FREE(passwd);
 passwd = virXPathString(xpath, ctxt);
 VIR_FREE(xpath);
 
@@ -10840,12 +10848,17 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
 }
 
 /* Print out our full URI */
+VIR_FREE(output);
 output = virBufferContentAndReset();
 vshPrint(ctl, "%s", output);
 
 /* We got what we came for so return successfully */
 ret = true;
-break;
+if (!all) {
+break;
+} else {
+vshPrint(ctl, "\n");
+}
 }
 
 if (!ret) {
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 49abda9..6255b36 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -1229,7 +1229,8 @@ Output a URI which can be used to connect to the 
graphical display of the
 domain via VNC, SPICE or RDP.  The particular graphical display type can
 be selected using the B parameter (e.g. "vnc", "spice", "rdp").  If
 I<--include-password> is specified, the SPICE channel password will be
-included in the URI.
+included in the URI. If I<--all> is specified, then all show all possible
+graphical displays, for a VM could have more than one graphical displays.
 
 =item B I
 
-- 
1.8.3.1


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] libvirt-storage.c:Lines too long, use 80 character columns.

[Daniel P. Berrange]

On Wed, Sep 28, 2016 at 12:27:21AM +0530, Nitesh Konkar wrote:
> Signed-off-by: Nitesh Konkar 
> ---
>  src/libvirt-storage.c | 24 
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/src/libvirt-storage.c b/src/libvirt-storage.c
> index 48996ba..c4f2a03 100644
> --- a/src/libvirt-storage.c
> +++ b/src/libvirt-storage.c
> @@ -233,7 +233,8 @@ virConnectNumOfDefinedStoragePools(virConnectPtr conn)
>  
>  virCheckConnectReturn(conn, -1);
>  
> -if (conn->storageDriver && 
> conn->storageDriver->connectNumOfDefinedStoragePools) {
> +if (conn->storageDriver &&
> +conn->storageDriver->connectNumOfDefinedStoragePools) {
>  int ret;
>  ret = conn->storageDriver->connectNumOfDefinedStoragePools(conn);
>  if (ret < 0)
> @@ -280,7 +281,8 @@ virConnectListDefinedStoragePools(virConnectPtr conn,
>  virCheckNonNullArgGoto(names, error);
>  virCheckNonNegativeArgGoto(maxnames, error);
>  
> -if (conn->storageDriver && 
> conn->storageDriver->connectListDefinedStoragePools) {
> +if (conn->storageDriver &&
> +conn->storageDriver->connectListDefinedStoragePools) {
>  int ret;
>  ret = conn->storageDriver->connectListDefinedStoragePools(conn, 
> names, maxnames);
>  if (ret < 0)
> @@ -332,7 +334,8 @@ virConnectFindStoragePoolSources(virConnectPtr conn,
>  virCheckNonNullArgGoto(type, error);
>  virCheckReadOnlyGoto(conn->flags, error);
>  
> -if (conn->storageDriver && 
> conn->storageDriver->connectFindStoragePoolSources) {
> +if (conn->storageDriver &&
> +conn->storageDriver->connectFindStoragePoolSources) {
>  char *ret;
>  ret = conn->storageDriver->connectFindStoragePoolSources(conn, type, 
> srcSpec, flags);
>  if (!ret)
> @@ -485,7 +488,8 @@ virStoragePoolLookupByVolume(virStorageVolPtr vol)
>  
>  virCheckStorageVolReturn(vol, NULL);
>  
> -if (vol->conn->storageDriver && 
> vol->conn->storageDriver->storagePoolLookupByVolume) {
> +if (vol->conn->storageDriver &&
> +vol->conn->storageDriver->storagePoolLookupByVolume) {
>  virStoragePoolPtr ret;
>  ret = vol->conn->storageDriver->storagePoolLookupByVolume(vol);
>  if (!ret)
> @@ -1188,7 +1192,8 @@ virStoragePoolNumOfVolumes(virStoragePoolPtr pool)
>  
>  virCheckStoragePoolReturn(pool, -1);
>  
> -if (pool->conn->storageDriver && 
> pool->conn->storageDriver->storagePoolNumOfVolumes) {
> +if (pool->conn->storageDriver &&
> +pool->conn->storageDriver->storagePoolNumOfVolumes) {
>  int ret;
>  ret = pool->conn->storageDriver->storagePoolNumOfVolumes(pool);
>  if (ret < 0)
> @@ -1230,7 +1235,8 @@ virStoragePoolListVolumes(virStoragePoolPtr pool,
>  virCheckNonNullArgGoto(names, error);
>  virCheckNonNegativeArgGoto(maxnames, error);
>  
> -if (pool->conn->storageDriver && 
> pool->conn->storageDriver->storagePoolListVolumes) {
> +if (pool->conn->storageDriver &&
> +pool->conn->storageDriver->storagePoolListVolumes) {
>  int ret;
>  ret = pool->conn->storageDriver->storagePoolListVolumes(pool, names, 
> maxnames);
>  if (ret < 0)
> @@ -1297,7 +1303,8 @@ virStorageVolLookupByName(virStoragePoolPtr pool,
>  virCheckStoragePoolReturn(pool, NULL);
>  virCheckNonNullArgGoto(name, error);
>  
> -if (pool->conn->storageDriver && 
> pool->conn->storageDriver->storageVolLookupByName) {
> +if (pool->conn->storageDriver &&
> +pool->conn->storageDriver->storageVolLookupByName) {
>  virStorageVolPtr ret;
>  ret = pool->conn->storageDriver->storageVolLookupByName(pool, name);
>  if (!ret)
> @@ -1471,7 +1478,8 @@ virStorageVolCreateXML(virStoragePoolPtr pool,
>  virCheckNonNullArgGoto(xmlDesc, error);
>  virCheckReadOnlyGoto(pool->conn->flags, error);
>  
> -if (pool->conn->storageDriver && 
> pool->conn->storageDriver->storageVolCreateXML) {
> +if (pool->conn->storageDriver &&
> +pool->conn->storageDriver->storageVolCreateXML) {
>  virStorageVolPtr ret;
>  ret = pool->conn->storageDriver->storageVolCreateXML(pool, xmlDesc, 
> flags);
>  if (!ret)

NACK this kind of change is just pointless. Further you've only line
wrapped half of the long lines in this patch context.  We do *not* apply
a strict 80 character limit in libvirt.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] xen/libvirt freeze while attching network-device to vm´s - question about provided patches

[Jim Fehlig]

On 09/28/2016 07:56 AM, guido.rossmuel...@gdata.de wrote:
> Hello,
>
> we have tested the provided patches from Jim Fehlig.
> Our testsetup based on an Fedora 23 with xen 4.6.1 and libvirt 2.1
>
> The testsystem restores 15 vm´s (WindowsXP , HVM) in parallel, attached a 
> block-device to each vm and an nic, run´s the vm 60 sec and then destroy them.
>
> The testsystem did 18150 restores and we observe no problems.
>
> so from our side it looks realy good and we would embrace, if the patches 
> would be include in the libvirt code.

Just to be clear, the patch you tested is this one right?

https://www.redhat.com/archives/libvir-list/2016-September/msg00970.html

At least I hope that is the patch you tested :-). I've already pushed it to
libvirt.git and it will be included in the upcoming 2.3.0 release.

Regards,
Jim

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] doc: fix note about Xen credit scheduler

[Jim Fehlig]

On 09/28/2016 05:49 AM, John Ferlan wrote:
>
> On 09/28/2016 12:50 AM, Jim Fehlig wrote:
>> Commit 6c504d6a added a note to the virsh man page about the
>> deprecation of 'cap' and 'weight' settings for the credit
>> scheduler. To this day, the default scheduler in Xen is credit
>> and it supports setting 'cap' and 'weight'. Remove the deprecation
>> notice from the note on the Xen credit scheduler.
>>
>> Reported-by: Volo M. 
>> Signed-off-by: Jim Fehlig 
>> ---
>>  tools/virsh.pod | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
> ACK (and safe)

I've pushed this one too.

Regards,
Jim

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] libxl: fix param assignment in domainGetSchedulerParameters

[Jim Fehlig]

On 09/28/2016 05:47 AM, John Ferlan wrote:
>
> On 09/28/2016 12:33 AM, Jim Fehlig wrote:
>> Due to a copy and paste error, the scheduler 'cap' parameter
>> was over-writing the 'weight' parameter when preparing the
>> return parameters in libxlDomainGetSchedulerParametersFlags.
>> As a result, the scheduler weight was never shown when getting
>> schedinfo and setting the weight failed as well
>>
>> virsh  schedinfo testvm
>> Scheduler  : credit
>> cap: 0
>>
>> virsh  schedinfo testvm --cap 50 --weight 500
>> Scheduler  : credit
>> error: invalid scheduler option: weight
>>
>> The obvious fix is to assign the 'caps' parameter to the correct
>> item in the parameter list.
>>
>> Reported-by: Volo M. 
>> Signed-off-by: Jim Fehlig 
>> ---
>>  src/libxl/libxl_driver.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
> ACK (and safe)

Thanks! Pushed now.

Regards,
Jim

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] xen/libvirt freeze while attching network-device to vm´s - question about provided patches

[Guido.Rossmueller]

Hello,

we have tested the provided patches from Jim Fehlig.
Our testsetup based on an Fedora 23 with xen 4.6.1 and libvirt 2.1

The testsystem restores 15 vm´s (WindowsXP , HVM) in parallel, attached a 
block-device to each vm and an nic, run´s the vm 60 sec and then destroy them.

The testsystem did 18150 restores and we observe no problems.

so from our side it looks realy good and we would embrace, if the patches would 
be include in the libvirt code.

all the best
guido

Von: Jim Fehlig [jfeh...@suse.com]
Gesendet: Mittwoch, 21. September 2016 23:36
An: Rossmueller, Guido
Cc: abolo...@redhat.com; libvir-list@redhat.com; usterman...@web.de
Betreff: Re: AW: [libvirt] xen/libvirt freeze while attching network-device to 
vm´s - question about provided patches

guido.rossmuel...@gdata.de wrote:
> Hi Jim,
>
> thanks for your response ans sorry for my late answer,
>
> i´m sorry, i don´t really understand what you are missing in [2], or from 
> where in the log-output you see that an
> shutdown event is missing.

Well, during my testing of the patch I found that on occasion libxl was not
delivering a shutdown event to libvirt. But after re-reading the patch thread
I'm not sure how I mis-interpreted Ian Campbell's suggestion here

https://www.redhat.com/archives/libvir-list/2015-November/msg00851.html

I've posted a much simpler patch implementing Ian's suggestion, which so far is
working great in my testing

https://www.redhat.com/archives/libvir-list/2016-September/msg00970.html

Can you please test that patch in your environment and report back? Thanks!

Regards,
Jim


Virus checked by G Data MailSecurity
Version: AVA 25.8447 dated 28.09.2016
Virus news: www.antiviruslab.com

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] xen/libvirt freeze while attching network-device to vm´s - question about provided patches

[Guido.Rossmueller]

Hi Jim,

yes, i tested the patch 
https://www.redhat.com/archives/libvir-list/2016-September/msg00970.html

all the best guido

Von: Jim Fehlig [jfeh...@suse.com]
Gesendet: Mittwoch, 28. September 2016 16:08
An: Rossmueller, Guido
Cc: abolo...@redhat.com; libvir-list@redhat.com; usterman...@web.de
Betreff: Re: AW: AW: [libvirt] xen/libvirt freeze while attching network-device 
to vm´s - question about provided patches

On 09/28/2016 07:56 AM, guido.rossmuel...@gdata.de wrote:
> Hello,
>
> we have tested the provided patches from Jim Fehlig.
> Our testsetup based on an Fedora 23 with xen 4.6.1 and libvirt 2.1
>
> The testsystem restores 15 vm´s (WindowsXP , HVM) in parallel, attached a 
> block-device to each vm and an nic, run´s the vm 60 sec and then destroy them.
>
> The testsystem did 18150 restores and we observe no problems.
>
> so from our side it looks realy good and we would embrace, if the patches 
> would be include in the libvirt code.

Just to be clear, the patch you tested is this one right?

https://www.redhat.com/archives/libvir-list/2016-September/msg00970.html

At least I hope that is the patch you tested :-). I've already pushed it to
libvirt.git and it will be included in the upcoming 2.3.0 release.

Regards,
Jim


Virus checked by G Data MailSecurity
Version: AVA 25.8447 dated 28.09.2016
Virus news: www.antiviruslab.com

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] xen/libvirt freeze while attching network-device to vm´s - question about provided patches

[Jim Fehlig]

On 09/28/2016 08:15 AM, guido.rossmuel...@gdata.de wrote:
> Hi Jim,
>
> yes, i tested the patch 
> https://www.redhat.com/archives/libvir-list/2016-September/msg00970.html

Awesome! Thanks for testing and providing feedback.

Regards,
Jim

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] network: Add support for configuring dhcp lease time

[Martin Wilck]

On Tue, 2016-09-27 at 09:15 +0530, Nehal J Wani wrote:
> 
> Seems like the previous experiment involved a happy accident indeed.
> 

So, what are we going to do about libvirt+dnsmasq's handling of expired
leases now?

Martin

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list