[libvirt] [PATCH 5/5] libvirtd: Fix order of cleanup processing

[John Ferlan]

Current cleanup processing is ad-hoc at best - it's led to a couple of
strange and hard to diagnose timing problems and crashes.

So rather than perform cleanup in a somewhat random order, let's
perform cleanup in the exact opposite order of startup.

Signed-off-by: John Ferlan 
---
 daemon/libvirtd.c | 42 +-
 1 file changed, 29 insertions(+), 13 deletions(-)

diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 87c5b22710..92037e9070 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -1524,9 +1524,35 @@ int main(int argc, char **argv) {
 0, "shutdown", NULL, NULL);
 
  cleanup:
-virNetlinkEventServiceStopAll();
+/* NB: Order for cleanup should attempt to kept in the inverse order
+ * was was used to create/start the daemon - there are some fairly
+ * important reliances built into the startup processing that use
+ * refcnt's in order to manage objects. Removing too early could
+ * cause crashes. */
 virNetDaemonClose(dmn);
+
+virNetlinkEventServiceStopAll();
+
+if (driversInitialized) {
+/* Set via daemonRunStateInit thread in daemonStateInit.
+ * NB: It is possible that virNetlinkEventServerStart fails
+ * and we jump to cleanup before driversInitialized has
+ * been set. That could leave things inconsistent; however,
+ * resolution of that possibility is perhaps more trouble than
+ * it's worth to handle. */
+driversInitialized = false;
+virStateCleanup();
+}
+
+virObjectUnref(dmn);
+
 virNetlinkShutdown();
+
+if (pid_file_fd != -1)
+virPidFileReleasePath(pid_file, pid_file_fd);
+
+VIR_FREE(run_dir);
+
 if (statuswrite != -1) {
 if (ret != 0) {
 /* Tell parent of daemon what failed */
@@ -1537,25 +1563,15 @@ int main(int argc, char **argv) {
 }
 VIR_FORCE_CLOSE(statuswrite);
 }
-if (pid_file_fd != -1)
-virPidFileReleasePath(pid_file, pid_file_fd);
 
 VIR_FREE(sock_file);
 VIR_FREE(sock_file_ro);
 VIR_FREE(sock_file_adm);
+
 VIR_FREE(pid_file);
-VIR_FREE(remote_config_file);
-VIR_FREE(run_dir);
 
+VIR_FREE(remote_config_file);
 daemonConfigFree(config);
 
-if (driversInitialized) {
-driversInitialized = false;
-virStateCleanup();
-}
-/* Now that the hypervisor shutdown inhibition functions that use
- * 'dmn' as a parameter are done, we can finally unref 'dmn' */
-virObjectUnref(dmn);
-
 return ret;
 }
-- 
2.13.6

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

[libvirt] [PATCH 4/5] libvirtd: Alter refcnt processing for server program objects

[John Ferlan]

Instead of holding onto the reference for each program added to
a NetServer, let's Unref the program reference as soon as the
attempt is made to add to the @srv->programs list. This then
allows the virNetServerDispose to handle the final Unref of
the object for actual deletion and cleans up the cleanup: path.

Signed-off-by: John Ferlan 
---
 daemon/libvirtd.c | 24 
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 5c47e49d48..87c5b22710 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -1352,7 +1352,10 @@ int main(int argc, char **argv) {
 ret = VIR_DAEMON_ERR_INIT;
 goto cleanup;
 }
-if (virNetServerAddProgram(srv, remoteProgram) < 0) {
+
+rc = virNetServerAddProgram(srv, remoteProgram);
+virObjectUnref(remoteProgram);
+if (rc < 0) {
 ret = VIR_DAEMON_ERR_INIT;
 goto cleanup;
 }
@@ -1364,7 +1367,10 @@ int main(int argc, char **argv) {
 ret = VIR_DAEMON_ERR_INIT;
 goto cleanup;
 }
-if (virNetServerAddProgram(srv, lxcProgram) < 0) {
+
+rc = virNetServerAddProgram(srv, lxcProgram);
+virObjectUnref(lxcProgram);
+if (rc < 0) {
 ret = VIR_DAEMON_ERR_INIT;
 goto cleanup;
 }
@@ -1376,7 +1382,10 @@ int main(int argc, char **argv) {
 ret = VIR_DAEMON_ERR_INIT;
 goto cleanup;
 }
-if (virNetServerAddProgram(srv, qemuProgram) < 0) {
+
+rc = virNetServerAddProgram(srv, qemuProgram);
+virObjectUnref(qemuProgram);
+if (rc < 0) {
 ret = VIR_DAEMON_ERR_INIT;
 goto cleanup;
 }
@@ -1414,7 +1423,10 @@ int main(int argc, char **argv) {
 ret = VIR_DAEMON_ERR_INIT;
 goto cleanup;
 }
-if (virNetServerAddProgram(srvAdm, adminProgram) < 0) {
+
+rc = virNetServerAddProgram(srvAdm, adminProgram);
+virObjectUnref(adminProgram);
+if (rc < 0) {
 ret = VIR_DAEMON_ERR_INIT;
 goto cleanup;
 }
@@ -1513,10 +1525,6 @@ int main(int argc, char **argv) {
 
  cleanup:
 virNetlinkEventServiceStopAll();
-virObjectUnref(remoteProgram);
-virObjectUnref(lxcProgram);
-virObjectUnref(qemuProgram);
-virObjectUnref(adminProgram);
 virNetDaemonClose(dmn);
 virNetlinkShutdown();
 if (statuswrite != -1) {
-- 
2.13.6

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

[libvirt] [PATCH 1/5] libvirtd: Move pid_file_fd setup to before run_dir

[John Ferlan]

Once we have forked the daemon successfully, let's claim the pidfile
immediately rather than waiting for setup of run_dir.

Signed-off-by: John Ferlan 
---
 daemon/libvirtd.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 589b32192e..4fc33ba0d4 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -1258,6 +1258,12 @@ int main(int argc, char **argv) {
 }
 }
 
+/* Try to claim the pidfile, exiting if we can't */
+if ((pid_file_fd = virPidFileAcquirePath(pid_file, false, getpid())) < 0) {
+ret = VIR_DAEMON_ERR_PIDFILE;
+goto cleanup;
+}
+
 /* Ensure the rundir exists (on tmpfs on some systems) */
 if (privileged) {
 if (VIR_STRDUP_QUIET(run_dir, LOCALSTATEDIR "/run/libvirt") < 0) {
@@ -1286,12 +1292,6 @@ int main(int argc, char **argv) {
 }
 umask(old_umask);
 
-/* Try to claim the pidfile, exiting if we can't */
-if ((pid_file_fd = virPidFileAcquirePath(pid_file, false, getpid())) < 0) {
-ret = VIR_DAEMON_ERR_PIDFILE;
-goto cleanup;
-}
-
 if (virNetlinkStartup() < 0) {
 ret = VIR_DAEMON_ERR_INIT;
 goto cleanup;
-- 
2.13.6

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

[libvirt] [PATCH 3/5] libvirtd: Alter refcnt processing for domain server objects

[John Ferlan]

Whether the @srv/@srvAdm is added to the dmn->servers list or not,
the reference kept for the allocation can be dropped leaving just the
reference for being on the dmn->servers list be the sole deciding
factor when to really free the associated memory. The @dmn dispose
function (virNetDaemonDispose) will handle the Unref of the objects
during the virHashFree.

Signed-off-by: John Ferlan 
---
 daemon/libvirtd.c | 15 +++
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 73f24915df..5c47e49d48 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -1065,6 +1065,7 @@ int main(int argc, char **argv) {
 char *remote_config_file = NULL;
 int statuswrite = -1;
 int ret = 1;
+int rc;
 int pid_file_fd = -1;
 char *pid_file = NULL;
 char *sock_file = NULL;
@@ -1319,7 +1320,11 @@ int main(int argc, char **argv) {
 goto cleanup;
 }
 
-if (virNetDaemonAddServer(dmn, srv) < 0) {
+/* Add @srv to @dmn servers hash table and Unref to allow removal from
+ * hash table at @dmn disposal to decide when to free @srv */
+rc = virNetDaemonAddServer(dmn, srv);
+virObjectUnref(srv);
+if (rc < 0) {
 ret = VIR_DAEMON_ERR_INIT;
 goto cleanup;
 }
@@ -1393,7 +1398,11 @@ int main(int argc, char **argv) {
 goto cleanup;
 }
 
-if (virNetDaemonAddServer(dmn, srvAdm) < 0) {
+/* Add @srvAdm to @dmn servers hash table and Unref to allow removal from
+ * hash table at @dmn disposal to decide when to free @srvAdm */
+rc = virNetDaemonAddServer(dmn, srvAdm);
+virObjectUnref(srvAdm);
+if (rc < 0) {
 ret = VIR_DAEMON_ERR_INIT;
 goto cleanup;
 }
@@ -1509,8 +1518,6 @@ int main(int argc, char **argv) {
 virObjectUnref(qemuProgram);
 virObjectUnref(adminProgram);
 virNetDaemonClose(dmn);
-virObjectUnref(srv);
-virObjectUnref(srvAdm);
 virNetlinkShutdown();
 if (statuswrite != -1) {
 if (ret != 0) {
-- 
2.13.6

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

[libvirt] [PATCH 2/5] libvirtd: Alter order of virNetDaemonNew

[John Ferlan]

Let's be sure we can get a Daemon object before the server object.
This is a more "orderly" way to do things since the svr object would
be added to the dmn object afterwards.

Signed-off-by: John Ferlan 
---
 daemon/libvirtd.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 4fc33ba0d4..73f24915df 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -1297,6 +1297,11 @@ int main(int argc, char **argv) {
 goto cleanup;
 }
 
+if (!(dmn = virNetDaemonNew())) {
+ret = VIR_DAEMON_ERR_INIT;
+goto cleanup;
+}
+
 if (!(srv = virNetServerNew("libvirtd", 1,
 config->min_workers,
 config->max_workers,
@@ -1314,8 +1319,7 @@ int main(int argc, char **argv) {
 goto cleanup;
 }
 
-if (!(dmn = virNetDaemonNew()) ||
-virNetDaemonAddServer(dmn, srv) < 0) {
+if (virNetDaemonAddServer(dmn, srv) < 0) {
 ret = VIR_DAEMON_ERR_INIT;
 goto cleanup;
 }
-- 
2.13.6

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

[libvirt] [PATCH 0/5] libvirtd: Adjustments to startup and cleanup processing

[John Ferlan]

Essentially a followup from the review of:

daemon: fix termination/reload issues

https://www.redhat.com/archives/libvir-list/2017-October/msg01347.html

and in particular patch 3/3, a review which has exteneded into Nov.:

https://www.redhat.com/archives/libvir-list/2017-November/msg00066.html


Make a few adjustments as noted in the patches as an alternative
to patch 3/3 from above.  The cleanup path is essentially reordered
in inverse order of startup.  Other adjustments include virObjectUnref
after attempts to place object into domain or server hash tables or
linked lists.

John Ferlan (5):
  libvirtd: Move pid_file_fd setup to before run_dir
  libvirtd: Alter order of virNetDaemonNew
  libvirtd: Alter refcnt processing for domain server objects
  libvirtd: Alter refcnt processing for server program objects
  libvirtd: Fix order of cleanup processing

 daemon/libvirtd.c | 95 +--
 1 file changed, 65 insertions(+), 30 deletions(-)

-- 
2.13.6

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

Re: [libvirt] [PATCH v3 4/5] qemu: Use predictable file names for memory-backend-file

[John Ferlan]

On 11/07/2017 10:51 AM, Michal Privoznik wrote:
> In some cases management application needs to allocate memory for
> qemu upfront and then just let qemu use that. Since we don't want
> to expose path for memory-backend-file anywhere in the domain
> XML, we can generate predictable paths. In this case:
> 
>   $memoryBackingDir/libvirt/qemu/$shortName/$alias
> 
> where $shortName is result of virDomainObjGetShortName().

Just realized the s/Obj/Def/ from my previous review was missed.

John

> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_command.c|  2 +-
>  src/qemu/qemu_conf.c   | 55 +-
>  src/qemu/qemu_conf.h   |  9 ++-
>  src/qemu/qemu_driver.c | 17 ++
>  src/qemu/qemu_process.c| 65 
> ++
>  .../qemuxml2argv-cpu-numa-memshared.args   |  6 +-
>  .../qemuxml2argv-fd-memory-numa-topology.args  |  3 +-
>  .../qemuxml2argv-fd-memory-numa-topology2.args |  6 +-
>  .../qemuxml2argv-fd-memory-numa-topology3.args |  9 ++-
>  .../qemuxml2argv-hugepages-memaccess2.args |  9 ++-
>  10 files changed, 155 insertions(+), 26 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index c949afe90..c12cfec7a 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3419,7 +3419,7 @@ qemuBuildMemoryBackendStr(virJSONValuePtr *backendProps,
>  } else {
>  /* We can have both pagesize and mem source. If that's the case,
>   * prefer hugepages as those are more specific. */
> -if (qemuGetMemoryBackingPath(cfg, &memPath) < 0)
> +if (qemuGetMemoryBackingPath(def, cfg, mem->info.alias, 
> &memPath) < 0)
>  goto cleanup;
>  }
>  
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index f3cff1503..af503d31c 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -1750,9 +1750,41 @@ qemuGetDomainHupageMemPath(const virDomainDef *def,
>  }
>  
>  
> +int
> +qemuGetMemoryBackingBasePath(virQEMUDriverConfigPtr cfg,
> + char **path)
> +{
> +return virAsprintf(path, "%s/libvirt/qemu", cfg->memoryBackingDir);
> +}
> +
> +
> +int
> +qemuGetMemoryBackingDomainPath(const virDomainDef *def,
> +   virQEMUDriverConfigPtr cfg,
> +   char **path)
> +{
> +char *shortName = NULL;
> +char *base = NULL;
> +int ret = -1;
> +
> +if (!(shortName = virDomainDefGetShortName(def)) ||
> +qemuGetMemoryBackingBasePath(cfg, &base) < 0 ||
> +virAsprintf(path, "%s/%s", base, shortName) < 0)
> +goto cleanup;
> +
> +ret = 0;
> + cleanup:
> +VIR_FREE(base);
> +VIR_FREE(shortName);
> +return ret;
> +}
> +
> +
>  /**
>   * qemuGetMemoryBackingPath:
> + * @def: domain definition
>   * @cfg: the driver config
> + * @alias: memory object alias
>   * @memPath: constructed path
>   *
>   * Constructs path to memory backing dir and stores it at @memPath.
> @@ -1761,8 +1793,27 @@ qemuGetDomainHupageMemPath(const virDomainDef *def,
>   *  -1 otherwise (with error reported).
>   */
>  int
> -qemuGetMemoryBackingPath(virQEMUDriverConfigPtr cfg,
> +qemuGetMemoryBackingPath(const virDomainDef *def,
> + virQEMUDriverConfigPtr cfg,
> + const char *alias,
>   char **memPath)
>  {
> -return VIR_STRDUP(*memPath, cfg->memoryBackingDir);
> +char *domainPath = NULL;
> +int ret = -1;
> +
> +if (!alias) {
> +/* This should never happen (TM) */
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("memory device alias is not assigned"));
> +goto cleanup;
> +}
> +
> +if (qemuGetMemoryBackingDomainPath(def, cfg, &domainPath) < 0 ||
> +virAsprintf(memPath, "%s/%s", domainPath, alias) < 0)
> +goto cleanup;
> +
> +ret = 0;
> + cleanup:
> +VIR_FREE(domainPath);
> +return ret;
>  }
> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> index 9d6866816..a553e30e2 100644
> --- a/src/qemu/qemu_conf.h
> +++ b/src/qemu/qemu_conf.h
> @@ -364,6 +364,13 @@ int qemuGetDomainHupageMemPath(const virDomainDef *def,
> unsigned long long pagesize,
> char **memPath);
>  
> -int qemuGetMemoryBackingPath(virQEMUDriverConfigPtr cfg,
> +int qemuGetMemoryBackingBasePath(virQEMUDriverConfigPtr cfg,
> + char **path);
> +int qemuGetMemoryBackingDomainPath(const virDomainDef *def,
> +   virQEMUDriverConfigPtr cfg,
> +   char **path);
> +int qemuGetMemoryBackingPath(const virDomainDef *def,
> + virQEMUDriverConfigPtr cfg,
> +  

Re: [libvirt] [PATCH 4/4] tests: Add caps for QEMU 2.10.0 on ppc64

[John Ferlan]

On 11/06/2017 08:26 AM, Andrea Bolognani wrote:
> Signed-off-by: Andrea Bolognani 
> ---
>  .../qemucapabilitiesdata/caps_2.10.0.ppc64.replies | 20572 
> +++
>  tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml   |  1072 +
>  tests/qemucapabilitiestest.c   | 1 +
>  3 files changed, 21645 insertions(+)
>  create mode 100644 tests/qemucapabilitiesdata/caps_2.10.0.ppc64.replies
>  create mode 100644 tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 3/4] tests: Rename ppc64le caps to ppc64

[John Ferlan]

On 11/06/2017 08:26 AM, Andrea Bolognani wrote:
> The architecture itself is called ppc64, and it can run both in big
> endian and little endian mode - the latter is known as ppc64le.
> 
>>From the (virtual) hardware point of view, ppc64 is a more accurate
> name so it should be used here.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  .../{qemu_2.6.0.ppc64le.xml => qemu_2.6.0.ppc64.xml}  | 2 +-
>  tests/domaincapstest.c| 2 +-
>  .../{caps_2.6.0.ppc64le.replies => caps_2.6.0.ppc64.replies}  | 0
>  .../{caps_2.6.0.ppc64le.xml => caps_2.6.0.ppc64.xml}  | 0
>  .../{caps_2.9.0.ppc64le.replies => caps_2.9.0.ppc64.replies}  | 0
>  .../{caps_2.9.0.ppc64le.xml => caps_2.9.0.ppc64.xml}  | 0
>  tests/qemucapabilitiestest.c  | 4 
> ++--
>  7 files changed, 4 insertions(+), 4 deletions(-)
>  rename tests/domaincapsschemadata/{qemu_2.6.0.ppc64le.xml => 
> qemu_2.6.0.ppc64.xml} (98%)
>  rename tests/qemucapabilitiesdata/{caps_2.6.0.ppc64le.replies => 
> caps_2.6.0.ppc64.replies} (100%)
>  rename tests/qemucapabilitiesdata/{caps_2.6.0.ppc64le.xml => 
> caps_2.6.0.ppc64.xml} (100%)
>  rename tests/qemucapabilitiesdata/{caps_2.9.0.ppc64le.replies => 
> caps_2.9.0.ppc64.replies} (100%)
>  rename tests/qemucapabilitiesdata/{caps_2.9.0.ppc64le.xml => 
> caps_2.9.0.ppc64.xml} (100%)
> 

So this is just for our tests right?  I guess I see a name change like
this and I think - is there some sort of migrate or save/restore issue
we could have by changing the "name"...

Anyway - I see the the "ppc64" matches the JSON return:

{
  "return": {
"arch": "ppc64"
  },
  "id": "libvirt-3"
}

So, close my eyes and hope it all works ;-)

Reviewed-by: John Ferlan 

John

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

[libvirt] [PATCH 3/2] news: Update for vbox 5.2 support

[Dawid Zamirski]

---
 docs/news.xml | 5 +
 1 file changed, 5 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml
index 9361a9165..630784a0a 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -37,6 +37,11 @@
 
 
 
+  
+
+  vbox: Add VirtualBox 5.2 support
+
+  
   
 
   vbox: Add support for configuring storage controllers
-- 
2.14.2

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

Re: [libvirt] Libvirt config converter can't handle file not ending with new line

[Jim Fehlig]

On 11/07/2017 08:54 AM, Wim ten Have wrote:

On Tue, 7 Nov 2017 12:20:05 +
Wei Liu  wrote:


On Mon, Nov 06, 2017 at 09:41:01PM -0700, Jim Fehlig wrote:

On 10/30/2017 06:17 AM, Wei Liu wrote:

Hi Jim

I discover a problem when using xen_xl converter. When the file in
question doesn't end with a new line, I get the following error:

error: configuration file syntax error: memory conf:53: expecting a value


I'm not able to reproduce this issue. The libvirt.git tree I tried was a bit
dated, but even after updating to latest master I can't reproduce.
   

After digging a bit (but haven't read libvirt code), it appears that the
file didn't end with a new line.


I tried several files without ending new lines, going both directions
(domxml-to-native and domxml-from-native), but didn't see the mentioned
error. Perhaps your config is revealing another bug which is being
improperly reported. Can you provide an example of the problematic config?
   


I tried to get the exact file that caused the problem but it is already
destroyed by osstest.

A similar file:

http://logs.test-lab.xenproject.org/osstest/logs/115436/test-amd64-amd64-libvirt-pair/debian.guest.osstest.cfg

If you hexdump -C it, you can see the last character is 0a. Remove it and
feed the file into the converter.
Wei.


   The phenonomem you point out is indeed weird.  And my first response
   is that this is a bug parsing the cfg input.  I did little explore and
   think that src/util/virconf.c (virConfParseLong(), virConfParseValue())
   should be reworked as pointed out in below context diffs.

 git diff
diff --git a/src/util/virconf.c b/src/util/virconf.c
index 39c2bd917..bc8e57ec3 100644
--- a/src/util/virconf.c
+++ b/src/util/virconf.c
@@ -352,7 +352,7 @@ virConfParseLong(virConfParserCtxtPtr ctxt, long 
long *val)
 } else if (CUR == '+') {
 NEXT;
 }
-if ((ctxt->cur >= ctxt->end) || (!c_isdigit(CUR))) {
+if ((ctxt->cur > ctxt->end) || (!c_isdigit(CUR))) {
 virConfError(ctxt, VIR_ERR_CONF_SYNTAX, _("unterminated 
number"));
 return -1;
 }
@@ -456,7 +456,7 @@ virConfParseValue(virConfParserCtxtPtr ctxt)
 long long l = 0;

 SKIP_BLANKS;
-if (ctxt->cur >= ctxt->end) {
+if (ctxt->cur > ctxt->end) {
 virConfError(ctxt, VIR_ERR_CONF_SYNTAX, _("expecting a 
value"));
 return NULL;
 }

   I did not go beyond this yet.


Thanks Wim. I noticed Cole fixed a similar issue when parsing content from a 
file with commit 3cc2a9e0d4. But I think instead of replicating that fix in 
virConfReadString(), we should just set the end of content correctly in 
virConfParse(). I've sent a patch along those lines that fixes Wei's test case 
and doesn't regress Cole's test case


https://www.redhat.com/archives/libvir-list/2017-November/msg00286.html

Regards,
Jim

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

[libvirt] [PATCH 1/2] vbox: Add vbox 5.2 CAPI header file.

[Dawid Zamirski]

Extracted from 5.2 SDK and reindented with cppi

0001-vbox-Add-vbox-5.2-CAPI-header-file.tar.xz
Description: application/xz-compressed-tar
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] virconf: properly set the end of content

[Jim Fehlig]

There was a recent report of the xen-xl converter not handling
config files missing an ending newline

https://www.redhat.com/archives/libvir-list/2017-October/msg01353.html

Commit 3cc2a9e0 fixed a similar problem when parsing content of a
file but missed parsing in-memory content. But AFAICT, the better
fix is to properly set the end of the content when initializing the
virConfParserCtxt in virConfParse().

This commit reverts the part of 3cc2a9e0 that appends a newline to
files missing it, and fixes setting the end of content when
initializing virConfParserCtxt.

Signed-off-by: Jim Fehlig 
---
 src/util/virconf.c | 13 ++---
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/src/util/virconf.c b/src/util/virconf.c
index 39c2bd917..a82a509ca 100644
--- a/src/util/virconf.c
+++ b/src/util/virconf.c
@@ -705,7 +705,7 @@ virConfParse(const char *filename, const char *content, int 
len,
 
 ctxt.filename = filename;
 ctxt.base = ctxt.cur = content;
-ctxt.end = content + len - 1;
+ctxt.end = content + len;
 ctxt.line = 1;
 
 ctxt.conf = virConfCreate(filename, flags);
@@ -745,7 +745,7 @@ virConfReadFile(const char *filename, unsigned int flags)
 {
 char *content;
 int len;
-virConfPtr conf = NULL;
+virConfPtr conf;
 
 VIR_DEBUG("filename=%s", NULLSTR(filename));
 
@@ -757,17 +757,8 @@ virConfReadFile(const char *filename, unsigned int flags)
 if ((len = virFileReadAll(filename, MAX_CONFIG_FILE_SIZE, &content)) < 0)
 return NULL;
 
-if (len && len < MAX_CONFIG_FILE_SIZE && content[len - 1] != '\n') {
-VIR_DEBUG("appending newline to busted config file %s", filename);
-if (VIR_REALLOC_N(content, len + 2) < 0)
-goto cleanup;
-content[len++] = '\n';
-content[len] = '\0';
-}
-
 conf = virConfParse(filename, content, len, flags);
 
- cleanup:
 VIR_FREE(content);
 
 return conf;
-- 
2.14.2

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

[libvirt] [PATCH 2/2] vbox: Add support for 5.2.x

[Dawid Zamirski]

Simply add the 5.2 SDK header to the existing unified framework. No
other special handling is needed as there's no API break between
exising 5.1 and the just added 5.2.
---
 src/Makefile.am   |  1 +
 src/vbox/vbox_V5_2.c  | 13 +
 src/vbox/vbox_common.h|  2 ++
 src/vbox/vbox_storage.c   |  2 ++
 src/vbox/vbox_tmpl.c  |  2 ++
 src/vbox/vbox_uniformed_api.h |  1 +
 6 files changed, 21 insertions(+)
 create mode 100644 src/vbox/vbox_V5_2.c

diff --git a/src/Makefile.am b/src/Makefile.am
index 1d2423124..de43d0f50 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -881,6 +881,7 @@ VBOX_DRIVER_SOURCES = \
vbox/vbox_V4_3_4.c vbox/vbox_CAPI_v4_3_4.h \
vbox/vbox_V5_0.c vbox/vbox_CAPI_v5_0.h \
vbox/vbox_V5_1.c vbox/vbox_CAPI_v5_1.h \
+   vbox/vbox_V5_2.c vbox/vbox_CAPI_v5_2.h \
vbox/vbox_common.c vbox/vbox_common.h \
vbox/vbox_uniformed_api.h \
vbox/vbox_get_driver.h \
diff --git a/src/vbox/vbox_V5_2.c b/src/vbox/vbox_V5_2.c
new file mode 100644
index 0..86d40f3f7
--- /dev/null
+++ b/src/vbox/vbox_V5_2.c
@@ -0,0 +1,13 @@
+/** @file vbox_V5_2.c
+ * C file to include support for multiple versions of VirtualBox
+ * at runtime.
+ */
+
+#include 
+
+/** The API Version */
+#define VBOX_API_VERSION 5002000
+/** Version specific prefix. */
+#define NAME(name) vbox52##name
+
+#include "vbox_tmpl.c"
diff --git a/src/vbox/vbox_common.h b/src/vbox/vbox_common.h
index 05636fea2..5709ff8b4 100644
--- a/src/vbox/vbox_common.h
+++ b/src/vbox/vbox_common.h
@@ -446,6 +446,8 @@ typedef nsISupports IKeyboard;
 vbox50InstallUniformedAPI(&gVBoxAPI); \
 } else if (uVersion >= 551 && uVersion < 5001051) { \
 vbox51InstallUniformedAPI(&gVBoxAPI); \
+} else if (uVersion >= 5001051 && uVersion < 5002051) { \
+vbox52InstallUniformedAPI(&gVBoxAPI); \
 } else { \
 result = -1; \
 } \
diff --git a/src/vbox/vbox_storage.c b/src/vbox/vbox_storage.c
index c2de1ce23..672caa6f9 100644
--- a/src/vbox/vbox_storage.c
+++ b/src/vbox/vbox_storage.c
@@ -902,6 +902,8 @@ virStorageDriverPtr vboxGetStorageDriver(uint32_t uVersion)
 vbox50InstallUniformedAPI(&gVBoxAPI);
 } else if (uVersion >= 551 && uVersion < 5001051) {
 vbox51InstallUniformedAPI(&gVBoxAPI);
+} else if (uVersion >= 5001051 && uVersion < 5002051) {
+vbox52InstallUniformedAPI(&gVBoxAPI);
 } else {
 return NULL;
 }
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index 98d4bbf0d..88792c992 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -65,6 +65,8 @@
 # include "vbox_CAPI_v5_0.h"
 #elif VBOX_API_VERSION == 5001000
 # include "vbox_CAPI_v5_1.h"
+#elif VBOX_API_VERSION == 5002000
+# include "vbox_CAPI_v5_2.h"
 #else
 # error "Unsupport VBOX_API_VERSION"
 #endif
diff --git a/src/vbox/vbox_uniformed_api.h b/src/vbox/vbox_uniformed_api.h
index c51191e7d..65c24d094 100644
--- a/src/vbox/vbox_uniformed_api.h
+++ b/src/vbox/vbox_uniformed_api.h
@@ -564,5 +564,6 @@ void vbox43InstallUniformedAPI(vboxUniformedAPI *pVBoxAPI);
 void vbox43_4InstallUniformedAPI(vboxUniformedAPI *pVBoxAPI);
 void vbox50InstallUniformedAPI(vboxUniformedAPI *pVBoxAPI);
 void vbox51InstallUniformedAPI(vboxUniformedAPI *pVBoxAPI);
+void vbox52InstallUniformedAPI(vboxUniformedAPI *pVBoxAPI);
 
 #endif /* VBOX_UNIFORMED_API_H */
-- 
2.14.2

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

[libvirt] [PATCH 0/2] vbox: Add support for 5.2.x

[Dawid Zamirski]

With each minor VBOX release a new SDK header needs to be added and wired up to
the driver. This is basically the same as last time [1]. The first patch will
be sent as a compressed attachment because it's a large header file taken from
the SDK that would not pass the mailing list size constraints.

[1] https://www.redhat.com/archives/libvir-list/2016-November/msg00355.html
Dawid Zamirski (2):
  vbox: Add vbox 5.2 CAPI header file.
  vbox: Add support for 5.2.x

 src/Makefile.am   | 1 +
 src/vbox/vbox_CAPI_v5_2.h | 26870 
 src/vbox/vbox_V5_2.c  |13 +
 src/vbox/vbox_common.h| 2 +
 src/vbox/vbox_storage.c   | 2 +
 src/vbox/vbox_tmpl.c  | 2 +
 src/vbox/vbox_uniformed_api.h | 1 +
 7 files changed, 26891 insertions(+)
 create mode 100644 src/vbox/vbox_CAPI_v5_2.h
 create mode 100644 src/vbox/vbox_V5_2.c

-- 
2.14.2

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

Re: [libvirt] [PATCH 2/4] tests: Add caps for QEMU 2.10.0 on aarch64 (GICv3)

[John Ferlan]

On 11/06/2017 08:26 AM, Andrea Bolognani wrote:
> Signed-off-by: Andrea Bolognani 
> ---
>  .../caps_2.10.0-gicv3.aarch64.replies  | 16535 
> +++
>  .../caps_2.10.0-gicv3.aarch64.xml  |   303 +
>  tests/qemucapabilitiestest.c   | 1 +
>  3 files changed, 16839 insertions(+)
>  create mode 100644 
> tests/qemucapabilitiesdata/caps_2.10.0-gicv3.aarch64.replies
>  create mode 100644 tests/qemucapabilitiesdata/caps_2.10.0-gicv3.aarch64.xml
> 

Reviewed-by: John Ferlan 


John

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

Re: [libvirt] [PATCH 1/4] tests: Add caps for QEMU 2.10.0 on aarch64 (GICv2)

[John Ferlan]

On 11/06/2017 08:26 AM, Andrea Bolognani wrote:
> Signed-off-by: Andrea Bolognani 
> ---
>  .../caps_2.10.0-gicv2.aarch64.replies  | 16535 
> +++
>  .../caps_2.10.0-gicv2.aarch64.xml  |   303 +
>  tests/qemucapabilitiestest.c   | 1 +
>  3 files changed, 16839 insertions(+)
>  create mode 100644 
> tests/qemucapabilitiesdata/caps_2.10.0-gicv2.aarch64.replies
>  create mode 100644 tests/qemucapabilitiesdata/caps_2.10.0-gicv2.aarch64.xml
> 

/me wonders if these start falling into the trivial category some day...
 Too bad there wasn't some automated way to do it ;-)

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 0/3] cputest: Add more CPU data files

[John Ferlan]

On 11/02/2017 03:57 PM, Jiri Denemark wrote:
> Jiri Denemark (3):
>   cputest: Do not drop v[0-9] from CPU names
>   cputest: Add data for Intel(R) Xeon(R) CPU E5-2650 v4
>   cputest: Add data for Intel(R) Core(TM) i7-7700 CPU
> 

Looks reasonable to me...  Passes build, syntax-check

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 1/2] docs: Update vbox driver documentation.

[Dawid Zamirski]

On Tue, 2017-11-07 at 17:05 -0500, John Ferlan wrote:
> 
> On 11/07/2017 04:36 PM, Dawid Zamirski wrote:
> > * libvirt no longer supports vbox <= 3.x
> > * update XML definition sample to show how to attach disks to
> > VBOX's SAS
> >   controller and how to change IDE controller model.
> > * update XML to show how to create RDP display with autoport.
> > ---
> >  docs/drvvbox.html.in | 28 +++-
> >  1 file changed, 27 insertions(+), 1 deletion(-)
> > 
> > diff --git a/docs/drvvbox.html.in b/docs/drvvbox.html.in
> > index 63f166b24..a56f5db11 100644
> > --- a/docs/drvvbox.html.in
> > +++ b/docs/drvvbox.html.in
> > @@ -5,7 +5,7 @@
> >  VirtualBox hypervisor driver
> >  
> >  The libvirt VirtualBox driver can manage any VirtualBox
> > version
> > -from version 2.2 onwards.
> > +from version 4.0 onwards.
> 
> Hmmm... we should have changed that as part of commit id '73c6f16ba'
> or
> 'ccdf108c'.
> 
> Mind if I add a "(since libvirt 3.0.0)"
> prior
> to the end of line?
> 

Sure no problem :-)

> E.g.:
> 
>The libvirt VirtualBox driver can manage any VirtualBox
> version
>from version 4.0 onwards
>(since libvirt 3.0.0).
> 
> >  
> >  
> >  Project Links
> > @@ -68,6 +68,14 @@ vbox+ssh://u...@example.com/session  (remote
> > access, SSH tunnelled)
> >
> >  
> >
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> >  
> >
> >
> > @@ -79,6 +87,19 @@ vbox+ssh://u...@example.com/session  (remote
> > access, SSH tunnelled)
> >
> >  
> >  
> > +
> > +
> > +  
> > +  
> > +
> > +
> > +
> > +
> > +  
> > +  
> > +  
 > unit='0'/>
> > +
> > +
> >  
> >
> >
> > @@ -101,6 +122,11 @@ vbox+ssh://u...@example.com/session  (remote
> > access, SSH tunnelled)
> >
> >
> >  
> > +
> 
> There's some extraneous whitespace above as git am so kindly told me:
> 
> Applying: docs: Update vbox driver documentation.
> .git/rebase-apply/patch:57: trailing whitespace.
> 
> warning: 1 line adds whitespace errors.
> 
> I will fix up before pushing...
> 
> John
> 
> > +
> > +
> > +
> > +
> >  
> >  
> >  
> > 

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

Re: [libvirt] [PATCH 0/2] Update documentation for latest vbox changes.

[John Ferlan]

On 11/07/2017 04:36 PM, Dawid Zamirski wrote:
> * update docs/drvvbox.html.in to show support for 
> * update docs/fomatdomain.html.in to describe autoport behavior in the
>   vbox driver
> 
> Dawid Zamirski (2):
>   docs: Update vbox driver documentation.
>   docs: Document autoport behavior in the vbox driver
> 
>  docs/drvvbox.html.in  | 28 +++-
>  docs/formatdomain.html.in |  5 -
>  2 files changed, 31 insertions(+), 2 deletions(-)
> 

Reviewed-by: John Ferlan 
(series)

I'll push once I get feedback from you regarding patch 1 question.

John

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

Re: [libvirt] [PATCH 1/2] docs: Update vbox driver documentation.

[John Ferlan]

On 11/07/2017 04:36 PM, Dawid Zamirski wrote:
> * libvirt no longer supports vbox <= 3.x
> * update XML definition sample to show how to attach disks to VBOX's SAS
>   controller and how to change IDE controller model.
> * update XML to show how to create RDP display with autoport.
> ---
>  docs/drvvbox.html.in | 28 +++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/drvvbox.html.in b/docs/drvvbox.html.in
> index 63f166b24..a56f5db11 100644
> --- a/docs/drvvbox.html.in
> +++ b/docs/drvvbox.html.in
> @@ -5,7 +5,7 @@
>  VirtualBox hypervisor driver
>  
>  The libvirt VirtualBox driver can manage any VirtualBox version
> -from version 2.2 onwards.
> +from version 4.0 onwards.

Hmmm... we should have changed that as part of commit id '73c6f16ba' or
'ccdf108c'.

Mind if I add a "(since libvirt 3.0.0)" prior
to the end of line?

E.g.:

   The libvirt VirtualBox driver can manage any VirtualBox version
   from version 4.0 onwards
   (since libvirt 3.0.0).

>  
>  
>  Project Links
> @@ -68,6 +68,14 @@ vbox+ssh://u...@example.com/session  (remote access, SSH 
> tunnelled)
>
>  
>
> +
> +
> +
> +
> +
> +
> +
> +
>  
>
>
> @@ -79,6 +87,19 @@ vbox+ssh://u...@example.com/session  (remote access, SSH 
> tunnelled)
>
>  
>  
> +
> +
> +  
> +  
> +
> +
> +
> +
> +  
> +  
> +  
 unit='0'/>
> +
> +
>  
>
>
> @@ -101,6 +122,11 @@ vbox+ssh://u...@example.com/session  (remote access, SSH 
> tunnelled)
>
>
>  
> +

There's some extraneous whitespace above as git am so kindly told me:

Applying: docs: Update vbox driver documentation.
.git/rebase-apply/patch:57: trailing whitespace.

warning: 1 line adds whitespace errors.

I will fix up before pushing...

John

> +
> +
> +
> +
>  
>  
>  
> 

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

Re: [libvirt] [PATCH v3 0/5] Predictable file names for memory-backend-file

[John Ferlan]

On 11/07/2017 10:50 AM, Michal Privoznik wrote:
> v3 of:
> 
> https://www.redhat.com/archives/libvir-list/2017-October/msg01091.html
> 
> diff to v2:
> - Pushed 1/4 and 2/4 from the original series
> - Split 3/4 into smaller patches
> 
> Michal Privoznik (5):
>   qemu: Set alias for memory cell in qemuBuildMemoryCellBackendStr
>   qemu: Rename qemuProcessBuildDestroyHugepagesPath
>   qemu: Destroy whole memory tree
>   qemu: Use predictable file names for memory-backend-file
>   news: Document predictable file names for memory-backend-file
> 
>  docs/news.xml  |  11 ++
>  src/qemu/qemu_command.c|   9 +-
>  src/qemu/qemu_conf.c   |  55 -
>  src/qemu/qemu_conf.h   |   9 +-
>  src/qemu/qemu_driver.c |  17 +++
>  src/qemu/qemu_hotplug.c|   2 +-
>  src/qemu/qemu_process.c| 137 
> +++--
>  src/qemu/qemu_process.h|   8 +-
>  .../qemuxml2argv-cpu-numa-memshared.args   |   6 +-
>  .../qemuxml2argv-fd-memory-numa-topology.args  |   3 +-
>  .../qemuxml2argv-fd-memory-numa-topology2.args |   6 +-
>  .../qemuxml2argv-fd-memory-numa-topology3.args |   9 +-
>  .../qemuxml2argv-hugepages-memaccess2.args |   9 +-
>  13 files changed, 218 insertions(+), 63 deletions(-)
> 

There's a couple of things to clean up, but in general with those
cleanups in place,

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH v3 5/5] news: Document predictable file names for memory-backend-file

[John Ferlan]

On 11/07/2017 10:51 AM, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik 
> ---
>  docs/news.xml | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/docs/news.xml b/docs/news.xml
> index ef855d895..fb0ded97c 100644
> --- a/docs/news.xml
> +++ b/docs/news.xml
> @@ -37,6 +37,17 @@
>  
>  
>  
> +  
> +
> +  qemu: Generate predictable paths for qemu memory backends
> +
> +
> +  In some cases management applications need to know
> +  paths passed to memory-backend-file objects upfront.
> +  Libvirt now generates predictable paths so applications
> +  can prepare the files if they need to do so.
> +
> +  

Not quite sure this is in the right place... I think you added it to the
comment section rather than that 3.10.0 section... At least that's what
git am did for me

The adjustment is rather obvious

John
>  
>  
>  
> 

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

[libvirt] [PATCH 2/2] docs: Document autoport behavior in the vbox driver

[Dawid Zamirski]

---
 docs/formatdomain.html.in | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 92e14a919..d3cabac44 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -6100,7 +6100,10 @@ qemu-kvm -net nic,model=? /dev/null
   TCP port number (with -1 as legacy syntax indicating that it 
should
   be auto-allocated). The autoport attribute is the 
new
   preferred syntax for indicating auto-allocation of the TCP port 
to
-  use. The multiUser attribute is a boolean deciding
+  use. In the VirtualBox driver, the autoport will 
make
+  the hypervisor pick available port from 3389-3689 range when the 
VM
+  is started. The chosen port will be reflected in the 
port
+  attribute. The multiUser attribute is a boolean 
deciding
   whether multiple simultaneous connections to the VM are 
permitted.
   The replaceUser attribute is a boolean deciding 
whether
   the existing connection must be dropped and a new connection must
-- 
2.14.2

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

[libvirt] [PATCH 0/2] Update documentation for latest vbox changes.

[Dawid Zamirski]

* update docs/drvvbox.html.in to show support for 
* update docs/fomatdomain.html.in to describe autoport behavior in the
  vbox driver

Dawid Zamirski (2):
  docs: Update vbox driver documentation.
  docs: Document autoport behavior in the vbox driver

 docs/drvvbox.html.in  | 28 +++-
 docs/formatdomain.html.in |  5 -
 2 files changed, 31 insertions(+), 2 deletions(-)

-- 
2.14.2

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

[libvirt] [PATCH 1/2] docs: Update vbox driver documentation.

[Dawid Zamirski]

* libvirt no longer supports vbox <= 3.x
* update XML definition sample to show how to attach disks to VBOX's SAS
  controller and how to change IDE controller model.
* update XML to show how to create RDP display with autoport.
---
 docs/drvvbox.html.in | 28 +++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/docs/drvvbox.html.in b/docs/drvvbox.html.in
index 63f166b24..a56f5db11 100644
--- a/docs/drvvbox.html.in
+++ b/docs/drvvbox.html.in
@@ -5,7 +5,7 @@
 VirtualBox hypervisor driver
 
 The libvirt VirtualBox driver can manage any VirtualBox version
-from version 2.2 onwards.
+from version 4.0 onwards.
 
 
 Project Links
@@ -68,6 +68,14 @@ vbox+ssh://u...@example.com/session  (remote access, SSH 
tunnelled)
   
 
   
+
+
+
+
+
+
+
+
 
   
   
@@ -79,6 +87,19 @@ vbox+ssh://u...@example.com/session  (remote access, SSH 
tunnelled)
   
 
 
+
+
+  
+  
+
+
+
+
+  
+  
+  

+
+
 
   
   
@@ -101,6 +122,11 @@ vbox+ssh://u...@example.com/session  (remote access, SSH 
tunnelled)
   
   
 
+
+
+
+
+
 
 
 
-- 
2.14.2

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

Re: [libvirt] [PATCH v3 3/5] qemu: Destroy whole memory tree

[John Ferlan]

On 11/07/2017 10:51 AM, Michal Privoznik wrote:
> When removing path where huge pages are call virFileDeleteTree
> instead of plain rmdir(). The reason is that in the near future
> there's going to be more in the path than just files - some
> subdirs. Therefore plain rmdir() is not going to be enough.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_process.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index e27cd0d40..8eef2794e 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3348,7 +3348,7 @@ qemuProcessBuildDestroyMemoryPathsImpl(virQEMUDriverPtr 
> driver,
>  return -1;
>  }
>  } else {
> -if (rmdir(path) < 0 &&
> +if (virFileDeleteTree(path) < 0 &&
>  errno != ENOENT)
>  VIR_WARN("Unable to remove hugepage path: %s (errno=%d)",
>   path, errno);

No way ENOENT could be returned here since virFileDeleteTree checks that
first...

Also virFileDeleteTree will emit a virReportSystemError on rmdir failure
(that also has an ENOENT check), plus any number of other checks.

Since this code path returns 0/success, then we should probably
"consume" the Last error message and splat it as the VIR_WARN message or
just decide to actually return an error here now.

John

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

Re: [libvirt] [PATCH v3 2/5] qemu: Rename qemuProcessBuildDestroyHugepagesPath

[John Ferlan]

On 11/07/2017 10:51 AM, Michal Privoznik wrote:
> At the same time, move its internals into a separate function so
> that they can be reused.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_hotplug.c |  2 +-
>  src/qemu/qemu_process.c | 76 
> +
>  src/qemu/qemu_process.h |  8 +++---
>  3 files changed, 50 insertions(+), 36 deletions(-)
> 

[...]

> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 7df440ee4..e27cd0d40 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3324,11 +3324,45 @@ qemuProcessNeedHugepagesPath(virDomainDefPtr def,
>  }
>  
>  
> +static int
> +qemuProcessBuildDestroyMemoryPathsImpl(virQEMUDriverPtr driver,
> +   virDomainDefPtr def,
> +   const char *path,
> +   bool build)
> +{
> +if (build) {
> +if (virFileExists(path))
> +return 0;
> +
> +if (virFileMakePathWithMode(path, 0700) < 0) {
> +virReportSystemError(errno,
> + _("Unable to create %s"),
> + path);
> +return -1;
> +}
> +
> +if (qemuSecurityDomainSetPathLabel(driver->securityManager,
> +   def, path) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +_("Unable to label %s"), path);
> +return -1;
> +}
> +} else {
> +if (rmdir(path) < 0 &&
> +errno != ENOENT)
> +VIR_WARN("Unable to remove hugepage path: %s (errno=%d)",

This won't be hugepage specific soon...

John

> + path, errno);
> +}
> +
> +return 0;
> +}
> +
> +

[...]

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

Re: [libvirt] [PATCH v3 13/13] docs: Update news.xml with vbox changes.

[John Ferlan]

On 11/07/2017 01:49 PM, Dawid Zamirski wrote:
> ---
>  docs/news.xml | 74 
> +++
>  1 file changed, 74 insertions(+)
> 

Perhaps more verbose than some would like, but I'm perfectly fine with
it!  There's a couple of syntax things... Which I'll fix up before pushing.

> diff --git a/docs/news.xml b/docs/news.xml
> index ef855d895..454c63d8b 100644
> --- a/docs/news.xml
> +++ b/docs/news.xml
> @@ -37,8 +37,82 @@
>  
>  
>  
> +  
> +
> +  vbox: Add support for configuring storage controllers
> +
> +
> +  The VirtualBox driver now supports the 
> 
> +  element in the domain XML for configuring storage controllers in 
> VBOX
> +  VMs.  Additionally, libvirt's domain XML schema was updated to 
> allow
> +  optional model attribute for  +  type='ide'> which is used by the VBOX driver to set 
> the
> +  IDE controller model to be one of 'piix4', 'piix4' (default), or
> +  'ich6'.  Finally, with this change dumpxml generates
> +   elements that correspond to current
> +  VBOX VM storage controller configuration.
> +
> +  
> +  
> +
> +  vbox: Add support for attaching empty removable disks
> +
> +
> +  The VirutalBox driver now supports adding CD-ROM and floppy disk
> +  devices that do not have the disk source specified. Previously such
> +  devices were silently ignored.
> +
> +  
> +  
> +
> +  vbox: Add support for attaching SAS storage controllers
> +
> +
> +  In VirtualBox, SCSI and SAS are distinct controller types whereas
> +  libvirt does not make such distinction. Therefore, the VBOX driver 
> was
> +  updated to allow attaching SAS controllers via  +  type='scsi' model='lsisas1068'> element. If there are
> +  both SCSI and SAS controllers present in the VBOX VM, the domain 
> XML
> +  can associate the disk device using the 
> 

> +  element with the controller attribute, and optionally,
> +  set the port via unit attribute.
> +
> +  
>  
>  
> +  
> +
> +  vbox: Do not ignore failures to attach disk devices when defining
> +
> +
> +  The define now fails and reports an error if any of 
> the
> +  controller or disk devices specified in 
> the
> +  domain XML fail to attach to the VirtualBox VM.
> +
> +  
> +  
> +
> +  vbox: Fix dumpxml to always output disk devices 

Cannot have  in  and there was an extra space at the end
of the line.

> +
> +
> +  The VirtualBox driver was ignoring any disk devices in
> +  dumpxml output if there was a SAS storage controller
> +  attached to the VM.
> +
> +  
> +  
> +
> +  vbox: Fix dumpxml to always generate valid domain XML

Cannot have  in  +
> +
> +  When a VirtualBox VM has multiple disks attached, each to a 
> different
> +  storage controller that uses 'sd' prefix for block device names 
> e.g.
> +  one disk attached to SATA and one to SCSI controller, it no longer
> +  generates XML where both would have 'sda' device name assigned.
> +  Instead it properly assigns 'sda' and 'sdb' to those disks in the
> +  order of appearance.
> +
> +  
>  
>
>
> 

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

Re: [libvirt] [PATCH v3 00/13] vbox: Improve handling of storage devices

[John Ferlan]

On 11/07/2017 01:49 PM, Dawid Zamirski wrote:
> v2:
> https://www.redhat.com/archives/libvir-list/2017-October/msg01108.html
> 
> Changes since v2:
>  * Rebase on master
>  * Do not segfault in 'Cleanup partially-defined VM on failure' when
>code jumps to cleanup and machine is NULL
>  * Take out SAS controller handling code into a separate patch
>  * Split up the snapshot function cleanup code into separate patches
>  * Fixed code formatting as pointed out in review
>  * Rename vboxDumpIDEHDDs -> vboxDumpDisks into a separate patch
>  * Add docs/news.xml patch to describe improvements/fixes in the series
> 
> Dawid Zamirski (13):
>   vbox: Cleanup partially-defined VM on failure
>   vbox: Process  element in domain XML
>   vbox: Add vboxDumpStorageControllers
>   vbox: Rename vboxDumpIDEHDDs to vboxDumpDisks
>   vbox: Cleanup/prepare snasphot dumpxml functions
>   vbox: Do not free disk definitions on cleanup
>   vbox: Swap vboxSnapshotGetReadOnlyDisks arguments
>   vbox: Correctly generate drive name in dumpxml
>   vbox: Cleanup vboxDumpDisks implementation
>   vbox: Process empty removable disks in dumpxml
>   vbox: Generate disk address element in dumpxml
>   vbox: Add SAS controller support
>   docs: Update news.xml with vbox changes.
> 
>  docs/news.xml  |   74 
>  src/vbox/vbox_common.c | 1105 
> +++-
>  src/vbox/vbox_common.h |8 +
>  3 files changed, 792 insertions(+), 395 deletions(-)
> 

I noted a couple of things along the way - one is something perhaps for
later and the other I fixed up...

Reviewed-by: John Ferlan 
(series)

... and will push in a little while...

Tks,

John

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

Re: [libvirt] [PATCH v3 08/13] vbox: Correctly generate drive name in dumpxml

[John Ferlan]

On 11/07/2017 01:49 PM, Dawid Zamirski wrote:
> If a VBOX VM has e.g. a SATA and SCSI disk attached, the XML generated
> by dumpxml used to produce "sda" for both of those disks. This is an
> invalid domain XML as libvirt does not allow duplicate device names. To
> address this, keep the running total of disks that will use "sd" prefix
> for device name and pass it to the vboxGenerateMediumName which no
> longer tries to "compute" the value based only on current and max
> port and slot values. After this the vboxGetMaxPortSlotValues is not
> needed and was deleted.
> ---
>  src/vbox/vbox_common.c | 192 
> ++---
>  1 file changed, 52 insertions(+), 140 deletions(-)
> 
> diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
> index 4d39beb1e..57b0fd515 100644
> --- a/src/vbox/vbox_common.c
> +++ b/src/vbox/vbox_common.c
> @@ -290,61 +290,6 @@ static int openSessionForMachine(vboxDriverPtr data, 
> const unsigned char *dom_uu
>  return 0;
>  }
>  
> -/**
> - * function to get the values for max port per
> - * instance and max slots per port for the devices
> - *
> - * @returns true on Success, false on failure.
> - * @param   vboxInput IVirtualBox pointer
> - * @param   maxPortPerInst  Output array of max port per instance
> - * @param   maxSlotPerPort  Output array of max slot per port
> - *
> - */
> -
> -static bool vboxGetMaxPortSlotValues(IVirtualBox *vbox,
> - PRUint32 *maxPortPerInst,
> - PRUint32 *maxSlotPerPort)
> -{
> -ISystemProperties *sysProps = NULL;
> -
> -if (!vbox)
> -return false;
> -
> -gVBoxAPI.UIVirtualBox.GetSystemProperties(vbox, &sysProps);
> -
> -if (!sysProps)
> -return false;
> -
> -gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
> - StorageBus_IDE,
> - 
> &maxPortPerInst[StorageBus_IDE]);
> -gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
> - StorageBus_SATA,
> - 
> &maxPortPerInst[StorageBus_SATA]);
> -gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
> - StorageBus_SCSI,
> - 
> &maxPortPerInst[StorageBus_SCSI]);
> -gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
> - 
> StorageBus_Floppy,
> - 
> &maxPortPerInst[StorageBus_Floppy]);
> -
> -gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
> -  
> StorageBus_IDE,
> -  
> &maxSlotPerPort[StorageBus_IDE]);
> -gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
> -  
> StorageBus_SATA,
> -  
> &maxSlotPerPort[StorageBus_SATA]);
> -gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
> -  
> StorageBus_SCSI,
> -  
> &maxSlotPerPort[StorageBus_SCSI]);
> -gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
> -  
> StorageBus_Floppy,
> -  
> &maxSlotPerPort[StorageBus_Floppy]);
> -
> -VBOX_RELEASE(sysProps);
> -
> -return true;
> -}
>  
>  /**
>   * function to generate the name for medium,
> @@ -352,57 +297,39 @@ static bool vboxGetMaxPortSlotValues(IVirtualBox *vbox,
>   *
>   * @returns null terminated string with device name or NULL
>   *  for failures
> - * @param   connInput Connection Pointer
>   * @param   storageBus  Input storage bus type
> - * @param   deviceInst  Input device instance number
>   * @param   devicePort  Input port number
>   * @param   deviceSlot  Input slot number
> - * @param   aMaxPortPerInst Input array of max port per device instance
> - * @param   aMaxSlotPerPort Input array of max slot per device port
> - *
> + * @param   sdCount Running total of disk devices with "sd" 
> prefix
>   */
> -static char *vboxGenerateMediumName(PRUint32 storageBus,
> -PRInt32 deviceInst,
> -PRInt32 devicePort,
> -

Re: [libvirt] [PATCH] numa: avoid failure in nodememstats on non-NUMA systems

[John Ferlan]

On 09/22/2017 10:12 AM, Viktor Mihajlovski wrote:
> libvirt reports a fake NUMA topology in virConnectGetCapabilities
> even if built without numactl support. The fake NUMA topology consists
> of a single cell representing the host's cpu and memory resources.
> Currently this is the case for ARM and s390[x] RPM builds.
> 
> A client iterating over NUMA cells obtained via virConnectGetCapabilities
> and invoking virNodeGetMemoryStats on them will see an internal failure
> "NUMA isn't available on this host". An example for such a client is
> VDSM.

Message is from virNumaGetMaxNode call...

> 
> Since the intention seems to be that libvirt always reports at least
> a single cell it is necessary to return "fake" node memory statistics
> matching the previously reported fake cell in case NUMA isn't supported
> on the system.
> 
> Signed-off-by: Viktor Mihajlovski 
> ---
>  src/util/virhostmem.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/src/util/virhostmem.c b/src/util/virhostmem.c
> index a9ba278..fa04a37 100644
> --- a/src/util/virhostmem.c
> +++ b/src/util/virhostmem.c
> @@ -267,6 +267,14 @@ virHostMemGetStats(int cellNum ATTRIBUTE_UNUSED,
>  FILE *meminfo;
>  int max_node;
>  
> +/* 
 ^
A 'git am' told me there was an extra blank space after the * ;-)

> + * Even if built without numactl, libvirt claims
> + * to have a one-cells NUMA topology. In such a
> + * case return the statistics for the entire host.
> + */
> +if (!virNumaIsAvailable() && cellNum == 0)
> +cellNum = VIR_NODE_MEMORY_STATS_ALL_CELLS;
> +

I'm NUMA challenged, but this certainly seems reasonable... and it
appears you have an ACK from Boris anyway... So I'll give it until
tomorrow for anyone else to throw up their hands and say this isn't
right. ;-)... if no one does, then I'll push with the space fix...

Reviewed-by: John Ferlan 

John



>  if (cellNum == VIR_NODE_MEMORY_STATS_ALL_CELLS) {
>  if (VIR_STRDUP(meminfo_path, MEMINFO_PATH) < 0)
>  return -1;
> 

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

[libvirt] [PATCH go-xml] Add support for host paravirt bootloader, used by Xen and bhyve.

[Brandon Bergren]

---
 domain.go  |  2 ++
 domain_test.go | 16 
 2 files changed, 18 insertions(+)

diff --git a/domain.go b/domain.go
index aea54a5..debc5b2 100644
--- a/domain.go
+++ b/domain.go
@@ -797,6 +797,8 @@ type Domain struct {
VCPU*DomainVCPU  `xml:"vcpu"`
VCPUs   *DomainVCPUs `xml:"vcpus"`
CPUTune *DomainCPUTune   `xml:"cputune"`
+   Bootloader  string   `xml:"bootloader,omitempty"`
+   BootloaderArgs  string   `xml:"bootloader_args,omitempty"`
Resource*DomainResource  `xml:"resource"`
SysInfo *DomainSysInfo   `xml:"sysinfo"`
OS  *DomainOS`xml:"os"`
diff --git a/domain_test.go b/domain_test.go
index a7f629e..1ad5125 100644
--- a/domain_test.go
+++ b/domain_test.go
@@ -1722,6 +1722,22 @@ var domainTestData = []struct {
``,
},
},
+   /* Host Bootloader -- bhyve, Xen */
+   {
+   Object: &Domain{
+   Type: "bhyve",
+   Name: "test",
+   Bootloader: "/usr/local/sbin/grub-bhyve",
+   BootloaderArgs: "-r cd0 -m /tmp/test-device.map -M 
1024M linuxguest",
+   },
+   Expected: []string{
+   ``,
+   `  test`,
+   `  /usr/local/sbin/grub-bhyve`,
+   `  -r cd0 -m /tmp/test-device.map -M 
1024M linuxguest`,
+   ``,
+   },
+   },
 }
 
 func TestDomain(t *testing.T) {
-- 
2.14.1

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

Re: [libvirt] [PATCH v2 12/15] vbox: Correctly generate drive name in dumpxml

[Dawid Zamirski]

On Fri, 2017-11-03 at 09:52 -0400, John Ferlan wrote:
> 
> On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
> > If a VBOX VM has e.g. a SATA and SCSI disk attached, the XML
> > generated
> > by dumpxml used to produce "sda" for both of those disks. This is
> > an
> > invalid domain XML as libvirt does not allow duplicate device
> > names. To
> > address this, keep the running total of disks that will use "sd"
> > prefix
> > for device name and pass it to the vboxGenerateMediumName which no
> > longer tries to "compute" the value based only on current and max
> > port and slot values. After this the vboxGetMaxPortSlotValues is
> > not
> > needed and was deleted.
> > ---
> >  src/vbox/vbox_common.c | 414 +--
> > --
> >  1 file changed, 177 insertions(+), 237 deletions(-)
> > 
> 
> I think this needs a bit more splitting.
> 
> 1. As noted in patch 10, managing StorageBus_SAS should already be
> separated so that it doesn't show as a new range/comparison check
> here
> 
> 2. The rename of vboxDumpIDEHDDs to vboxDumpDisks and change from
> void
> to int should be its own patch. While it seems superfluous, it makes
> review that much easier.  NB: I have a couple more thoughts within
> the
> function about
> 
> 3. I think the changes to vboxSnapshotGetReadWriteDisks and
> vboxSnapshotGetReadOnlyDisks could be in a separate patch and is
> where
> the vboxGetMaxPortSlotValues would get deleted. Although I'm not 100%
> sure w/r/t the relationship. Perhaps once vboxDumpDisks is cleaned up
> a
> bit before making the change described in the commit message it'd be
> clearer. Just a lot going on.
> 
> 

Hi John,

I've just send V3 [1] when I separated most of the changes into
individual patches as you requested. However, I could not separate
vboxSnapshotGetReadWrite and vboxSnapshotGetReadOnly disks completely
as the changes made to vboxGenerateMedium name must be combined with
the removal of vboxGetMaxPortSlotValues so instead, I've isolated the
code movement changes where I moved the code to read VBOX's storage
controller, slot and port to the beginning of the loop body - either
way, I think it's much cleaner and easier to follow the changes now.

[1] https://www.redhat.com/archives/libvir-list/2017-November/msg00252.
html

Dawid

> 
> > diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
> > index 715eb670e..9dc36a1b2 100644
> > --- a/src/vbox/vbox_common.c
> > +++ b/src/vbox/vbox_common.c
> > @@ -290,61 +290,6 @@ static int openSessionForMachine(vboxDriverPtr
> > data, const unsigned char *dom_uu
> >  return 0;
> >  }
> >  
> > -/**
> > - * function to get the values for max port per
> > - * instance and max slots per port for the devices
> > - *
> > - * @returns true on Success, false on failure.
> > - * @param   vboxInput IVirtualBox pointer
> > - * @param   maxPortPerInst  Output array of max port per
> > instance
> > - * @param   maxSlotPerPort  Output array of max slot per port
> > - *
> > - */
> > -
> > -static bool vboxGetMaxPortSlotValues(IVirtualBox *vbox,
> > - PRUint32 *maxPortPerInst,
> > - PRUint32 *maxSlotPerPort)
> > -{
> > -ISystemProperties *sysProps = NULL;
> > -
> > -if (!vbox)
> > -return false;
> > -
> > -gVBoxAPI.UIVirtualBox.GetSystemProperties(vbox, &sysProps);
> > -
> > -if (!sysProps)
> > -return false;
> > -
> > -gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysPr
> > ops,
> > - Stora
> > geBus_IDE,
> > - &maxP
> > ortPerInst[StorageBus_IDE]);
> > -gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysPr
> > ops,
> > - Stora
> > geBus_SATA,
> > - &maxP
> > ortPerInst[StorageBus_SATA]);
> > -gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysPr
> > ops,
> > - Stora
> > geBus_SCSI,
> > - &maxP
> > ortPerInst[StorageBus_SCSI]);
> > -gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysPr
> > ops,
> > - Stora
> > geBus_Floppy,
> > - &maxP
> > ortPerInst[StorageBus_Floppy]);
> > -
> > -gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(
> > sysProps,
> > -  
> > StorageBus_IDE,
> > -  
> > &maxSlotPerPort[StorageBus_IDE]);
> > -gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(
> > sysProps,
> > - 

[libvirt] [PATCH v3 02/13] vbox: Process element in domain XML

[Dawid Zamirski]

With this patch, the vbox driver will no longer attach all supported
storage controllers by default even if no disk devices are associated
with them. Instead, it will attach only those that are implicitly added
by virDomainDefAddImplicitController based on  element or if
explicitly specified via the  element.
---
 src/vbox/vbox_common.c | 199 ++---
 src/vbox/vbox_common.h |   7 ++
 2 files changed, 161 insertions(+), 45 deletions(-)

diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index d93b0855f..49df52c12 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -406,6 +406,154 @@ static char *vboxGenerateMediumName(PRUint32 storageBus,
 return name;
 }
 
+
+static int
+vboxSetStorageController(virDomainControllerDefPtr controller,
+ vboxDriverPtr data,
+ IMachine *machine)
+{
+PRUnichar *controllerName = NULL;
+PRInt32 vboxModel = StorageControllerType_Null;
+PRInt32 vboxBusType = StorageBus_Null;
+IStorageController *vboxController = NULL;
+nsresult rc = 0;
+char *debugName = NULL;
+int ret = -1;
+
+/* libvirt controller type => vbox bus type */
+switch ((virDomainControllerType) controller->type) {
+case VIR_DOMAIN_CONTROLLER_TYPE_FDC:
+VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_FLOPPY_NAME, &controllerName);
+vboxBusType = StorageBus_Floppy;
+
+break;
+case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
+VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_IDE_NAME, &controllerName);
+vboxBusType = StorageBus_IDE;
+
+break;
+case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
+VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SCSI_NAME, &controllerName);
+vboxBusType = StorageBus_SCSI;
+
+break;
+case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
+VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SATA_NAME, &controllerName);
+vboxBusType = StorageBus_SATA;
+
+break;
+case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL:
+case VIR_DOMAIN_CONTROLLER_TYPE_CCID:
+case VIR_DOMAIN_CONTROLLER_TYPE_USB:
+case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
+case VIR_DOMAIN_CONTROLLER_TYPE_LAST:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("The vbox driver does not support %s controller 
type"),
+   virDomainControllerTypeToString(controller->type));
+return -1;
+}
+
+/* libvirt scsi model => vbox scsi model */
+if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
+switch ((virDomainControllerModelSCSI) controller->model) {
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC:
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO:
+vboxModel = StorageControllerType_LsiLogic;
+
+break;
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC:
+vboxModel = StorageControllerType_BusLogic;
+
+break;
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VMPVSCSI:
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI:
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI:
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068:
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078:
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("The vbox driver does not support %s SCSI "
+ "controller model"),
+   
virDomainControllerModelSCSITypeToString(controller->model));
+goto cleanup;
+}
+/* libvirt ide model => vbox ide model */
+} else if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) {
+switch ((virDomainControllerModelIDE) controller->model) {
+case VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX3:
+vboxModel = StorageControllerType_PIIX3;
+
+break;
+case VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX4:
+vboxModel = StorageControllerType_PIIX4;
+
+break;
+case VIR_DOMAIN_CONTROLLER_MODEL_IDE_ICH6:
+vboxModel = StorageControllerType_ICH6;
+
+break;
+case VIR_DOMAIN_CONTROLLER_MODEL_IDE_LAST:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("The vbox driver does not support %s IDE "
+ "controller model"),
+ 
virDomainControllerModelIDETypeToString(controller->model));
+goto cleanup;
+}
+}
+
+VBOX_UTF16_TO_UTF8(controllerName, &debugName);
+VIR_DEBUG("Adding VBOX storage controller (name: %s, busType: %d)",
+   debugName, vboxBusType);
+
+rc = gVBoxAPI.UIMachine.AddStorageController(machine, controllerName,
+ vboxBusType, &vboxController);
+
+if (NS_FAILED(rc)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Failed to add storage controller "

[libvirt] [PATCH v3 13/13] docs: Update news.xml with vbox changes.

[Dawid Zamirski]

---
 docs/news.xml | 74 +++
 1 file changed, 74 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml
index ef855d895..454c63d8b 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -37,8 +37,82 @@
 
 
 
+  
+
+  vbox: Add support for configuring storage controllers
+
+
+  The VirtualBox driver now supports the 

+  element in the domain XML for configuring storage controllers in VBOX
+  VMs.  Additionally, libvirt's domain XML schema was updated to allow
+  optional model attribute for  which is used by the VBOX driver to set the
+  IDE controller model to be one of 'piix4', 'piix4' (default), or
+  'ich6'.  Finally, with this change dumpxml generates
+   elements that correspond to current
+  VBOX VM storage controller configuration.
+
+  
+  
+
+  vbox: Add support for attaching empty removable disks
+
+
+  The VirutalBox driver now supports adding CD-ROM and floppy disk
+  devices that do not have the disk source specified. Previously such
+  devices were silently ignored.
+
+  
+  
+
+  vbox: Add support for attaching SAS storage controllers
+
+
+  In VirtualBox, SCSI and SAS are distinct controller types whereas
+  libvirt does not make such distinction. Therefore, the VBOX driver 
was
+  updated to allow attaching SAS controllers via  element. If there are
+  both SCSI and SAS controllers present in the VBOX VM, the domain XML
+  can associate the disk device using the 

+  element with the controller attribute, and optionally,
+  set the port via unit attribute.
+
+  
 
 
+  
+
+  vbox: Do not ignore failures to attach disk devices when defining
+
+
+  The define now fails and reports an error if any of the
+  controller or disk devices specified in the
+  domain XML fail to attach to the VirtualBox VM.
+
+  
+  
+
+  vbox: Fix dumpxml to always output disk devices 
+
+
+  The VirtualBox driver was ignoring any disk devices in
+  dumpxml output if there was a SAS storage controller
+  attached to the VM.
+
+  
+  
+
+  vbox: Fix dumpxml to always generate valid domain XML
+
+
+  When a VirtualBox VM has multiple disks attached, each to a different
+  storage controller that uses 'sd' prefix for block device names e.g.
+  one disk attached to SATA and one to SCSI controller, it no longer
+  generates XML where both would have 'sda' device name assigned.
+  Instead it properly assigns 'sda' and 'sdb' to those disks in the
+  order of appearance.
+
+  
 
   
   
-- 
2.14.3

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

[libvirt] [PATCH v3 04/13] vbox: Rename vboxDumpIDEHDDs to vboxDumpDisks

[Dawid Zamirski]

Because it deals with other disk types as well not just IDE. Also this
function now returns -1 on error
---
 src/vbox/vbox_common.c | 57 ++
 1 file changed, 25 insertions(+), 32 deletions(-)

diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 4d596075c..2e0d7ee9a 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -3254,13 +3254,11 @@ vboxDumpStorageControllers(virDomainDefPtr def, 
IMachine *machine)
 }
 
 
-static void
-vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
+static int
+vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
 {
-/* dump IDE hdds if present */
 vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER;
-bool error = false;
-int diskCount = 0;
+int ret = -1, diskCount = 0;
 size_t i;
 PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {};
 PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {};
@@ -3284,24 +3282,22 @@ vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr 
data, IMachine *machine)
 }
 
 /* Allocate mem, if fails return error */
-if (VIR_ALLOC_N(def->disks, def->ndisks) >= 0) {
-for (i = 0; i < def->ndisks; i++) {
-virDomainDiskDefPtr disk = virDomainDiskDefNew(NULL);
-if (!disk) {
-error = true;
-break;
-}
-def->disks[i] = disk;
-}
-} else {
-error = true;
+if (VIR_ALLOC_N(def->disks, def->ndisks) < 0)
+goto cleanup;
+
+for (i = 0; i < def->ndisks; i++) {
+virDomainDiskDefPtr disk = virDomainDiskDefNew(NULL);
+if (!disk)
+goto cleanup;
+
+def->disks[i] = disk;
 }
 
-if (!error)
-error = !vboxGetMaxPortSlotValues(data->vboxObj, maxPortPerInst, 
maxSlotPerPort);
+if (!vboxGetMaxPortSlotValues(data->vboxObj, maxPortPerInst, 
maxSlotPerPort))
+goto cleanup;
 
 /* get the attachment details here */
-for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks && 
!error; i++) {
+for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks; i++) {
 IMediumAttachment *imediumattach = mediumAttachments.items[i];
 IStorageController *storageController = NULL;
 PRUnichar *storageControllerName = NULL;
@@ -3347,8 +3343,8 @@ vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, 
IMachine *machine)
 if (!virDomainDiskGetSource(def->disks[diskCount])) {
 VBOX_RELEASE(medium);
 VBOX_RELEASE(storageController);
-error = true;
-break;
+
+goto cleanup;
 }
 
 gVBoxAPI.UIStorageController.GetBus(storageController, &storageBus);
@@ -3385,8 +3381,8 @@ vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, 
IMachine *machine)
deviceInst, devicePort, deviceSlot);
 VBOX_RELEASE(medium);
 VBOX_RELEASE(storageController);
-error = true;
-break;
+
+goto cleanup;
 }
 
 gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly);
@@ -3401,15 +3397,12 @@ vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr 
data, IMachine *machine)
 diskCount++;
 }
 
+ret = 0;
+
+ cleanup:
 gVBoxAPI.UArray.vboxArrayRelease(&mediumAttachments);
 
-/* cleanup on error */
-if (error) {
-for (i = 0; i < def->ndisks; i++)
-VIR_FREE(def->disks[i]);
-VIR_FREE(def->disks);
-def->ndisks = 0;
-}
+return ret;
 }
 
 static int
@@ -4103,8 +4096,8 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, 
unsigned int flags)
 goto cleanup;
 if (vboxDumpStorageControllers(def, machine) < 0)
 goto cleanup;
-
-vboxDumpIDEHDDs(def, data, machine);
+if (vboxDumpDisks(def, data, machine) < 0)
+goto cleanup;
 
 vboxDumpSharedFolders(def, data, machine);
 vboxDumpNetwork(def, data, machine, networkAdapterCount);
-- 
2.14.3

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

[libvirt] [PATCH v3 08/13] vbox: Correctly generate drive name in dumpxml

[Dawid Zamirski]

If a VBOX VM has e.g. a SATA and SCSI disk attached, the XML generated
by dumpxml used to produce "sda" for both of those disks. This is an
invalid domain XML as libvirt does not allow duplicate device names. To
address this, keep the running total of disks that will use "sd" prefix
for device name and pass it to the vboxGenerateMediumName which no
longer tries to "compute" the value based only on current and max
port and slot values. After this the vboxGetMaxPortSlotValues is not
needed and was deleted.
---
 src/vbox/vbox_common.c | 192 ++---
 1 file changed, 52 insertions(+), 140 deletions(-)

diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 4d39beb1e..57b0fd515 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -290,61 +290,6 @@ static int openSessionForMachine(vboxDriverPtr data, const 
unsigned char *dom_uu
 return 0;
 }
 
-/**
- * function to get the values for max port per
- * instance and max slots per port for the devices
- *
- * @returns true on Success, false on failure.
- * @param   vboxInput IVirtualBox pointer
- * @param   maxPortPerInst  Output array of max port per instance
- * @param   maxSlotPerPort  Output array of max slot per port
- *
- */
-
-static bool vboxGetMaxPortSlotValues(IVirtualBox *vbox,
- PRUint32 *maxPortPerInst,
- PRUint32 *maxSlotPerPort)
-{
-ISystemProperties *sysProps = NULL;
-
-if (!vbox)
-return false;
-
-gVBoxAPI.UIVirtualBox.GetSystemProperties(vbox, &sysProps);
-
-if (!sysProps)
-return false;
-
-gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
- StorageBus_IDE,
- 
&maxPortPerInst[StorageBus_IDE]);
-gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
- StorageBus_SATA,
- 
&maxPortPerInst[StorageBus_SATA]);
-gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
- StorageBus_SCSI,
- 
&maxPortPerInst[StorageBus_SCSI]);
-gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
- StorageBus_Floppy,
- 
&maxPortPerInst[StorageBus_Floppy]);
-
-gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
-  
StorageBus_IDE,
-  
&maxSlotPerPort[StorageBus_IDE]);
-gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
-  
StorageBus_SATA,
-  
&maxSlotPerPort[StorageBus_SATA]);
-gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
-  
StorageBus_SCSI,
-  
&maxSlotPerPort[StorageBus_SCSI]);
-gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
-  
StorageBus_Floppy,
-  
&maxSlotPerPort[StorageBus_Floppy]);
-
-VBOX_RELEASE(sysProps);
-
-return true;
-}
 
 /**
  * function to generate the name for medium,
@@ -352,57 +297,39 @@ static bool vboxGetMaxPortSlotValues(IVirtualBox *vbox,
  *
  * @returns null terminated string with device name or NULL
  *  for failures
- * @param   connInput Connection Pointer
  * @param   storageBus  Input storage bus type
- * @param   deviceInst  Input device instance number
  * @param   devicePort  Input port number
  * @param   deviceSlot  Input slot number
- * @param   aMaxPortPerInst Input array of max port per device instance
- * @param   aMaxSlotPerPort Input array of max slot per device port
- *
+ * @param   sdCount Running total of disk devices with "sd" prefix
  */
-static char *vboxGenerateMediumName(PRUint32 storageBus,
-PRInt32 deviceInst,
-PRInt32 devicePort,
-PRInt32 deviceSlot,
-PRUint32 *aMaxPortPerInst,
-PRUint32 *aMaxSlotPerPort)
+static char *
+vboxGenerateMediumName(PRUint32 storageBus,
+   PRInt32 devicePort,

[libvirt] [PATCH v3 05/13] vbox: Cleanup/prepare snasphot dumpxml functions

[Dawid Zamirski]

This patch prepares the vboxSnapshotGetReadOnlyDisks and
vboxSnapshotGetReadWriteDisks functions for further changes so that
the code movement does not obstruct the gist of those future changes.
This is done primarily because we'll need to know the type of vbox
storage controller as early as possible and make decisions based on
that info.
---
 src/vbox/vbox_common.c | 185 -
 1 file changed, 105 insertions(+), 80 deletions(-)

diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 2e0d7ee9a..d26ce1bce 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -5653,8 +5653,9 @@ vboxDomainSnapshotGet(vboxDriverPtr data,
 return snapshot;
 }
 
-static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def,
- virDomainSnapshotPtr snapshot)
+static int
+vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def,
+  virDomainSnapshotPtr snapshot)
 {
 virDomainPtr dom = snapshot->domain;
 vboxDriverPtr data = dom->conn->privateData;
@@ -5755,26 +5756,72 @@ static int 
vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def,
 void *handle;
 size_t j = 0;
 size_t k = 0;
+
 if (!imediumattach)
 continue;
-rc = gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &disk);
+
+rc = gVBoxAPI.UIMediumAttachment.GetController(imediumattach,
+   &storageControllerName);
 if (NS_FAILED(rc)) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("cannot get medium"));
+   _("Cannot get storage controller name"));
 goto cleanup;
 }
-if (!disk)
-continue;
-rc = gVBoxAPI.UIMediumAttachment.GetController(imediumattach, 
&storageControllerName);
+
+rc = gVBoxAPI.UIMachine.GetStorageControllerByName(machine,
+   
storageControllerName,
+   &storageController);
+VBOX_UTF16_FREE(storageControllerName);
 if (NS_FAILED(rc)) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("cannot get controller"));
+   _("Cannot get storage controller by name"));
 goto cleanup;
 }
-if (!storageControllerName) {
-VBOX_RELEASE(disk);
+
+rc = gVBoxAPI.UIStorageController.GetBus(storageController, 
&storageBus);
+if (NS_FAILED(rc)) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Cannot get storage controller bus"));
+VBOX_RELEASE(storageController);
+goto cleanup;
+}
+
+rc = gVBoxAPI.UIMediumAttachment.GetType(imediumattach, &deviceType);
+if (NS_FAILED(rc)) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Cannot get medium attachment type"));
+VBOX_RELEASE(storageController);
+goto cleanup;
+}
+rc = gVBoxAPI.UIMediumAttachment.GetPort(imediumattach, &devicePort);
+if (NS_FAILED(rc)) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Cannot get medium attachment port"));
+VBOX_RELEASE(storageController);
+goto cleanup;
+}
+rc = gVBoxAPI.UIMediumAttachment.GetDevice(imediumattach, &deviceSlot);
+if (NS_FAILED(rc)) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Cannot get medium attachment slot"));
+VBOX_RELEASE(storageController);
+goto cleanup;
+}
+
+rc = gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &disk);
+if (NS_FAILED(rc)) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Cannot get medium"));
+VBOX_RELEASE(storageController);
+goto cleanup;
+}
+
+/* skip empty removable disk */
+if (!disk) {
+VBOX_RELEASE(storageController);
 continue;
 }
+
 handle = gVBoxAPI.UArray.handleMediumGetChildren(disk);
 rc = gVBoxAPI.UArray.vboxArrayGet(&children, disk, handle);
 if (NS_FAILED(rc)) {
@@ -5797,53 +5844,25 @@ static int 
vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def,
 char *diskSnapIdStr = NULL;
 VBOX_UTF16_TO_UTF8(diskSnapId, &diskSnapIdStr);
 if (STREQ(diskSnapIdStr, snapshotUuidStr)) {
-rc = gVBoxAPI.UIMachine.GetStorageControllerByName(machine,
-   
storageControllerName,
-   
&storageController);
-

[libvirt] [PATCH v3 11/13] vbox: Generate disk address element in dumpxml

[Dawid Zamirski]

This patch adds  element to each  device since device
names alone won't adequately reflect the storage device layout in the
VM. With this patch, the ouput produced by dumpxml will faithfully
reproduce the storage layout of the VM if used with define.
---
 src/vbox/vbox_common.c | 51 ++
 1 file changed, 43 insertions(+), 8 deletions(-)

diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 661e09a27..8da08240e 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -3336,25 +3336,60 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, 
IMachine *machine)
 goto cleanup;
 }
 
-if (storageBus == StorageBus_IDE) {
+disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE;
+disk->info.addr.drive.bus = 0;
+disk->info.addr.drive.unit = devicePort;
+
+switch ((enum StorageBus) storageBus) {
+case StorageBus_IDE:
 disk->bus = VIR_DOMAIN_DISK_BUS_IDE;
-} else if (storageBus == StorageBus_SATA) {
-sdCount++;
+disk->info.addr.drive.bus = devicePort; /* primary, secondary */
+disk->info.addr.drive.unit = deviceSlot; /* master, slave */
+
+break;
+case StorageBus_SATA:
 disk->bus = VIR_DOMAIN_DISK_BUS_SATA;
-} else if (storageBus == StorageBus_SCSI) {
 sdCount++;
+
+break;
+case StorageBus_SCSI:
 disk->bus = VIR_DOMAIN_DISK_BUS_SCSI;
-} else if (storageBus == StorageBus_Floppy) {
+sdCount++;
+
+break;
+case StorageBus_Floppy:
 disk->bus = VIR_DOMAIN_DISK_BUS_FDC;
+
+break;
+case StorageBus_SAS:
+case StorageBus_Null:
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Unsupported null storage bus"));
+goto cleanup;
 }
 
-if (deviceType == DeviceType_HardDisk)
+switch ((enum DeviceType) deviceType) {
+case DeviceType_HardDisk:
 disk->device = VIR_DOMAIN_DISK_DEVICE_DISK;
-else if (deviceType == DeviceType_Floppy)
+
+break;
+case DeviceType_Floppy:
 disk->device = VIR_DOMAIN_DISK_DEVICE_FLOPPY;
-else if (deviceType == DeviceType_DVD)
+
+break;
+case DeviceType_DVD:
 disk->device = VIR_DOMAIN_DISK_DEVICE_CDROM;
 
+break;
+case DeviceType_Network:
+case DeviceType_USB:
+case DeviceType_SharedFolder:
+case DeviceType_Null:
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Unsupported vbox device type: %d"), deviceType);
+goto cleanup;
+}
+
 if (readOnly == PR_TRUE)
 disk->src->readonly = true;
 
-- 
2.14.3

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

[libvirt] [PATCH v3 03/13] vbox: Add vboxDumpStorageControllers

[Dawid Zamirski]

---
 src/vbox/vbox_common.c | 109 +
 1 file changed, 109 insertions(+)

diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 49df52c12..4d596075c 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -3147,6 +3147,113 @@ vboxHostDeviceGetXMLDesc(vboxDriverPtr data, 
virDomainDefPtr def, IMachine *mach
 goto release_filters;
 }
 
+
+static int
+vboxDumpStorageControllers(virDomainDefPtr def, IMachine *machine)
+{
+vboxArray storageControllers = VBOX_ARRAY_INITIALIZER;
+IStorageController *controller = NULL;
+PRUint32 storageBus = StorageBus_Null;
+PRUint32 controllerType = StorageControllerType_Null;
+virDomainControllerDefPtr cont = NULL;
+size_t i = 0;
+int model = -1, ret = -1;
+virDomainControllerType type = VIR_DOMAIN_CONTROLLER_TYPE_LAST;
+
+gVBoxAPI.UArray.vboxArrayGet(&storageControllers, machine,
+ gVBoxAPI.UArray.handleMachineGetStorageControllers(machine));
+
+for (i = 0; i < storageControllers.count; i++) {
+controller = storageControllers.items[i];
+storageBus = StorageBus_Null;
+controllerType = StorageControllerType_Null;
+type = VIR_DOMAIN_CONTROLLER_TYPE_LAST;
+model = -1;
+
+if (!controller)
+continue;
+
+gVBoxAPI.UIStorageController.GetBus(controller, &storageBus);
+gVBoxAPI.UIStorageController.GetControllerType(controller,
+   &controllerType);
+
+/* vbox controller model => libvirt controller model */
+switch ((enum StorageControllerType) controllerType) {
+case StorageControllerType_PIIX3:
+model = VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX3;
+
+break;
+case StorageControllerType_PIIX4:
+model = VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX4;
+
+break;
+case StorageControllerType_ICH6:
+model = VIR_DOMAIN_CONTROLLER_MODEL_IDE_ICH6;
+
+break;
+case StorageControllerType_BusLogic:
+model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC;
+
+break;
+case StorageControllerType_LsiLogic:
+model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC;
+
+break;
+case StorageControllerType_LsiLogicSas:
+case StorageControllerType_IntelAhci:
+case StorageControllerType_I82078:
+case StorageControllerType_Null:
+model = -1;
+
+break;
+}
+
+/* vbox controller bus => libvirt controller type */
+switch ((enum StorageBus) storageBus) {
+case StorageBus_IDE:
+type = VIR_DOMAIN_CONTROLLER_TYPE_IDE;
+
+break;
+case StorageBus_SCSI:
+case StorageBus_SAS:
+type = VIR_DOMAIN_CONTROLLER_TYPE_SCSI;
+
+break;
+case StorageBus_SATA:
+type = VIR_DOMAIN_CONTROLLER_TYPE_SATA;
+
+break;
+case StorageBus_Floppy:
+type = VIR_DOMAIN_CONTROLLER_TYPE_FDC;
+
+break;
+case StorageBus_Null:
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Unsupported null storage bus"));
+
+goto cleanup;
+}
+
+if (type != VIR_DOMAIN_CONTROLLER_TYPE_LAST) {
+cont = virDomainDefAddController(def, type, -1, model);
+if (!cont) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Failed to add %s controller type 
definition"),
+   virDomainControllerTypeToString(type));
+goto cleanup;
+}
+}
+}
+
+ret = 0;
+
+ cleanup:
+gVBoxAPI.UArray.vboxArrayRelease(&storageControllers);
+
+return ret;
+}
+
+
 static void
 vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
 {
@@ -3994,6 +4101,8 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, 
unsigned int flags)
 goto cleanup;
 if (vboxDumpDisplay(def, data, machine) < 0)
 goto cleanup;
+if (vboxDumpStorageControllers(def, machine) < 0)
+goto cleanup;
 
 vboxDumpIDEHDDs(def, data, machine);
 
-- 
2.14.3

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

[libvirt] [PATCH v3 06/13] vbox: Do not free disk definitions on cleanup

[Dawid Zamirski]

Both vboxSnapshotGetReadWriteDisks and vboxSnapshotGetReadWriteDisks do
not need to free the def->disks on cleanup because it's being done by
the caller via virDomainSnaphotDefFree
---
 src/vbox/vbox_common.c | 14 ++
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index d26ce1bce..79030e36e 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -5883,13 +5883,8 @@ vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr 
def,
 ret = 0;
 
  cleanup:
-if (ret < 0) {
-for (i = 0; i < def->ndisks; i++)
-VIR_FREE(def->disks[i].src);
-VIR_FREE(def->disks);
-def->ndisks = 0;
-}
 VBOX_RELEASE(snap);
+
 return ret;
 }
 
@@ -6105,16 +6100,11 @@ vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr 
snapshot,
 ret = 0;
 
  cleanup:
-if (ret < 0) {
-for (i = 0; i < def->dom->ndisks; i++)
-virDomainDiskDefFree(def->dom->disks[i]);
-VIR_FREE(def->dom->disks);
-def->dom->ndisks = 0;
-}
 VBOX_RELEASE(disk);
 VBOX_RELEASE(storageController);
 gVBoxAPI.UArray.vboxArrayRelease(&mediumAttachments);
 VBOX_RELEASE(snap);
+
 return ret;
 }
 
-- 
2.14.3

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

[libvirt] [PATCH v3 09/13] vbox: Cleanup vboxDumpDisks implementation

[Dawid Zamirski]

Primer the code for further changes:

* move variable declarations to the top of the function
* group together free/release statements
* error check and report VBOX API calls used
---
 src/vbox/vbox_common.c | 190 +++--
 1 file changed, 122 insertions(+), 68 deletions(-)

diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 57b0fd515..248f315eb 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -3186,6 +3186,16 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, 
IMachine *machine)
 {
 vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER;
 int ret = -1, diskCount = 0;
+IMediumAttachment *mediumAttachment = NULL;
+IMedium *medium = NULL;
+IStorageController *controller = NULL;
+PRUnichar *controllerName = NULL, *mediumLocUtf16 = NULL;
+PRUint32 deviceType, storageBus;
+PRInt32 devicePort, deviceSlot;
+PRBool readOnly;
+nsresult rc;
+virDomainDiskDefPtr disk = NULL;
+char *mediumLocUtf8 = NULL;
 size_t sdCount = 0, i;
 
 def->ndisks = 0;
@@ -3194,15 +3204,14 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, 
IMachine *machine)
 
 /* get the number of attachments */
 for (i = 0; i < mediumAttachments.count; i++) {
-IMediumAttachment *imediumattach = mediumAttachments.items[i];
-if (imediumattach) {
-IMedium *medium = NULL;
+mediumAttachment = mediumAttachments.items[i];
+if (!mediumAttachment)
+continue;
 
-gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &medium);
-if (medium) {
-def->ndisks++;
-VBOX_RELEASE(medium);
-}
+gVBoxAPI.UIMediumAttachment.GetMedium(mediumAttachment, &medium);
+if (medium) {
+def->ndisks++;
+VBOX_RELEASE(medium);
 }
 }
 
@@ -3211,7 +3220,7 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, 
IMachine *machine)
 goto cleanup;
 
 for (i = 0; i < def->ndisks; i++) {
-virDomainDiskDefPtr disk = virDomainDiskDefNew(NULL);
+disk = virDomainDiskDefNew(NULL);
 if (!disk)
 goto cleanup;
 
@@ -3220,103 +3229,140 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr 
data, IMachine *machine)
 
 /* get the attachment details here */
 for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks; i++) {
-IMediumAttachment *imediumattach = mediumAttachments.items[i];
-IStorageController *storageController = NULL;
-PRUnichar *storageControllerName = NULL;
-PRUint32 deviceType = DeviceType_Null;
-PRUint32 storageBus = StorageBus_Null;
-PRBool readOnly = PR_FALSE;
-IMedium *medium = NULL;
-PRUnichar *mediumLocUtf16 = NULL;
-char *mediumLocUtf8 = NULL;
-PRInt32 devicePort = 0;
-PRInt32 deviceSlot = 0;
-
-if (!imediumattach)
+mediumAttachment = mediumAttachments.items[i];
+controller = NULL;
+controllerName = NULL;
+deviceType = DeviceType_Null;
+storageBus = StorageBus_Null;
+readOnly = PR_FALSE;
+medium = NULL;
+mediumLocUtf16 = NULL;
+mediumLocUtf8 = NULL;
+devicePort = 0;
+deviceSlot = 0;
+disk = def->disks[diskCount];
+
+if (!mediumAttachment)
 continue;
 
-gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &medium);
+rc = gVBoxAPI.UIMediumAttachment.GetMedium(mediumAttachment, &medium);
+if (NS_FAILED(rc)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Could not get IMedium, rc=%08x"), rc);
+goto cleanup;
+}
+
 if (!medium)
 continue;
 
-gVBoxAPI.UIMediumAttachment.GetController(imediumattach, 
&storageControllerName);
-if (!storageControllerName) {
-VBOX_RELEASE(medium);
-continue;
+rc = gVBoxAPI.UIMediumAttachment.GetController(mediumAttachment,
+   &controllerName);
+if (NS_FAILED(rc)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Failed to get storage controller name, rc=%08x"),
+   rc);
+goto cleanup;
 }
 
-gVBoxAPI.UIMachine.GetStorageControllerByName(machine,
-  storageControllerName,
-  &storageController);
-VBOX_UTF16_FREE(storageControllerName);
-if (!storageController) {
-VBOX_RELEASE(medium);
-continue;
+rc = gVBoxAPI.UIMachine.GetStorageControllerByName(machine,
+   controllerName,
+   &controller);
+if (NS_FAILED(rc)) {

[libvirt] [PATCH v3 10/13] vbox: Process empty removable disks in dumpxml

[Dawid Zamirski]

Previously any removable storage device without media attached was
omitted from domain XML dump. They're still (rightfully) omitted in
snapshot XMl dump but need to be accounted properly to for the device
names to stay in 'sync' between domain and snapshot XML dumps.
---
 src/vbox/vbox_common.c | 82 ++
 1 file changed, 49 insertions(+), 33 deletions(-)

diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 248f315eb..661e09a27 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -3185,7 +3185,7 @@ static int
 vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
 {
 vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER;
-int ret = -1, diskCount = 0;
+int ret = -1;
 IMediumAttachment *mediumAttachment = NULL;
 IMedium *medium = NULL;
 IStorageController *controller = NULL;
@@ -3208,11 +3208,15 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, 
IMachine *machine)
 if (!mediumAttachment)
 continue;
 
-gVBoxAPI.UIMediumAttachment.GetMedium(mediumAttachment, &medium);
-if (medium) {
-def->ndisks++;
-VBOX_RELEASE(medium);
+rc = gVBoxAPI.UIMediumAttachment.GetMedium(mediumAttachment, &medium);
+if (NS_FAILED(rc)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Could not get IMedium, rc=%08x"), rc);
+goto cleanup;
 }
+
+def->ndisks++;
+VBOX_RELEASE(medium);
 }
 
 /* Allocate mem, if fails return error */
@@ -3228,7 +3232,7 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, 
IMachine *machine)
 }
 
 /* get the attachment details here */
-for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks; i++) {
+for (i = 0; i < mediumAttachments.count; i++) {
 mediumAttachment = mediumAttachments.items[i];
 controller = NULL;
 controllerName = NULL;
@@ -3240,7 +3244,7 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, 
IMachine *machine)
 mediumLocUtf8 = NULL;
 devicePort = 0;
 deviceSlot = 0;
-disk = def->disks[diskCount];
+disk = def->disks[i];
 
 if (!mediumAttachment)
 continue;
@@ -3252,9 +3256,6 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, 
IMachine *machine)
 goto cleanup;
 }
 
-if (!medium)
-continue;
-
 rc = gVBoxAPI.UIMediumAttachment.GetController(mediumAttachment,
&controllerName);
 if (NS_FAILED(rc)) {
@@ -3274,22 +3275,6 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, 
IMachine *machine)
 goto cleanup;
 }
 
-rc = gVBoxAPI.UIMedium.GetLocation(medium, &mediumLocUtf16);
-if (NS_FAILED(rc)) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("Could not get medium storage location, rc=%08x"),
-   rc);
-goto cleanup;
-}
-
-VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8);
-
-if (virDomainDiskSetSource(disk, mediumLocUtf8) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Could not set disk source"));
-goto cleanup;
-}
-
 rc = gVBoxAPI.UIMediumAttachment.GetType(mediumAttachment, 
&deviceType);
 if (NS_FAILED(rc)) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -3315,11 +3300,30 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, 
IMachine *machine)
rc);
 goto cleanup;
 }
-rc = gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly);
-if (NS_FAILED(rc)) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("Could not get read only state, rc=%08x"), rc);
-goto cleanup;
+
+if (medium) {
+rc = gVBoxAPI.UIMedium.GetLocation(medium, &mediumLocUtf16);
+if (NS_FAILED(rc)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Could not get medium storage location, 
rc=%08x"),
+   rc);
+goto cleanup;
+}
+
+VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8);
+
+if (virDomainDiskSetSource(disk, mediumLocUtf8) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Could not set disk source"));
+goto cleanup;
+}
+
+rc = gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly);
+if (NS_FAILED(rc)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Could not get read only state, rc=%08x"), 
rc);
+goto cleanup;
+}
 }
 
 disk->dst = vboxGenerateMediumName(st

[libvirt] [PATCH v3 12/13] vbox: Add SAS controller support

[Dawid Zamirski]

In VirtualBox SAS and SCSI are separate controller types whereas libvirt
does not make such distinction. This patch adds support for attaching
the VBOX SAS controllers by mapping the 'lsisas1068' controller model in
libvirt XML to VBOX SAS controller type. If VBOX VM has disks attached
to both SCSI and SAS controller libvirt domain XML will have two
 elements with index and model attributes set
accordingly. In this case, each respective  element must have
 element specified to assign it to respective SCSI controller.
---
 src/vbox/vbox_common.c | 86 --
 src/vbox/vbox_common.h |  1 +
 2 files changed, 70 insertions(+), 17 deletions(-)

diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 8da08240e..a1d9b2e7d 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -312,20 +312,27 @@ vboxGenerateMediumName(PRUint32 storageBus,
 char *name = NULL;
 int total = 0;
 
-if ((storageBus < StorageBus_IDE) ||
-(storageBus > StorageBus_Floppy))
-return NULL;
-
-if (storageBus == StorageBus_IDE) {
+switch ((enum StorageBus) storageBus) {
+case StorageBus_IDE:
 prefix = "hd";
 total = devicePort * 2 + deviceSlot;
-} else if ((storageBus == StorageBus_SATA) ||
-   (storageBus == StorageBus_SCSI)) {
+
+break;
+case StorageBus_SATA:
+case StorageBus_SCSI:
+case StorageBus_SAS:
 prefix = "sd";
 total = sdCount;
-} else if (storageBus == StorageBus_Floppy) {
+
+break;
+case StorageBus_Floppy:
 total = deviceSlot;
 prefix = "fd";
+
+break;
+case StorageBus_Null:
+
+return NULL;
 }
 
 name = virIndexToDiskName(total, prefix);
@@ -391,11 +398,17 @@ vboxSetStorageController(virDomainControllerDefPtr 
controller,
 case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC:
 vboxModel = StorageControllerType_BusLogic;
 
+break;
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068:
+/* in vbox, lsisas has a dedicated SAS bus type with no model */
+VBOX_UTF16_FREE(controllerName);
+VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SAS_NAME, &controllerName);
+vboxBusType = StorageBus_SAS;
+
 break;
 case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VMPVSCSI:
 case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI:
 case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI:
-case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068:
 case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078:
 case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST:
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -1034,7 +1047,7 @@ static int
 vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
 {
 size_t i;
-int type, ret = 0;
+int type, ret = 0, model = -1;
 const char *src = NULL;
 nsresult rc = 0;
 virDomainDiskDefPtr disk = NULL;
@@ -1113,6 +1126,13 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr 
data, IMachine *machine)
 case VIR_DOMAIN_DISK_BUS_SCSI:
 VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SCSI_NAME, &storageCtlName);
 
+model = virDomainDeviceFindControllerModel(def, &disk->info,
+   
VIR_DOMAIN_CONTROLLER_TYPE_SCSI);
+if (model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068) {
+VBOX_UTF16_FREE(storageCtlName);
+VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SAS_NAME, &storageCtlName);
+}
+
 break;
 case VIR_DOMAIN_DISK_BUS_FDC:
 VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_FLOPPY_NAME, &storageCtlName);
@@ -3127,6 +3147,9 @@ vboxDumpStorageControllers(virDomainDefPtr def, IMachine 
*machine)
 
 break;
 case StorageControllerType_LsiLogicSas:
+model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068;
+
+break;
 case StorageControllerType_IntelAhci:
 case StorageControllerType_I82078:
 case StorageControllerType_Null:
@@ -3195,12 +3218,13 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, 
IMachine *machine)
 PRBool readOnly;
 nsresult rc;
 virDomainDiskDefPtr disk = NULL;
+virDomainControllerDefPtr ctrl = NULL;
 char *mediumLocUtf8 = NULL;
-size_t sdCount = 0, i;
+size_t sdCount = 0, i, j;
 
 def->ndisks = 0;
 gVBoxAPI.UArray.vboxArrayGet(&mediumAttachments, machine,
- 
gVBoxAPI.UArray.handleMachineGetMediumAttachments(machine));
+ gVBoxAPI.UArray.handleMachineGetMediumAttachments(machine));
 
 /* get the number of attachments */
 for (i = 0; i < mediumAttachments.count; i++) {
@@ -3353,15 +3377,38 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, 
IMachine *machine)
 
 break;
 case StorageBus_SCSI:
+case StorageBus_SAS:
 disk->bus 

[libvirt] [PATCH v3 07/13] vbox: Swap vboxSnapshotGetReadOnlyDisks arguments

[Dawid Zamirski]

So that the function signature matches vboxSnapshotGetReadWriteDisks
---
 src/vbox/vbox_common.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 79030e36e..4d39beb1e 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -5889,8 +5889,8 @@ vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def,
 }
 
 static int
-vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot,
- virDomainSnapshotDefPtr def)
+vboxSnapshotGetReadOnlyDisks(virDomainSnapshotDefPtr def,
+ virDomainSnapshotPtr snapshot)
 {
 virDomainPtr dom = snapshot->domain;
 vboxDriverPtr data = dom->conn->privateData;
@@ -6173,7 +6173,7 @@ static char 
*vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot,
 if (vboxSnapshotGetReadWriteDisks(def, snapshot) < 0)
 VIR_DEBUG("Could not get read write disks for snapshot");
 
-if (vboxSnapshotGetReadOnlyDisks(snapshot, def) < 0)
+if (vboxSnapshotGetReadOnlyDisks(def, snapshot) < 0)
 VIR_DEBUG("Could not get Readonly disks for snapshot");
 }
 
-- 
2.14.3

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

[libvirt] [PATCH v3 00/13] vbox: Improve handling of storage devices

[Dawid Zamirski]

v2:
https://www.redhat.com/archives/libvir-list/2017-October/msg01108.html

Changes since v2:
 * Rebase on master
 * Do not segfault in 'Cleanup partially-defined VM on failure' when
   code jumps to cleanup and machine is NULL
 * Take out SAS controller handling code into a separate patch
 * Split up the snapshot function cleanup code into separate patches
 * Fixed code formatting as pointed out in review
 * Rename vboxDumpIDEHDDs -> vboxDumpDisks into a separate patch
 * Add docs/news.xml patch to describe improvements/fixes in the series

Dawid Zamirski (13):
  vbox: Cleanup partially-defined VM on failure
  vbox: Process  element in domain XML
  vbox: Add vboxDumpStorageControllers
  vbox: Rename vboxDumpIDEHDDs to vboxDumpDisks
  vbox: Cleanup/prepare snasphot dumpxml functions
  vbox: Do not free disk definitions on cleanup
  vbox: Swap vboxSnapshotGetReadOnlyDisks arguments
  vbox: Correctly generate drive name in dumpxml
  vbox: Cleanup vboxDumpDisks implementation
  vbox: Process empty removable disks in dumpxml
  vbox: Generate disk address element in dumpxml
  vbox: Add SAS controller support
  docs: Update news.xml with vbox changes.

 docs/news.xml  |   74 
 src/vbox/vbox_common.c | 1105 +++-
 src/vbox/vbox_common.h |8 +
 3 files changed, 792 insertions(+), 395 deletions(-)

-- 
2.14.3

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

[libvirt] [PATCH v3 01/13] vbox: Cleanup partially-defined VM on failure

[Dawid Zamirski]

Since the VBOX API requires to register an initial VM before proceeding
to attach any remaining devices to it, any failure to attach such
devices should result in automatic cleanup of the initially registered
VM so that the state of VBOX registry remains clean without any leftover
"aborted" VMs in it. Failure to cleanup of such partial VMs results in a
warning log so that actual define error stays on the top of the error
stack.
---
 src/vbox/vbox_common.c | 52 ++
 1 file changed, 36 insertions(+), 16 deletions(-)

diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index aa2563cf1..d93b0855f 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -1821,6 +1821,8 @@ vboxDomainDefineXMLFlags(virConnectPtr conn, const char 
*xml, unsigned int flags
 char uuidstr[VIR_UUID_STRING_BUFLEN];
 virDomainPtr ret = NULL;
 unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE;
+bool machineReady = false;
+
 
 virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL);
 
@@ -1830,12 +1832,11 @@ vboxDomainDefineXMLFlags(virConnectPtr conn, const char 
*xml, unsigned int flags
 if (!data->vboxObj)
 return ret;
 
-VBOX_IID_INITIALIZE(&mchiid);
 if (!(def = virDomainDefParseString(xml, data->caps, data->xmlopt,
-NULL, parse_flags))) {
-goto cleanup;
-}
+NULL, parse_flags)))
+return ret;
 
+VBOX_IID_INITIALIZE(&mchiid);
 virUUIDFormat(def->uuid, uuidstr);
 
 rc = gVBoxAPI.UIVirtualBox.CreateMachine(data, def, &machine, uuidstr);
@@ -1928,30 +1929,49 @@ vboxDomainDefineXMLFlags(virConnectPtr conn, const char 
*xml, unsigned int flags
 vboxAttachUSB(def, data, machine);
 vboxAttachSharedFolder(def, data, machine);
 
-/* Save the machine settings made till now and close the
- * session. also free up the mchiid variable used.
+machineReady = true;
+
+ cleanup:
+/* if machine wasn't even created, cleanup is trivial */
+if (!machine) {
+vboxIIDUnalloc(&mchiid);
+virDomainDefFree(def);
+
+return ret;
+}
+
+/* Save the machine settings made till now, even when jumped here on error,
+ * as otherwise unregister won't cleanup properly. For example, it won't
+ * close media that were partially attached. The VBOX SDK docs say that
+ * unregister implicitly calls saveSettings but evidently it's not so...
  */
 rc = gVBoxAPI.UIMachine.SaveSettings(machine);
 if (NS_FAILED(rc)) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("failed no saving settings, rc=%08x"), (unsigned)rc);
-goto cleanup;
+   _("Failed to save VM settings, rc=%08x"), rc);
+machineReady = false;
 }
 
 gVBoxAPI.UISession.Close(data->vboxSession);
-vboxIIDUnalloc(&mchiid);
-
-ret = virGetDomain(conn, def->name, def->uuid, -1);
-VBOX_RELEASE(machine);
 
-virDomainDefFree(def);
+if (machineReady) {
+ret = virGetDomain(conn, def->name, def->uuid, -1);
+} else {
+/* Unregister incompletely configured VM to not leave garbage behind */
+rc = gVBoxAPI.unregisterMachine(data, &mchiid, &machine);
 
-return ret;
+if (NS_SUCCEEDED(rc))
+gVBoxAPI.deleteConfig(machine);
+else
+VIR_WARN("Could not cleanup partially created VM after failure, "
+ "rc=%08x", rc);
+}
 
- cleanup:
 VBOX_RELEASE(machine);
+vboxIIDUnalloc(&mchiid);
 virDomainDefFree(def);
-return NULL;
+
+return ret;
 }
 
 static virDomainPtr
-- 
2.14.3

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

Re: [libvirt] [PATCH v3 4/5] qemu: Use predictable file names for memory-backend-file

[Martin Kletzander]

On Tue, Nov 07, 2017 at 03:57:45PM +, Daniel P. Berrange wrote:

On Tue, Nov 07, 2017 at 04:51:03PM +0100, Michal Privoznik wrote:

In some cases management application needs to allocate memory for
qemu upfront and then just let qemu use that. Since we don't want
to expose path for memory-backend-file anywhere in the domain
XML, we can generate predictable paths. In this case:

  $memoryBackingDir/libvirt/qemu/$shortName/$alias

where $shortName is result of virDomainObjGetShortName().




diff --git a/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args 
b/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
index 5700c3413..352819429 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
@@ -10,10 +10,12 @@ QEMU_AUDIO_DRV=none \
 -M pc \
 -m 214 \
 -smp 16,sockets=2,cores=4,threads=2 \
--object memory-backend-file,id=ram-node0,mem-path=/var/lib/libvirt/qemu/ram,\
+-object memory-backend-file,id=ram-node0,\
+mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-QEMUGuest1/ram-node0,\


Heh, we're getting '-1' in all the paths here. Presumably this is because in
the test suite we've not bothered to set the 'id' field to any value.

I can't help thinking that virDomainObjGetShortName ought to return a fatal
error if 'id' is still -1, as in non-test suite code, this would be indication
of a significant screw up by someone trying to create names before the VM config
is updated to reflect running state.  This would of course mean our tests
should set 'id' to a sensible value too.

No need to fix for this patch though - just a curiosity to look at a later
date



That's actually on purpose, this should not happen in the code, if it did, then
it would still function normally.  We should start setting id for live XML in
tests, yes.  When trying to change the fact that parsing was done with _INACTIVE
even for live domains, I found out that we are changing parse flags to _INACTIVE
if the id for the domain is not set, but actually the fallout from that change
is kind of bigger than expected and I did not bother fixing it everywhere.
Dunno about Michal, though, maybe he did :)



Regards,
Daniel
--
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v5 3/3] libvirtd: fix crash on termination

[Erik Skultety]

On Tue, Nov 07, 2017 at 10:28:10AM -0500, John Ferlan wrote:
>
>
> On 11/07/2017 04:46 AM, Martin Kletzander wrote:
> > On Mon, Nov 06, 2017 at 03:20:13PM -0500, John Ferlan wrote:
> >>
> >>
> >> On 11/03/2017 05:20 AM, Nikolay Shirokovskiy wrote:
> >>>
> >>>
> >>> On 03.11.2017 11:42, Martin Kletzander wrote:
>  On Fri, Nov 03, 2017 at 11:07:36AM +0300, Nikolay Shirokovskiy wrote:
> > On 02.11.2017 19:32, Martin Kletzander wrote:
> >> This just makes the window of opportunity (between
> >> daemonServerClose()
> >> and the actual removal of the virNetServerPtr from the hash) smaller.
> >> That's why I don't see it as a fix, rather as a workaround.
> >>
> >> What I think should be done is the order of what's done with
> >> services,
> >> servers, drivers, daemon, workers, etc.
> >>
> >> I read the other threds and I think we're on the same note here, it's
> >> just that there is lot of confusion and we're all approaching it
> >> differently.
> >>
> >> Trying to catch up, but this thread has gone beyond where I was
> >> previously. I still have to push the first two patches of this series...
> >>
> >> Yes, libvirtd cleanup path is awful and there's quite a few "issues"
> >> that have cropped up because previous patches didn't take care of
> >> refcnt'ing properly and just "added" cleanup "somewhere" in the list of
> >> things to be cleaned up... Fortunately to a degree I think this is still
> >> all edge condition type stuff, but getting it right would be good...
> >>
> >> For me theory has always been to cleanup in the inverse order that
> >> something was created - for sure libvirtd doesn't follow that at all.
> >> For libvirtd the shutdown/cleanup logic just seemed too haphazard and
> >> fixing never made it to the top of the list of things to think about.
> >>
> >> Another "caveat" in libvirtd cleanup logic is that driversInitialized is
> >> set as part of a thread that could be running or have run if the code
> >> enters the cleanup: path as a result of virNetlinkEventServiceStart
> >> failure before we enter the event loop (e.g. virNetDaemonRun).
> >>
> >> Still like Erik - I do like Martin's suggestion and would like to see it
> >> as a formal patch - although perhaps with a few tweaks.
> >>
> >> IIRC, this particular patch "worked" because by removing the
> >> dmn->servers refcnt earlier made sure that we didn't have to wait for
> >> virObjectUnref(dmn) to occur after virStateCleanup.
> >>
> >
> > Would it be to daring to ask you guys to send one series that includes
> > all the
> > related patchsets?  We can then discuss there, it would be easier to
> > follow as
> > well.  I don't want anyone to do any extra work, but we might save some
> > review
> > bandwidth by resending those patches in one series.  Or maybe it's just
> > me and
> > I have a hard time keeping up with everything at the same time =)
> >
>
> Originally "this" crash fix was mixed in with the other series to add a
> shutdown state option, but I asked they be separate since they were IMO
> addressing different problems.
>
> To me this series was attempting to resolve a refcnt problem and a
> "latent" free of the dmn->servers that was IIRC attempting to be used
> before the state cleanup and the Unref(dmn) occurred to run the
> virHashFree in virNetDaemonDispose.

Exactly, this was only about refcounting of @dmn, see below for @driver issue.

>
> The other series seems to be adding a state shutdown step to be run
> before state cleanup; however, if we alter the cleanup code in libvirtd
> does that make the state shutdown step unnecessary? I don't know and

Actually, no, the stateShutdown was addressing the issue with running
StateCleanup while there still was an active worker thread that could
potentially access the @driver object, in which case - SIGSEGV, but as I wrote
in my response, I don't like the idea of having another state driver callback,
as I feel it introduces a bit of ambiguity, since by just looking at the
callback names, you can't have an idea, what the difference between
stateCleanup and stateShutdown is. I think this should be handled as part of
the final stage of leaving the event loop and simply register an appropriate
callback to the handle, that way, you don't need to define a new state callback
which would most likely used by a couple of drivers.

Erik

> really didn't want to think about it until we got through thinking about
> the cleanup processing order. Still if one considers that state
> initialization is really two steps (init and auto start), then splitting
> cleanup into shutdown and cleanup seems reasonable, but I don't think
> affects whether we have a crash or latent usage of dmn->servers. I could
> be wrong, but it's so hard to tell.
>
> [...]

Erik

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

Re: [libvirt] [PATCH 0/2] Misc apparmor fixes

[Michal Privoznik]

On 11/03/2017 09:46 AM, Christian Ehrhardt wrote:
> Hi,
> here a few more apparmor fixes for your review.
> 
> One is for an Ubuntu bug [1] which is non fatal, but denies a qemu fix to
> fully work.
> The other one I was carried in Ubuntu for some time and is related to ipv6
> only setups where virt-aa-helper can fail if not permitted inet6.
> 
> [1]: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1729626
> 
> Christian Ehrhardt (2):
>   apparmor: allow qemu to read max_segments
>   apparmor, virt-aa-helper: allow ipv6
> 
>  examples/apparmor/libvirt-qemu   | 3 +++
>  examples/apparmor/usr.lib.libvirt.virt-aa-helper | 1 +
>  2 files changed, 4 insertions(+)
> 

Pushed, thanks.

Michal

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

Re: [libvirt] [PATCH v3 4/5] qemu: Use predictable file names for memory-backend-file

[Michal Privoznik]

On 11/07/2017 04:57 PM, Daniel P. Berrange wrote:
> On Tue, Nov 07, 2017 at 04:51:03PM +0100, Michal Privoznik wrote:
>> In some cases management application needs to allocate memory for
>> qemu upfront and then just let qemu use that. Since we don't want
>> to expose path for memory-backend-file anywhere in the domain
>> XML, we can generate predictable paths. In this case:
>>
>>   $memoryBackingDir/libvirt/qemu/$shortName/$alias
>>
>> where $shortName is result of virDomainObjGetShortName().
>>
> 
>> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args 
>> b/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
>> index 5700c3413..352819429 100644
>> --- a/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
>> +++ b/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
>> @@ -10,10 +10,12 @@ QEMU_AUDIO_DRV=none \
>>  -M pc \
>>  -m 214 \
>>  -smp 16,sockets=2,cores=4,threads=2 \
>> --object 
>> memory-backend-file,id=ram-node0,mem-path=/var/lib/libvirt/qemu/ram,\
>> +-object memory-backend-file,id=ram-node0,\
>> +mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-QEMUGuest1/ram-node0,\
> 
> Heh, we're getting '-1' in all the paths here. Presumably this is because in
> the test suite we've not bothered to set the 'id' field to any value.
> 
> I can't help thinking that virDomainObjGetShortName ought to return a fatal
> error if 'id' is still -1, as in non-test suite code, this would be indication
> of a significant screw up by someone trying to create names before the VM 
> config
> is updated to reflect running state.  This would of course mean our tests
> should set 'id' to a sensible value too.
> 
> No need to fix for this patch though - just a curiosity to look at a later
> date

Yeah well,

libvirt.git $ git grep "domain.*-1" tests/ | grep args | wc -l
603

I strongly vote for fixing it up separately then.

Michal

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

Re: [libvirt] [PATCH v3 4/5] qemu: Use predictable file names for memory-backend-file

[Daniel P. Berrange]

On Tue, Nov 07, 2017 at 04:51:03PM +0100, Michal Privoznik wrote:
> In some cases management application needs to allocate memory for
> qemu upfront and then just let qemu use that. Since we don't want
> to expose path for memory-backend-file anywhere in the domain
> XML, we can generate predictable paths. In this case:
> 
>   $memoryBackingDir/libvirt/qemu/$shortName/$alias
> 
> where $shortName is result of virDomainObjGetShortName().
> 

> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args 
> b/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
> index 5700c3413..352819429 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args
> @@ -10,10 +10,12 @@ QEMU_AUDIO_DRV=none \
>  -M pc \
>  -m 214 \
>  -smp 16,sockets=2,cores=4,threads=2 \
> --object memory-backend-file,id=ram-node0,mem-path=/var/lib/libvirt/qemu/ram,\
> +-object memory-backend-file,id=ram-node0,\
> +mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-QEMUGuest1/ram-node0,\

Heh, we're getting '-1' in all the paths here. Presumably this is because in
the test suite we've not bothered to set the 'id' field to any value.

I can't help thinking that virDomainObjGetShortName ought to return a fatal
error if 'id' is still -1, as in non-test suite code, this would be indication
of a significant screw up by someone trying to create names before the VM config
is updated to reflect running state.  This would of course mean our tests
should set 'id' to a sensible value too.

No need to fix for this patch though - just a curiosity to look at a later
date


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] Libvirt config converter can't handle file not ending with new line

[Wim ten Have]

On Tue, 7 Nov 2017 12:20:05 +
Wei Liu  wrote:

> On Mon, Nov 06, 2017 at 09:41:01PM -0700, Jim Fehlig wrote:
> > On 10/30/2017 06:17 AM, Wei Liu wrote:  
> > > Hi Jim
> > > 
> > > I discover a problem when using xen_xl converter. When the file in
> > > question doesn't end with a new line, I get the following error:
> > > 
> > >error: configuration file syntax error: memory conf:53: expecting a 
> > > value  
> > 
> > I'm not able to reproduce this issue. The libvirt.git tree I tried was a bit
> > dated, but even after updating to latest master I can't reproduce.
> >   
> > > After digging a bit (but haven't read libvirt code), it appears that the
> > > file didn't end with a new line.  
> > 
> > I tried several files without ending new lines, going both directions
> > (domxml-to-native and domxml-from-native), but didn't see the mentioned
> > error. Perhaps your config is revealing another bug which is being
> > improperly reported. Can you provide an example of the problematic config?
> >   
> 
> I tried to get the exact file that caused the problem but it is already
> destroyed by osstest.
> 
> A similar file:
> 
> http://logs.test-lab.xenproject.org/osstest/logs/115436/test-amd64-amd64-libvirt-pair/debian.guest.osstest.cfg
> 
> If you hexdump -C it, you can see the last character is 0a. Remove it and
> feed the file into the converter.
> Wei.

  The phenonomem you point out is indeed weird.  And my first response
  is that this is a bug parsing the cfg input.  I did little explore and
  think that src/util/virconf.c (virConfParseLong(), virConfParseValue()) 
  should be reworked as pointed out in below context diffs.

 git diff
diff --git a/src/util/virconf.c b/src/util/virconf.c
index 39c2bd917..bc8e57ec3 100644
--- a/src/util/virconf.c
+++ b/src/util/virconf.c
@@ -352,7 +352,7 @@ virConfParseLong(virConfParserCtxtPtr ctxt, long 
long *val)
 } else if (CUR == '+') {
 NEXT;
 }
-if ((ctxt->cur >= ctxt->end) || (!c_isdigit(CUR))) {
+if ((ctxt->cur > ctxt->end) || (!c_isdigit(CUR))) {
 virConfError(ctxt, VIR_ERR_CONF_SYNTAX, _("unterminated 
number"));
 return -1;
 }
@@ -456,7 +456,7 @@ virConfParseValue(virConfParserCtxtPtr ctxt)
 long long l = 0;
 
 SKIP_BLANKS;
-if (ctxt->cur >= ctxt->end) {
+if (ctxt->cur > ctxt->end) {
 virConfError(ctxt, VIR_ERR_CONF_SYNTAX, _("expecting a 
value"));
 return NULL;
 }

  I did not go beyond this yet.

Rgds,
- Wim.

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

[libvirt] [PATCH v3 1/5] qemu: Set alias for memory cell in qemuBuildMemoryCellBackendStr

[Michal Privoznik]

Very soon qemuBuildMemoryBackendStr() is going to use memory cell
aliases. Therefore set one. At the same time, move it a bit
further - if virAsprintf() fails, there's no point in setting
rest of the members.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_command.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 2fe4ae380..c949afe90 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3522,12 +3522,13 @@ qemuBuildMemoryCellBackendStr(virDomainDefPtr def,
 unsigned long long memsize = virDomainNumaGetNodeMemorySize(def->numa,
 cell);
 
+if (virAsprintf(&alias, "ram-node%zu", cell) < 0)
+goto cleanup;
+
 *backendStr = NULL;
 mem.size = memsize;
 mem.targetNode = cell;
-
-if (virAsprintf(&alias, "ram-node%zu", cell) < 0)
-goto cleanup;
+mem.info.alias = alias;
 
 if ((rc = qemuBuildMemoryBackendStr(&props, &backendType, cfg, 
priv->qemuCaps,
 def, &mem, priv->autoNodeset, false)) 
< 0)
-- 
2.13.6

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

[libvirt] [PATCH v3 3/5] qemu: Destroy whole memory tree

[Michal Privoznik]

When removing path where huge pages are call virFileDeleteTree
instead of plain rmdir(). The reason is that in the near future
there's going to be more in the path than just files - some
subdirs. Therefore plain rmdir() is not going to be enough.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_process.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index e27cd0d40..8eef2794e 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3348,7 +3348,7 @@ qemuProcessBuildDestroyMemoryPathsImpl(virQEMUDriverPtr 
driver,
 return -1;
 }
 } else {
-if (rmdir(path) < 0 &&
+if (virFileDeleteTree(path) < 0 &&
 errno != ENOENT)
 VIR_WARN("Unable to remove hugepage path: %s (errno=%d)",
  path, errno);
-- 
2.13.6

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

Re: [libvirt] Should we switch to a different JSON library?

[Neal Gompa]

On Tue, Nov 7, 2017 at 9:34 AM, Martin Kletzander  wrote:
> On Tue, Nov 07, 2017 at 01:38:51PM +, Richard W.M. Jones wrote:
>>
>> On Tue, Nov 07, 2017 at 01:28:00PM +, Daniel P. Berrange wrote:
>>>
>>> On Tue, Nov 07, 2017 at 02:05:25PM +0100, Martin Kletzander wrote:
>>> >  4) The are workarounds for it in the code
>>>
>>> I don't recall what these are now ?
>>
>>
>
> I shouldn't have said 'workarounds', I meant all the conditioning for
> differentiation between yajl 1 and 2 and supporting both version with
> pkg-config
> and without.
>
>> libguestfs needs to disable some warnings around yajl code:
>>
>>  /* GCC can't work out that the YAJL_IS_ test is sufficient to
>>   * ensure that YAJL_GET_ later doesn't return NULL.
>>   */
>>  #if defined(__GNUC__) && __GNUC__ >= 6 /* gcc >= 6 */
>>  #pragma GCC diagnostic ignored "-Wnull-dereference"
>>  #endif
>>
>> However we don't need any other workarounds.
>>
>> As you say yajl "just works".
>>
>> It might however be a problem in future if JSON itself was changed,
>> eg. new types, fix the trailing comma problem, etc.  We'd want a more
>> responsive upstream to handle that.
>>
>> - - -
>>
>> Martin, did you look at the dependencies of these other libraries?
>> If they depend on other stuff, that could make them harder to use.
>> Also are they all available in at least Fedora, Debian, OpenSUSE and
>> Arch/AUR?
>>
>
> Just did, all of these were successful:
>
> docker run -it fedora dnf search jansson
> docker run -it opensuse zypper search jansson
> docker run -it base/archlinux pacman -Sys jansson
> docker run -it ubuntu sh -c "apt update; apt search jansson"
> docker run -it debian sh -c "apt update; apt search jansson"
>
> Even:
>
> docker run -it debian:stable sh -c "apt update; apt search jansson"
>
> found version 2.9 which is the second latest one, I believe.
>
>

I used Repology to check jansson:
https://repology.org/metapackage/jansson/versions

It looks like the range of supported versions is v2.7 to v2.10.

I see no reason it can't be used.

-- 
真実はいつも一つ！/ Always, there's only one truth!

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

[libvirt] [PATCH v3 5/5] news: Document predictable file names for memory-backend-file

[Michal Privoznik]

Signed-off-by: Michal Privoznik 
---
 docs/news.xml | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml
index ef855d895..fb0ded97c 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -37,6 +37,17 @@
 
 
 
+  
+
+  qemu: Generate predictable paths for qemu memory backends
+
+
+  In some cases management applications need to know
+  paths passed to memory-backend-file objects upfront.
+  Libvirt now generates predictable paths so applications
+  can prepare the files if they need to do so.
+
+  
 
 
 
-- 
2.13.6

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

[libvirt] [PATCH v3 2/5] qemu: Rename qemuProcessBuildDestroyHugepagesPath

[Michal Privoznik]

At the same time, move its internals into a separate function so
that they can be reused.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_hotplug.c |  2 +-
 src/qemu/qemu_process.c | 76 +
 src/qemu/qemu_process.h |  8 +++---
 3 files changed, 50 insertions(+), 36 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index e4157f631..ce63b4a4d 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2077,7 +2077,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
   priv->qemuCaps, vm->def, mem, NULL, true) < 
0)
 goto cleanup;
 
-if (qemuProcessBuildDestroyHugepagesPath(driver, vm, mem, true) < 0)
+if (qemuProcessBuildDestroyMemoryPaths(driver, vm, mem, true) < 0)
 goto cleanup;
 
 if (qemuDomainNamespaceSetupMemory(driver, vm, mem) < 0)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 7df440ee4..e27cd0d40 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3324,11 +3324,45 @@ qemuProcessNeedHugepagesPath(virDomainDefPtr def,
 }
 
 
+static int
+qemuProcessBuildDestroyMemoryPathsImpl(virQEMUDriverPtr driver,
+   virDomainDefPtr def,
+   const char *path,
+   bool build)
+{
+if (build) {
+if (virFileExists(path))
+return 0;
+
+if (virFileMakePathWithMode(path, 0700) < 0) {
+virReportSystemError(errno,
+ _("Unable to create %s"),
+ path);
+return -1;
+}
+
+if (qemuSecurityDomainSetPathLabel(driver->securityManager,
+   def, path) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+_("Unable to label %s"), path);
+return -1;
+}
+} else {
+if (rmdir(path) < 0 &&
+errno != ENOENT)
+VIR_WARN("Unable to remove hugepage path: %s (errno=%d)",
+ path, errno);
+}
+
+return 0;
+}
+
+
 int
-qemuProcessBuildDestroyHugepagesPath(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainMemoryDefPtr mem,
- bool build)
+qemuProcessBuildDestroyMemoryPaths(virQEMUDriverPtr driver,
+   virDomainObjPtr vm,
+   virDomainMemoryDefPtr mem,
+   bool build)
 {
 virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
 char *hugepagePath = NULL;
@@ -3347,31 +3381,11 @@ qemuProcessBuildDestroyHugepagesPath(virQEMUDriverPtr 
driver,
 if (!hugepagePath)
 goto cleanup;
 
-if (build) {
-if (virFileExists(hugepagePath)) {
-ret = 0;
-goto cleanup;
-}
+if (qemuProcessBuildDestroyMemoryPathsImpl(driver, vm->def,
+   hugepagePath, build) < 
0)
+goto cleanup;
 
-if (virFileMakePathWithMode(hugepagePath, 0700) < 0) {
-virReportSystemError(errno,
- _("Unable to create %s"),
- hugepagePath);
-goto cleanup;
-}
-
-if (qemuSecurityDomainSetPathLabel(driver->securityManager,
-   vm->def, hugepagePath) < 0) 
{
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   "%s", _("Unable to set huge path in 
security driver"));
-goto cleanup;
-}
-} else {
-if (rmdir(hugepagePath) < 0 &&
-errno != ENOENT)
-VIR_WARN("Unable to remove hugepage path: %s (errno=%d)",
- hugepagePath, errno);
-}
+VIR_FREE(hugepagePath);
 }
 }
 
@@ -5550,7 +5564,7 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver,
NULL) < 0)
 goto cleanup;
 
-if (qemuProcessBuildDestroyHugepagesPath(driver, vm, NULL, true) < 0)
+if (qemuProcessBuildDestroyMemoryPaths(driver, vm, NULL, true) < 0)
 goto cleanup;
 
 /* Ensure no historical cgroup for this VM is lying around bogus
@@ -6254,7 +6268,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,
 goto endjob;
 }
 
-qemuProcessBuildDestroyHugepagesPath(driver, vm, NULL, false);
+qemuProcessBuildDestroyMemoryPaths(driver, vm, NULL, false);
 
 vm->def->id = -1;
 
@@ -7112,7 +7126,7 @@ qemuProcessReconnect(void *opaque)
 goto cleanup;
 }
 
-if (qemuProcessBui

[libvirt] [PATCH v3 4/5] qemu: Use predictable file names for memory-backend-file

[Michal Privoznik]

In some cases management application needs to allocate memory for
qemu upfront and then just let qemu use that. Since we don't want
to expose path for memory-backend-file anywhere in the domain
XML, we can generate predictable paths. In this case:

  $memoryBackingDir/libvirt/qemu/$shortName/$alias

where $shortName is result of virDomainObjGetShortName().

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_command.c|  2 +-
 src/qemu/qemu_conf.c   | 55 +-
 src/qemu/qemu_conf.h   |  9 ++-
 src/qemu/qemu_driver.c | 17 ++
 src/qemu/qemu_process.c| 65 ++
 .../qemuxml2argv-cpu-numa-memshared.args   |  6 +-
 .../qemuxml2argv-fd-memory-numa-topology.args  |  3 +-
 .../qemuxml2argv-fd-memory-numa-topology2.args |  6 +-
 .../qemuxml2argv-fd-memory-numa-topology3.args |  9 ++-
 .../qemuxml2argv-hugepages-memaccess2.args |  9 ++-
 10 files changed, 155 insertions(+), 26 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c949afe90..c12cfec7a 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3419,7 +3419,7 @@ qemuBuildMemoryBackendStr(virJSONValuePtr *backendProps,
 } else {
 /* We can have both pagesize and mem source. If that's the case,
  * prefer hugepages as those are more specific. */
-if (qemuGetMemoryBackingPath(cfg, &memPath) < 0)
+if (qemuGetMemoryBackingPath(def, cfg, mem->info.alias, &memPath) 
< 0)
 goto cleanup;
 }
 
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index f3cff1503..af503d31c 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1750,9 +1750,41 @@ qemuGetDomainHupageMemPath(const virDomainDef *def,
 }
 
 
+int
+qemuGetMemoryBackingBasePath(virQEMUDriverConfigPtr cfg,
+ char **path)
+{
+return virAsprintf(path, "%s/libvirt/qemu", cfg->memoryBackingDir);
+}
+
+
+int
+qemuGetMemoryBackingDomainPath(const virDomainDef *def,
+   virQEMUDriverConfigPtr cfg,
+   char **path)
+{
+char *shortName = NULL;
+char *base = NULL;
+int ret = -1;
+
+if (!(shortName = virDomainDefGetShortName(def)) ||
+qemuGetMemoryBackingBasePath(cfg, &base) < 0 ||
+virAsprintf(path, "%s/%s", base, shortName) < 0)
+goto cleanup;
+
+ret = 0;
+ cleanup:
+VIR_FREE(base);
+VIR_FREE(shortName);
+return ret;
+}
+
+
 /**
  * qemuGetMemoryBackingPath:
+ * @def: domain definition
  * @cfg: the driver config
+ * @alias: memory object alias
  * @memPath: constructed path
  *
  * Constructs path to memory backing dir and stores it at @memPath.
@@ -1761,8 +1793,27 @@ qemuGetDomainHupageMemPath(const virDomainDef *def,
  *  -1 otherwise (with error reported).
  */
 int
-qemuGetMemoryBackingPath(virQEMUDriverConfigPtr cfg,
+qemuGetMemoryBackingPath(const virDomainDef *def,
+ virQEMUDriverConfigPtr cfg,
+ const char *alias,
  char **memPath)
 {
-return VIR_STRDUP(*memPath, cfg->memoryBackingDir);
+char *domainPath = NULL;
+int ret = -1;
+
+if (!alias) {
+/* This should never happen (TM) */
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("memory device alias is not assigned"));
+goto cleanup;
+}
+
+if (qemuGetMemoryBackingDomainPath(def, cfg, &domainPath) < 0 ||
+virAsprintf(memPath, "%s/%s", domainPath, alias) < 0)
+goto cleanup;
+
+ret = 0;
+ cleanup:
+VIR_FREE(domainPath);
+return ret;
 }
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 9d6866816..a553e30e2 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -364,6 +364,13 @@ int qemuGetDomainHupageMemPath(const virDomainDef *def,
unsigned long long pagesize,
char **memPath);
 
-int qemuGetMemoryBackingPath(virQEMUDriverConfigPtr cfg,
+int qemuGetMemoryBackingBasePath(virQEMUDriverConfigPtr cfg,
+ char **path);
+int qemuGetMemoryBackingDomainPath(const virDomainDef *def,
+   virQEMUDriverConfigPtr cfg,
+   char **path);
+int qemuGetMemoryBackingPath(const virDomainDef *def,
+ virQEMUDriverConfigPtr cfg,
+ const char *alias,
  char **memPath);
 #endif /* __QEMUD_CONF_H */
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index bb53d78ea..47f85b9bf 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -631,6 +631,7 @@ qemuStateInitialize(bool privileged,
 uid_t run_uid = -1;
 gid_t run_gid = -1;
 char *hugepagePath 

[libvirt] [PATCH v3 0/5] Predictable file names for memory-backend-file

[Michal Privoznik]

v3 of:

https://www.redhat.com/archives/libvir-list/2017-October/msg01091.html

diff to v2:
- Pushed 1/4 and 2/4 from the original series
- Split 3/4 into smaller patches

Michal Privoznik (5):
  qemu: Set alias for memory cell in qemuBuildMemoryCellBackendStr
  qemu: Rename qemuProcessBuildDestroyHugepagesPath
  qemu: Destroy whole memory tree
  qemu: Use predictable file names for memory-backend-file
  news: Document predictable file names for memory-backend-file

 docs/news.xml  |  11 ++
 src/qemu/qemu_command.c|   9 +-
 src/qemu/qemu_conf.c   |  55 -
 src/qemu/qemu_conf.h   |   9 +-
 src/qemu/qemu_driver.c |  17 +++
 src/qemu/qemu_hotplug.c|   2 +-
 src/qemu/qemu_process.c| 137 +++--
 src/qemu/qemu_process.h|   8 +-
 .../qemuxml2argv-cpu-numa-memshared.args   |   6 +-
 .../qemuxml2argv-fd-memory-numa-topology.args  |   3 +-
 .../qemuxml2argv-fd-memory-numa-topology2.args |   6 +-
 .../qemuxml2argv-fd-memory-numa-topology3.args |   9 +-
 .../qemuxml2argv-hugepages-memaccess2.args |   9 +-
 13 files changed, 218 insertions(+), 63 deletions(-)

-- 
2.13.6

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

Re: [libvirt] [PATCH 06/12] qemu: block: Add JSON props generator for iSCSI protocol

[Ján Tomko]

On Tue, Nov 07, 2017 at 04:04:02PM +0100, Peter Krempa wrote:

On Sun, Nov 05, 2017 at 16:03:25 +0100, Ján Tomko wrote:

On Fri, Nov 03, 2017 at 03:29:23PM +0100, Peter Krempa wrote:
> From: John Ferlan 
>
> ---
> +static virJSONValuePtr
> +qemuBlockStorageSourceGetISCSIProps(virStorageSourcePtr src)
> +{

[...]

> +if (VIR_STRDUP(target, src->path) < 0)
> +goto cleanup;
> +
> +/* Separate the target and lun */
> +if ((lunStr = strchr(target, '/'))) {
> +*(lunStr++) = '\0';
> +if (virStrToLong_ui(lunStr, NULL, 10, &lun) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("cannot parse target for lunStr '%s'"),
> +   target);
> +goto cleanup;
> +}
> +}
> +
> +/* combine host and port into portal */
> +if (virAsprintf(&portal, "%s:%u", src->hosts[0].name, src->hosts[0].port) 
< 0)
> +goto cleanup;

Can src->hosts[0].name possibly be a literal IPv6 address?


Yes, you are right. How about the following diff squashed in?

diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index ffe2892ab..428c0b465 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -742,8 +742,15 @@ qemuBlockStorageSourceGetISCSIProps(virStorageSourcePtr 
src)
}

/* combine host and port into portal */
-if (virAsprintf(&portal, "%s:%u", src->hosts[0].name, src->hosts[0].port) 
< 0)
-goto cleanup;
+if (virSocketAddrNumericFamily(src->hosts[0].name) == AF_INET6) {
+if (virAsprintf(&portal, "[%s]:%u",
+src->hosts[0].name, src->hosts[0].port) < 0)
+goto cleanup;
+} else {
+if (virAsprintf(&portal, "%s:%u",
+src->hosts[0].name, src->hosts[0].port) < 0)
+goto cleanup;
+}

if (src->auth) {
username = src->auth->username;



ACK


Also our parser is buggy. I'll send patches separately.


Thanks, patches are welcome.

Jan


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v5 3/3] libvirtd: fix crash on termination

[John Ferlan]

On 11/07/2017 04:46 AM, Martin Kletzander wrote:
> On Mon, Nov 06, 2017 at 03:20:13PM -0500, John Ferlan wrote:
>>
>>
>> On 11/03/2017 05:20 AM, Nikolay Shirokovskiy wrote:
>>>
>>>
>>> On 03.11.2017 11:42, Martin Kletzander wrote:
 On Fri, Nov 03, 2017 at 11:07:36AM +0300, Nikolay Shirokovskiy wrote:
> On 02.11.2017 19:32, Martin Kletzander wrote:
>> This just makes the window of opportunity (between
>> daemonServerClose()
>> and the actual removal of the virNetServerPtr from the hash) smaller.
>> That's why I don't see it as a fix, rather as a workaround.
>>
>> What I think should be done is the order of what's done with
>> services,
>> servers, drivers, daemon, workers, etc.
>>
>> I read the other threds and I think we're on the same note here, it's
>> just that there is lot of confusion and we're all approaching it
>> differently.
>>
>> Trying to catch up, but this thread has gone beyond where I was
>> previously. I still have to push the first two patches of this series...
>>
>> Yes, libvirtd cleanup path is awful and there's quite a few "issues"
>> that have cropped up because previous patches didn't take care of
>> refcnt'ing properly and just "added" cleanup "somewhere" in the list of
>> things to be cleaned up... Fortunately to a degree I think this is still
>> all edge condition type stuff, but getting it right would be good...
>>
>> For me theory has always been to cleanup in the inverse order that
>> something was created - for sure libvirtd doesn't follow that at all.
>> For libvirtd the shutdown/cleanup logic just seemed too haphazard and
>> fixing never made it to the top of the list of things to think about.
>>
>> Another "caveat" in libvirtd cleanup logic is that driversInitialized is
>> set as part of a thread that could be running or have run if the code
>> enters the cleanup: path as a result of virNetlinkEventServiceStart
>> failure before we enter the event loop (e.g. virNetDaemonRun).
>>
>> Still like Erik - I do like Martin's suggestion and would like to see it
>> as a formal patch - although perhaps with a few tweaks.
>>
>> IIRC, this particular patch "worked" because by removing the
>> dmn->servers refcnt earlier made sure that we didn't have to wait for
>> virObjectUnref(dmn) to occur after virStateCleanup.
>>
> 
> Would it be to daring to ask you guys to send one series that includes
> all the
> related patchsets?  We can then discuss there, it would be easier to
> follow as
> well.  I don't want anyone to do any extra work, but we might save some
> review
> bandwidth by resending those patches in one series.  Or maybe it's just
> me and
> I have a hard time keeping up with everything at the same time =)
> 

Originally "this" crash fix was mixed in with the other series to add a
shutdown state option, but I asked they be separate since they were IMO
addressing different problems.

To me this series was attempting to resolve a refcnt problem and a
"latent" free of the dmn->servers that was IIRC attempting to be used
before the state cleanup and the Unref(dmn) occurred to run the
virHashFree in virNetDaemonDispose.

The other series seems to be adding a state shutdown step to be run
before state cleanup; however, if we alter the cleanup code in libvirtd
does that make the state shutdown step unnecessary? I don't know and
really didn't want to think about it until we got through thinking about
the cleanup processing order. Still if one considers that state
initialization is really two steps (init and auto start), then splitting
cleanup into shutdown and cleanup seems reasonable, but I don't think
affects whether we have a crash or latent usage of dmn->servers. I could
be wrong, but it's so hard to tell.

[...]

>>
>>
>> So you cut off Martin's patch here, but if one essentially did inverse
>> order of startup they'd get the following: (the // comments are my notes
>> about startup order oddity).
>>
>> cleanup:
>>    virNetDaemonClose(dmn);
>>
>>    virNetlinkEventServiceStopAll();
>>
>>    if (driversInitialized) {
>>    /* Set via daemonRunStateInit in daemonStateInit */
>>    driversInitialized = false;
>>    virStateCleanup();
>>    }
>>
>>    virObjectUnref(adminProgram);
>>    virObjectUnref(srvAdm);
>>    virObjectUnref(qemuProgram);
>>    virObjectUnref(lxcProgram);
>>    virObjectUnref(remoteProgram);
>>
>>    // FWIW: Not exact here, but the order of startup should be
>>    // changed since srv is added to dmn, so realistically srv
>>    // depends on dmn existence.
>>    virObjectUnref(srv);
> 
> We can unref all of the above whenever we please.  @dmn keeps references
> for
> everything it needs, we just have @srv in order to set some things up,
> we can
> unref it right after we add it to @dmn.  The dependencies here are not the
> concern thanks to (hopefully) proper refcnt'ing.

OK - that's a bit more adjustment, but totally possible...

> 
>>    virObjectUnref(dmn);
>>
>>    virNetlinkShutdown();
>>
>

[libvirt] [PATCH] util: storage: Fix parsing of IPv6 portal address for iSCSI

[Peter Krempa]

Split on the last colon and avoid parsing port if the split remainder
contains the closing square bracket, so that IPv6 addresses are
interpreted correctly.
---
 src/util/virstoragefile.c |  3 ++-
 tests/virstoragetest.c| 20 
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 3a2d2aa05..e7fcb1238 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -3008,7 +3008,8 @@ virStorageSourceParseBackingJSONiSCSI(virStorageSourcePtr 
src,
 if (VIR_STRDUP(src->hosts->name, portal) < 0)
 goto cleanup;

-if ((port = strchr(src->hosts->name, ':'))) {
+if ((port = strrchr(src->hosts->name, ':')) &&
+!strchr(port, ']')) {
 if (virStringParsePort(port + 1, &src->hosts->port) < 0)
 goto cleanup;

diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index cfcd8a79c..52a685d91 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -1578,6 +1578,26 @@ mymain(void)
"\n"
"  \n"
"\n");
+TEST_BACKING_PARSE("json:{\"file\":{\"driver\":\"iscsi\","
+   "\"transport\":\"tcp\","
+   "\"portal\":\"[2001::0]:1234\","
+   
"\"target\":\"iqn.2016-12.com.virttest:emulated-iscsi-noauth.target\","
+   "\"lun\":6"
+  "}"
+"}",
+   "\n"
+   "  \n"
+   "\n");
+TEST_BACKING_PARSE("json:{\"file\":{\"driver\":\"iscsi\","
+   "\"transport\":\"tcp\","
+   "\"portal\":\"[2001::0]\","
+   
"\"target\":\"iqn.2016-12.com.virttest:emulated-iscsi-noauth.target\","
+   "\"lun\":6"
+  "}"
+"}",
+   "\n"
+   "  \n"
+   "\n");
 TEST_BACKING_PARSE("json:{\"file\":{\"driver\":\"sheepdog\","
"\"vdi\":\"test\","
"\"server\":{ \"type\":\"inet\","
-- 
2.14.3

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

Re: [libvirt] [PATCH 1/2] storage: Resolve storage driver crash

[Cedric Bosdonnat]

On Tue, 2017-11-07 at 08:43 -0500, John Ferlan wrote:
> 
> On 11/07/2017 04:18 AM, Cedric Bosdonnat wrote:
> > On Mon, 2017-11-06 at 15:53 -0500, John Ferlan wrote:
> > > Resolve a storage driver crash as a result of a long running
> > > storageVolCreateXML when the virStorageVolPoolRefreshThread is
> > > run as a result of a storageVolUpload complete and ran the
> > > virStoragePoolObjClearVols without checking if the creation
> > > code was currently processing a buildVol after incrementing
> > > the driver->asyncjob count.
> > > 
> > > The refreshThread needs to wait until all creation threads
> > > are completed so as to not alter the volume list while the
> > > volume is being built.
> > > 
> > > Crash from valgrind is as follows (with a bit of editing):
> > > 
> > > ==21309== Invalid read of size 8
> > > ==21309==at 0x153E47AF: storageBackendUpdateVolTargetInfo
> > > ==21309==by 0x153E4C30: virStorageBackendUpdateVolInfo
> > > ==21309==by 0x153E52DE: virStorageBackendVolRefreshLocal
> > > ==21309==by 0x153DE29E: storageVolCreateXML
> > > ==21309==by 0x562035B: virStorageVolCreateXML
> > > ==21309==by 0x147366: remoteDispatchStorageVolCreateXML
> > > ...
> > > ==21309==  Address 0x2590a720 is 64 bytes inside a block of size 336 
> > > free'd
> > > ==21309==at 0x4C2F2BB: free
> > > ==21309==by 0x54CB9FA: virFree
> > > ==21309==by 0x55BC800: virStorageVolDefFree
> > > ==21309==by 0x55BF1D8: virStoragePoolObjClearVols
> > > ==21309==by 0x153D967E: virStorageVolPoolRefreshThread
> > > ...
> > > ==21309==  Block was alloc'd at
> > > ==21309==at 0x4C300A5: calloc
> > > ==21309==by 0x54CB483: virAlloc
> > > ==21309==by 0x55BDC1F: virStorageVolDefParseXML
> > > ==21309==by 0x55BDC1F: virStorageVolDefParseNode
> > > ==21309==by 0x55BE5A4: virStorageVolDefParse
> > > ==21309==by 0x153DDFF1: storageVolCreateXML
> > > ==21309==by 0x562035B: virStorageVolCreateXML
> > > ==21309==by 0x147366: remoteDispatchStorageVolCreateXML
> > > ...
> > > 
> > > Signed-off-by: John Ferlan 
> > > ---
> > >  src/storage/storage_driver.c | 9 +
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> > > index b0edf9f885..5e920fc14c 100644
> > > --- a/src/storage/storage_driver.c
> > > +++ b/src/storage/storage_driver.c
> > > @@ -2257,6 +2257,7 @@ virStorageVolPoolRefreshThread(void *opaque)
> > >  virStorageBackendPtr backend;
> > >  virObjectEventPtr event = NULL;
> > >  
> > > + retry:
> > >  storageDriverLock();
> > >  if (cbdata->vol_path) {
> > >  if (virStorageBackendPloopRestoreDesc(cbdata->vol_path) < 0)
> > > @@ -2270,6 +2271,14 @@ virStorageVolPoolRefreshThread(void *opaque)
> > >  if (!(backend = virStorageBackendForType(def->type)))
> > >  goto cleanup;
> > >  
> > > +/* Some thread is creating a new volume in the pool, we need to 
> > > retry */
> > > +if (virStoragePoolObjGetAsyncjobs(obj) > 0) {
> > > +virStoragePoolObjUnlock(obj);
> > > +storageDriverUnlock();
> > > +usleep(100 * 1000);
> > > +goto retry;
> > > +}
> > > +
> > >  virStoragePoolObjClearVols(obj);
> > >  if (backend->refreshPool(NULL, obj) < 0)
> > >  VIR_DEBUG("Failed to refresh storage pool");
> > 
> > ACK, does the job here. I'm rather surprised to see you implementing it
> > with sleep, while you pointed me towards virCondWait yesterday. But using
> > sleep is a way less intrusive change.
> > 
> 
> Well you only asked about an alternative mechanism and condition
> variable was the first thing that popped into my mind; however, as I got
> to actually doing more thinking about it - asyncjobs is not blocking
> multiple creates from occurring; whereas, a condition variable would be
> waiting for one thing to complete.
> 
> My response to Jan also lists 2 other alternatives. This was just the
> "easiest" of the 3.  If there's other ideas, I'm open to suggestions.
> 
It looks fine to me, I was just surprised to see the sleep version ;)

--
Cedric

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

Re: [libvirt] [PATCH v3 REBASE v2 0/2] qemu: report block job errors from qemu to the user

[Nikolay Shirokovskiy]

ping

On 27.10.2017 15:37, Nikolay Shirokovskiy wrote:
> So that you can see nice report on migration:
> 
> "error: operation failed: migration of disk sda failed: No space left on 
> device"
> 
> diff from v2:
> 
> 1. split into 2 patches
> 2. change formal documentation where it is present accordingly
> 3. add variable initialization for safety
> 
> Nikolay Shirokovskiy (2):
>   qemu: prepare blockjob complete event error usage
>   qemu: report drive mirror errors on migration
> 
>  src/qemu/qemu_blockjob.c | 14 +--
>  src/qemu/qemu_blockjob.h |  3 ++-
>  src/qemu/qemu_domain.c   | 10 +++-
>  src/qemu/qemu_domain.h   |  1 +
>  src/qemu/qemu_driver.c   |  4 ++--
>  src/qemu/qemu_migration.c| 55 
> +++-
>  src/qemu/qemu_monitor.c  |  5 ++--
>  src/qemu/qemu_monitor.h  |  4 +++-
>  src/qemu/qemu_monitor_json.c |  4 +++-
>  src/qemu/qemu_process.c  |  4 
>  10 files changed, 78 insertions(+), 26 deletions(-)
> 

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

Re: [libvirt] [PATCH 06/12] qemu: block: Add JSON props generator for iSCSI protocol

[Peter Krempa]

On Sun, Nov 05, 2017 at 16:03:25 +0100, Ján Tomko wrote:
> On Fri, Nov 03, 2017 at 03:29:23PM +0100, Peter Krempa wrote:
> > From: John Ferlan 
> > 
> > ---
> > +static virJSONValuePtr
> > +qemuBlockStorageSourceGetISCSIProps(virStorageSourcePtr src)
> > +{
> 
> [...]
> 
> > +if (VIR_STRDUP(target, src->path) < 0)
> > +goto cleanup;
> > +
> > +/* Separate the target and lun */
> > +if ((lunStr = strchr(target, '/'))) {
> > +*(lunStr++) = '\0';
> > +if (virStrToLong_ui(lunStr, NULL, 10, &lun) < 0) {
> > +virReportError(VIR_ERR_INTERNAL_ERROR,
> > +   _("cannot parse target for lunStr '%s'"),
> > +   target);
> > +goto cleanup;
> > +}
> > +}
> > +
> > +/* combine host and port into portal */
> > +if (virAsprintf(&portal, "%s:%u", src->hosts[0].name, 
> > src->hosts[0].port) < 0)
> > +goto cleanup;
> 
> Can src->hosts[0].name possibly be a literal IPv6 address?

Yes, you are right. How about the following diff squashed in?

diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index ffe2892ab..428c0b465 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -742,8 +742,15 @@ qemuBlockStorageSourceGetISCSIProps(virStorageSourcePtr 
src)
 }

 /* combine host and port into portal */
-if (virAsprintf(&portal, "%s:%u", src->hosts[0].name, src->hosts[0].port) 
< 0)
-goto cleanup;
+if (virSocketAddrNumericFamily(src->hosts[0].name) == AF_INET6) {
+if (virAsprintf(&portal, "[%s]:%u",
+src->hosts[0].name, src->hosts[0].port) < 0)
+goto cleanup;
+} else {
+if (virAsprintf(&portal, "%s:%u",
+src->hosts[0].name, src->hosts[0].port) < 0)
+goto cleanup;
+}

 if (src->auth) {
 username = src->auth->username;

Also our parser is buggy. I'll send patches separately.


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] Should we switch to a different JSON library?

[Martin Kletzander]

On Tue, Nov 07, 2017 at 01:28:00PM +, Daniel P. Berrange wrote:

On Tue, Nov 07, 2017 at 02:05:25PM +0100, Martin Kletzander wrote:

Hi everyone,

so we are using yajl for parsing JSON.  However there are some reasons
why we might consider switching to another one:

 1) It is basically dead upstream


One could say that it is simply "feature complete" upstream so doesn't
need more work, but none the less, I have a general bad feeling about
relying on a library with no active maint work going on for best part
of 4 years, and 68 open pull requests.


 2) We're just using the lexer part of it
 3) We only use it for parsing


Aren't 2 & 3 the same point repeated ? Regardless though, we use yajl
for formatting JSON too (virJSONValueToString)



I meant 2 different things.  By only using the parser I meant that for
some reason (I guess the first implementation wasn't good enough, but
you'll be the one to know the proper one) we store it in our own object
for which we need to parse the data similarly to how SAX works, right?
We register callbacks and we do lot of stuff for each value found.  It
looks like jansson has enough APIs so that virJSON* could be just
trivial wrappers around json_* functions/structures and for example for
parsing we could just call json_load().  I haven't studied it
thoroughly, but that's the feeling I got.

The second thing was a bit of a misunderstanding on my part, we clearly
use it for formatting as well, yes.


 4) The are workarounds for it in the code


I don't recall what these are now ?


So I looked at some options and found few other libraries, I only took
those that are widely available (read: I checked if some random
downstream distro has them), however most of them were not very much
usable.  Except one.  But here's the list of few others that didn't look
that bad either.  All are MIT-licensed, try to be thread-safe and
support pkg-config:

- libfastjson [1] - from rsyslog community, optimized for working with
logs, used almost only by rsyslog, it's supposed to
have doxygen docs, they switched to libfastjson
from json-c due to some problems with it
(performance, ref counting bug, ...)


If its only used by 1 app, I would stay well clear of it, as that's just
one step away from be unused and dead.


- json-c [2] - looked abandoned until I looked at the proper github
   page, "documentation" in doxygen


Yeah, this is the main one I know about and existed way back when I
picked yajl originally.

It has plenty of apps using it in Fedora, which is good.

At first the header files look to have good namespacing, but then I
see horrific things like this polluting the namespaces:

 #undef FALSE
 #define FALSE ((json_bool)0)

 #undef TRUE
 #define TRUE ((json_bool)1)

 #define hexdigit(x) (((x) <= '9') ? (x) - '0' : ((x) & 7) + 9)
 #define error_ptr(error) ((void*)error)
 #define error_description(error)  (json_tokener_errors[error])
 #define is_error(ptr) (ptr == NULL)


- Jansson [3] - I really like this one.  The API seems very intuitive,
it has nice documentation [4] in readthedocs (and I'm
not talking about the visual style, but how easy is to
find information), it can be used for formatting JSON
in a similar way we are doing it.  It has json_auto_t
(optional) type that uses the attribute cleanup for
automatic scope dereference (just in case we want to
use it), it has iterators... did I tell you I like this
one a lot?


Like yajl did, they seem to have had an explicit ABI break at their 2.0
version, which was released in 2011. Not a big deal if we only support
their 2.x series, especially if we keep yajl as an optional fallback

It has 15 open PRs and 32 open bugs, but unlike yajl it actually has
active development taking place

Its header namespace looks clean too.


What do you (others) think of switching the JSON library?  Do you know
about any other projects that could be used considering license,
platform support, etc.?  Also feel free to fix any mistakes I might have
posted.  I double-checked it, but you know, "trust, but verify".


I'd be broadly in favour of switching to something that is not dead :-)

I agree that Jansson appears the most promising.

I think we would have to keep yajl support in parallel, but that's not
too hard, since this is all isolated in virjson.c



Sure, I guess we would just have 2 implementations like we used to have
with threads, selecting particular one at compile-time.


Regards,
Daniel
--
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
ht

Re: [libvirt] Should we switch to a different JSON library?

[Martin Kletzander]

On Tue, Nov 07, 2017 at 01:38:51PM +, Richard W.M. Jones wrote:

On Tue, Nov 07, 2017 at 01:28:00PM +, Daniel P. Berrange wrote:

On Tue, Nov 07, 2017 at 02:05:25PM +0100, Martin Kletzander wrote:
>  4) The are workarounds for it in the code

I don't recall what these are now ?




I shouldn't have said 'workarounds', I meant all the conditioning for
differentiation between yajl 1 and 2 and supporting both version with pkg-config
and without.


libguestfs needs to disable some warnings around yajl code:

 /* GCC can't work out that the YAJL_IS_ test is sufficient to
  * ensure that YAJL_GET_ later doesn't return NULL.
  */
 #if defined(__GNUC__) && __GNUC__ >= 6 /* gcc >= 6 */
 #pragma GCC diagnostic ignored "-Wnull-dereference"
 #endif

However we don't need any other workarounds.

As you say yajl "just works".

It might however be a problem in future if JSON itself was changed,
eg. new types, fix the trailing comma problem, etc.  We'd want a more
responsive upstream to handle that.

- - -

Martin, did you look at the dependencies of these other libraries?
If they depend on other stuff, that could make them harder to use.
Also are they all available in at least Fedora, Debian, OpenSUSE and
Arch/AUR?



Just did, all of these were successful:

docker run -it fedora dnf search jansson
docker run -it opensuse zypper search jansson
docker run -it base/archlinux pacman -Sys jansson
docker run -it ubuntu sh -c "apt update; apt search jansson"
docker run -it debian sh -c "apt update; apt search jansson"

Even:

docker run -it debian:stable sh -c "apt update; apt search jansson"

found version 2.9 which is the second latest one, I believe.


Rich.

--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/2] storage: Resolve storage driver crash

[John Ferlan]

On 11/07/2017 04:18 AM, Cedric Bosdonnat wrote:
> On Mon, 2017-11-06 at 15:53 -0500, John Ferlan wrote:
>> Resolve a storage driver crash as a result of a long running
>> storageVolCreateXML when the virStorageVolPoolRefreshThread is
>> run as a result of a storageVolUpload complete and ran the
>> virStoragePoolObjClearVols without checking if the creation
>> code was currently processing a buildVol after incrementing
>> the driver->asyncjob count.
>>
>> The refreshThread needs to wait until all creation threads
>> are completed so as to not alter the volume list while the
>> volume is being built.
>>
>> Crash from valgrind is as follows (with a bit of editing):
>>
>> ==21309== Invalid read of size 8
>> ==21309==at 0x153E47AF: storageBackendUpdateVolTargetInfo
>> ==21309==by 0x153E4C30: virStorageBackendUpdateVolInfo
>> ==21309==by 0x153E52DE: virStorageBackendVolRefreshLocal
>> ==21309==by 0x153DE29E: storageVolCreateXML
>> ==21309==by 0x562035B: virStorageVolCreateXML
>> ==21309==by 0x147366: remoteDispatchStorageVolCreateXML
>> ...
>> ==21309==  Address 0x2590a720 is 64 bytes inside a block of size 336 free'd
>> ==21309==at 0x4C2F2BB: free
>> ==21309==by 0x54CB9FA: virFree
>> ==21309==by 0x55BC800: virStorageVolDefFree
>> ==21309==by 0x55BF1D8: virStoragePoolObjClearVols
>> ==21309==by 0x153D967E: virStorageVolPoolRefreshThread
>> ...
>> ==21309==  Block was alloc'd at
>> ==21309==at 0x4C300A5: calloc
>> ==21309==by 0x54CB483: virAlloc
>> ==21309==by 0x55BDC1F: virStorageVolDefParseXML
>> ==21309==by 0x55BDC1F: virStorageVolDefParseNode
>> ==21309==by 0x55BE5A4: virStorageVolDefParse
>> ==21309==by 0x153DDFF1: storageVolCreateXML
>> ==21309==by 0x562035B: virStorageVolCreateXML
>> ==21309==by 0x147366: remoteDispatchStorageVolCreateXML
>> ...
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  src/storage/storage_driver.c | 9 +
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
>> index b0edf9f885..5e920fc14c 100644
>> --- a/src/storage/storage_driver.c
>> +++ b/src/storage/storage_driver.c
>> @@ -2257,6 +2257,7 @@ virStorageVolPoolRefreshThread(void *opaque)
>>  virStorageBackendPtr backend;
>>  virObjectEventPtr event = NULL;
>>  
>> + retry:
>>  storageDriverLock();
>>  if (cbdata->vol_path) {
>>  if (virStorageBackendPloopRestoreDesc(cbdata->vol_path) < 0)
>> @@ -2270,6 +2271,14 @@ virStorageVolPoolRefreshThread(void *opaque)
>>  if (!(backend = virStorageBackendForType(def->type)))
>>  goto cleanup;
>>  
>> +/* Some thread is creating a new volume in the pool, we need to retry */
>> +if (virStoragePoolObjGetAsyncjobs(obj) > 0) {
>> +virStoragePoolObjUnlock(obj);
>> +storageDriverUnlock();
>> +usleep(100 * 1000);
>> +goto retry;
>> +}
>> +
>>  virStoragePoolObjClearVols(obj);
>>  if (backend->refreshPool(NULL, obj) < 0)
>>  VIR_DEBUG("Failed to refresh storage pool");
> 
> ACK, does the job here. I'm rather surprised to see you implementing it
> with sleep, while you pointed me towards virCondWait yesterday. But using
> sleep is a way less intrusive change.
> 

Well you only asked about an alternative mechanism and condition
variable was the first thing that popped into my mind; however, as I got
to actually doing more thinking about it - asyncjobs is not blocking
multiple creates from occurring; whereas, a condition variable would be
waiting for one thing to complete.

My response to Jan also lists 2 other alternatives. This was just the
"easiest" of the 3.  If there's other ideas, I'm open to suggestions.

John

> --
> Cedric
> 

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

Re: [libvirt] [PATCH 1/2] storage: Resolve storage driver crash

[John Ferlan]

On 11/07/2017 04:36 AM, Ján Tomko wrote:
> On Mon, Nov 06, 2017 at 03:53:08PM -0500, John Ferlan wrote:
>> Resolve a storage driver crash as a result of a long running
>> storageVolCreateXML when the virStorageVolPoolRefreshThread is
>> run as a result of a storageVolUpload complete and ran the
>> virStoragePoolObjClearVols without checking if the creation
>> code was currently processing a buildVol after incrementing
>> the driver->asyncjob count.
> 
> I thought the whole point of refreshing the pool in a thread
> was to have the storage driver usable for other things.
> This effectively disables pool refresh during long-running
> jobs.
> 

This would "disable" the refresh during a long running create/build job,
but that's already being done as other than another create, not much can
be done while a create/build is "in progress". The pool undefine,
destroy, delete, and refresh calls are all blocked (they fail) until the
create completes.

The reason for virStorageVolPoolRefreshThread is that updateVol can be
long running and once it's done (the stream is closed), it was desired
that the pool sizing values be updated. That meant either refresh the
entire pool (the easy way - theoretically) or update just the volume and
adjust the pool sizing values with the delta from the upload (similar to
how create/build alters the values).

Unfortunately the latter is not possible (yet) because only a subset of
pool types that support uploadVol also support refreshVol. But this
really is only a "problem" for the intersection of those that support
the buildVol too. That is the only reason we'd be blocking is if a pool
type supports both buildVol/buildVolFrom and uploadVol - so the fs,
logical, and vstorage pools.

The virStoragePoolFCRefreshThread used similar type logic w/r/t
refreshing the pool after a createVport completed. Here though, we're
waiting for udev to work it's magic in creating the vHBA LUN. The
asyncjob won't be > 0 for scsi pools yet, so perhaps patch 2 isn't
necessary, but it's preventative.

In any case, at this point in time I would think it's more desirable to
block during the build/upload condition as opposed to crash.

An "alternative" is to refresh the pool after a create vol rather than
just altering the pool available/allocation values from the created
volume target allocation. That way the VolPoolRefreshThread could just
"exit" knowing that volCreate will do the update. However, this refresh
could only happen when there are no asyncjobs outstanding. In the mean
time the pool sizing values probably would still need to be updated if
asyncjobs > 0 to avoid multiple create jobs from "exhausting" the pool
because the sizing values weren't properly updated.

>>
>> The refreshThread needs to wait until all creation threads
>> are completed so as to not alter the volume list while the
>> volume is being built.
>>
>> Crash from valgrind is as follows (with a bit of editing):
>>
>> ==21309== Invalid read of size 8
>> ==21309==    at 0x153E47AF: storageBackendUpdateVolTargetInfo
>> ==21309==    by 0x153E4C30: virStorageBackendUpdateVolInfo
>> ==21309==    by 0x153E52DE: virStorageBackendVolRefreshLocal
>> ==21309==    by 0x153DE29E: storageVolCreateXML
> 
> This is a fault of storageVolCreateXML for using invalid objects
> after dropping the locks.
> 

Partially, but the code also goes through the trouble of incrementing
the pool->asyncjobs count and setting the voldef->building flag for what
appears to be the reasoning that nothing else should modify the pool
volume list while we create/build a new volume, but the refresh thread
just ignores that. The asyncjobs was added in much simpler times in
commit id '2cd9b2d8'.

And this brings up a possible 3rd alternative. Perhaps the more real
problem is that virStoragePoolObjClearVols and refreshPool processing is
overly invasive with respect to free-ing the entire volume list and
rebuilding it rather than perhaps doing some sort of more intelligent
processing to "add" new volumes found in a pool that are not in the
list, "remove" ones no longer in the pool that are in the list, and
"update" those that are still in the list. That to me is outside the
scope of this change, but a noble goal at some point in time.

John

> Jan
> 
>> ==21309==    by 0x562035B: virStorageVolCreateXML
>> ==21309==    by 0x147366: remoteDispatchStorageVolCreateXML
>> ...
>> ==21309==  Address 0x2590a720 is 64 bytes inside a block of size 336
>> free'd
>> ==21309==    at 0x4C2F2BB: free
>> ==21309==    by 0x54CB9FA: virFree
>> ==21309==    by 0x55BC800: virStorageVolDefFree
>> ==21309==    by 0x55BF1D8: virStoragePoolObjClearVols
>> ==21309==    by 0x153D967E: virStorageVolPoolRefreshThread
>> ...
>> ==21309==  Block was alloc'd at
>> ==21309==    at 0x4C300A5: calloc
>> ==21309==    by 0x54CB483: virAlloc
>> ==21309==    by 0x55BDC1F: virStorageVolDefParseXML
>> ==21309==    by 0x55BDC1F: virStorageVolDefParseNode
>> ==21309==    by 0x55BE5A4: virStorageVolDefParse
>> ==21309==    

Re: [libvirt] Should we switch to a different JSON library?

[Richard W.M. Jones]

On Tue, Nov 07, 2017 at 01:28:00PM +, Daniel P. Berrange wrote:
> On Tue, Nov 07, 2017 at 02:05:25PM +0100, Martin Kletzander wrote:
> >  4) The are workarounds for it in the code
> 
> I don't recall what these are now ?

libguestfs needs to disable some warnings around yajl code:

  /* GCC can't work out that the YAJL_IS_ test is sufficient to
   * ensure that YAJL_GET_ later doesn't return NULL.
   */
  #if defined(__GNUC__) && __GNUC__ >= 6 /* gcc >= 6 */
  #pragma GCC diagnostic ignored "-Wnull-dereference"
  #endif

However we don't need any other workarounds.

As you say yajl "just works".

It might however be a problem in future if JSON itself was changed,
eg. new types, fix the trailing comma problem, etc.  We'd want a more
responsive upstream to handle that.

- - -

Martin, did you look at the dependencies of these other libraries?
If they depend on other stuff, that could make them harder to use.
Also are they all available in at least Fedora, Debian, OpenSUSE and
Arch/AUR?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

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

Re: [libvirt] Should we switch to a different JSON library?

[Daniel P. Berrange]

On Tue, Nov 07, 2017 at 02:05:25PM +0100, Martin Kletzander wrote:
> Hi everyone,
> 
> so we are using yajl for parsing JSON.  However there are some reasons
> why we might consider switching to another one:
> 
>  1) It is basically dead upstream

One could say that it is simply "feature complete" upstream so doesn't
need more work, but none the less, I have a general bad feeling about
relying on a library with no active maint work going on for best part
of 4 years, and 68 open pull requests.

>  2) We're just using the lexer part of it
>  3) We only use it for parsing

Aren't 2 & 3 the same point repeated ? Regardless though, we use yajl
for formatting JSON too (virJSONValueToString)

>  4) The are workarounds for it in the code

I don't recall what these are now ?

> So I looked at some options and found few other libraries, I only took
> those that are widely available (read: I checked if some random
> downstream distro has them), however most of them were not very much
> usable.  Except one.  But here's the list of few others that didn't look
> that bad either.  All are MIT-licensed, try to be thread-safe and
> support pkg-config:
> 
> - libfastjson [1] - from rsyslog community, optimized for working with
> logs, used almost only by rsyslog, it's supposed to
> have doxygen docs, they switched to libfastjson
> from json-c due to some problems with it
> (performance, ref counting bug, ...)

If its only used by 1 app, I would stay well clear of it, as that's just
one step away from be unused and dead.

> - json-c [2] - looked abandoned until I looked at the proper github
>page, "documentation" in doxygen

Yeah, this is the main one I know about and existed way back when I
picked yajl originally.

It has plenty of apps using it in Fedora, which is good.

At first the header files look to have good namespacing, but then I
see horrific things like this polluting the namespaces:

  #undef FALSE
  #define FALSE ((json_bool)0)

  #undef TRUE
  #define TRUE ((json_bool)1)

  #define hexdigit(x) (((x) <= '9') ? (x) - '0' : ((x) & 7) + 9)
  #define error_ptr(error) ((void*)error)
  #define error_description(error)  (json_tokener_errors[error])
  #define is_error(ptr) (ptr == NULL)

> - Jansson [3] - I really like this one.  The API seems very intuitive,
> it has nice documentation [4] in readthedocs (and I'm
> not talking about the visual style, but how easy is to
> find information), it can be used for formatting JSON
> in a similar way we are doing it.  It has json_auto_t
> (optional) type that uses the attribute cleanup for
> automatic scope dereference (just in case we want to
> use it), it has iterators... did I tell you I like this
> one a lot?

Like yajl did, they seem to have had an explicit ABI break at their 2.0
version, which was released in 2011. Not a big deal if we only support
their 2.x series, especially if we keep yajl as an optional fallback

It has 15 open PRs and 32 open bugs, but unlike yajl it actually has
active development taking place

Its header namespace looks clean too.

> What do you (others) think of switching the JSON library?  Do you know
> about any other projects that could be used considering license,
> platform support, etc.?  Also feel free to fix any mistakes I might have
> posted.  I double-checked it, but you know, "trust, but verify".

I'd be broadly in favour of switching to something that is not dead :-)

I agree that Jansson appears the most promising.

I think we would have to keep yajl support in parallel, but that's not
too hard, since this is all isolated in virjson.c

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[libvirt] Should we switch to a different JSON library?

[Martin Kletzander]

Hi everyone,

so we are using yajl for parsing JSON.  However there are some reasons
why we might consider switching to another one:

 1) It is basically dead upstream
 2) We're just using the lexer part of it
 3) We only use it for parsing
 4) The are workarounds for it in the code

So I looked at some options and found few other libraries, I only took
those that are widely available (read: I checked if some random
downstream distro has them), however most of them were not very much
usable.  Except one.  But here's the list of few others that didn't look
that bad either.  All are MIT-licensed, try to be thread-safe and
support pkg-config:

- libfastjson [1] - from rsyslog community, optimized for working with
logs, used almost only by rsyslog, it's supposed to
have doxygen docs, they switched to libfastjson
from json-c due to some problems with it
(performance, ref counting bug, ...)

- json-c [2] - looked abandoned until I looked at the proper github
   page, "documentation" in doxygen

- Jansson [3] - I really like this one.  The API seems very intuitive,
it has nice documentation [4] in readthedocs (and I'm
not talking about the visual style, but how easy is to
find information), it can be used for formatting JSON
in a similar way we are doing it.  It has json_auto_t
(optional) type that uses the attribute cleanup for
automatic scope dereference (just in case we want to
use it), it has iterators... did I tell you I like this
one a lot?

What do you (others) think of switching the JSON library?  Do you know
about any other projects that could be used considering license,
platform support, etc.?  Also feel free to fix any mistakes I might have
posted.  I double-checked it, but you know, "trust, but verify".

Have a nice day,
Martin

[1] https://github.com/rsyslog/libfastjson
[2] https://github.com/json-c/json-c
[3] http://www.digip.org/jansson/
[4] https://jansson.readthedocs.io/


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 08/11] tools: Provide bash autompletion file

[Michal Privoznik]

Signed-off-by: Michal Privoznik 
---
 configure.ac   |  3 ++
 libvirt.spec.in|  2 ++
 m4/virt-bash-completion.m4 | 74 ++
 tools/Makefile.am  | 22 --
 tools/bash-completion/vsh  | 73 +
 5 files changed, 172 insertions(+), 2 deletions(-)
 create mode 100644 m4/virt-bash-completion.m4
 create mode 100644 tools/bash-completion/vsh

diff --git a/configure.ac b/configure.ac
index b2d991c3b..9103612bb 100644
--- a/configure.ac
+++ b/configure.ac
@@ -242,6 +242,7 @@ LIBVIRT_ARG_APPARMOR
 LIBVIRT_ARG_ATTR
 LIBVIRT_ARG_AUDIT
 LIBVIRT_ARG_AVAHI
+LIBVIRT_ARG_BASH_COMPLETION
 LIBVIRT_ARG_BLKID
 LIBVIRT_ARG_CAPNG
 LIBVIRT_ARG_CURL
@@ -278,6 +279,7 @@ LIBVIRT_CHECK_ATOMIC
 LIBVIRT_CHECK_ATTR
 LIBVIRT_CHECK_AUDIT
 LIBVIRT_CHECK_AVAHI
+LIBVIRT_CHECK_BASH_COMPLETION
 LIBVIRT_CHECK_BLKID
 LIBVIRT_CHECK_CAPNG
 LIBVIRT_CHECK_CURL
@@ -976,6 +978,7 @@ LIBVIRT_RESULT_APPARMOR
 LIBVIRT_RESULT_ATTR
 LIBVIRT_RESULT_AUDIT
 LIBVIRT_RESULT_AVAHI
+LIBVIRT_RESULT_BASH_COMPLETION
 LIBVIRT_RESULT_BLKID
 LIBVIRT_RESULT_CAPNG
 LIBVIRT_RESULT_CURL
diff --git a/libvirt.spec.in b/libvirt.spec.in
index b00689cab..67bbd128c 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -2043,6 +2043,8 @@ exit 0
 %{_datadir}/systemtap/tapset/libvirt_qemu_probes*.stp
 %{_datadir}/systemtap/tapset/libvirt_functions.stp
 
+%{_datadir}/bash-completion/completions/vsh
+
 
 %if %{with_systemd}
 %{_unitdir}/libvirt-guests.service
diff --git a/m4/virt-bash-completion.m4 b/m4/virt-bash-completion.m4
new file mode 100644
index 0..e1ef58740
--- /dev/null
+++ b/m4/virt-bash-completion.m4
@@ -0,0 +1,74 @@
+dnl Bash completion support
+dnl
+dnl Copyright (C) 2017 Red Hat, Inc.
+dnl
+dnl This library is free software; you can redistribute it and/or
+dnl modify it under the terms of the GNU Lesser General Public
+dnl License as published by the Free Software Foundation; either
+dnl version 2.1 of the License, or (at your option) any later version.
+dnl
+dnl This library is distributed in the hope that it will be useful,
+dnl but WITHOUT ANY WARRANTY; without even the implied warranty of
+dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+dnl Lesser General Public License for more details.
+dnl
+dnl You should have received a copy of the GNU Lesser General Public
+dnl License along with this library.  If not, see
+dnl .
+dnl
+dnl Inspired by libguestfs code.
+dnl
+
+AC_DEFUN([LIBVIRT_ARG_BASH_COMPLETION],[
+  LIBVIRT_ARG_WITH_FEATURE([BASH_COMPLETION], [bash-completion], [check], 
[2.0])
+  LIBVIRT_ARG_WITH([BASH_COMPLETIONS_DIR],
+   [directory containing bash completions scripts],
+   [check])
+])
+
+AC_DEFUN([LIBVIRT_CHECK_BASH_COMPLETION], [
+  AC_REQUIRE([LIBVIRT_CHECK_READLINE])
+
+  if test "x$with_readline" != "xyes" ; then
+if test "x$with_bash_completion" != "xyes" ; then
+  with_bash_completion=no
+else
+  AC_MSG_ERROR([readline is required for bash completion support])
+fi
+  else
+if test "x$with_bash_completion" = "xcheck" ; then
+  with_bash_completion=yes
+fi
+  fi
+
+  LIBVIRT_CHECK_PKG([BASH_COMPLETION], [bash-completion], [2.0])
+
+  if test "x$with_bash_completion" = "xyes" ; then
+if test "x$with_bash_completions_dir" = "xcheck"; then
+  AC_MSG_CHECKING([for bash-completions directory])
+  BASH_COMPLETIONS_DIR="$($PKG_CONFIG --variable=completionsdir 
bash-completion)"
+  AC_MSG_RESULT([$BASH_COMPLETIONS_DIR])
+
+  dnl Replace bash completions's exec_prefix with our own.
+  dnl Note that ${exec_prefix} is kept verbatim at this point in time,
+  dnl and will only be expanded later, when make is called: this makes
+  dnl it possible to override such prefix at compilation or installation
+  dnl time
+  bash_completions_prefix="$($PKG_CONFIG --variable=prefix 
bash-completion)"
+  if test "x$bash_completions_prefix" = "x" ; then
+bash_completions_prefix="/usr"
+  fi
+
+  
BASH_COMPLETIONS_DIR='${exec_prefix}'"${BASH_COMPLETIONS_DIR#$bash_completions_prefix}"
+elif test "x$with_bash_completions_dir" = "xno" || test 
"x$with_bash_completions_dir" = "xyes"; then
+  AC_MSG_ERROR([bash-completions-dir must be used only with valid path])
+else
+  BASH_COMPLETIONS_DIR=$with_bash_completions_dir
+fi
+AC_SUBST([BASH_COMPLETIONS_DIR])
+  fi
+])
+
+AC_DEFUN([LIBVIRT_RESULT_BASH_COMPLETION],[
+  LIBVIRT_RESULT_LIB([BASH_COMPLETION])
+])
diff --git a/tools/Makefile.am b/tools/Makefile.am
index 7513a73ac..34a81e69c 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -66,6 +66,7 @@ EXTRA_DIST = \
libvirt-guests.sysconf \
virt-login-shell.conf \
virsh-edit.c \
+   bash-completion/vsh \
$(PODFILES) \
$(MANINFILES) \
$(NULL)
@@ -332,9 +333,11 @@ POD2MAN = pod2man -c "Virtualization Sup

[libvirt] [PATCH 11/11] virt-admin: Introduce vshAdmServerCompleter

[Michal Privoznik]

Signed-off-by: Michal Privoznik 
---
 tools/Makefile.am|  9 ++
 tools/virt-admin-completer.c | 76 
 tools/virt-admin-completer.h | 33 +++
 tools/virt-admin.c   |  8 +
 4 files changed, 126 insertions(+)
 create mode 100644 tools/virt-admin-completer.c
 create mode 100644 tools/virt-admin-completer.h

diff --git a/tools/Makefile.am b/tools/Makefile.am
index 8eddc7bbe..a1652d459 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -261,6 +261,15 @@ virt_admin_SOURCES = \
virt-admin.c virt-admin.h \
$(NULL)
 
+VIRT_ADMIN_COMPLETER = \
+   virt-admin-completer.c virt-admin-completer.h
+
+if WITH_READLINE
+virt_admin_SOURCES += $(VIRT_ADMIN_COMPLETER)
+else ! WITH_READLINE
+EXTRA_DIST += $(VIRT_ADMIN_COMPLETER)
+endif ! WITH_READLINE
+
 virt_admin_LDFLAGS = \
$(AM_LDFLAGS) \
$(COVERAGE_LDFLAGS) \
diff --git a/tools/virt-admin-completer.c b/tools/virt-admin-completer.c
new file mode 100644
index 0..2cd471f32
--- /dev/null
+++ b/tools/virt-admin-completer.c
@@ -0,0 +1,76 @@
+/*
+ * virt-admin-completer.c: virt-admin completer callbacks
+ *
+ * Copyright (C) 2017 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ *
+ *  Michal Privoznik 
+ *
+ */
+
+#include 
+
+#include "virt-admin-completer.h"
+#include "internal.h"
+#include "virt-admin.h"
+#include "viralloc.h"
+#include "virstring.h"
+
+
+char **
+vshAdmServerCompleter(vshControl *ctl,
+  const vshCmd *cmd ATTRIBUTE_UNUSED,
+  unsigned int flags)
+{
+vshAdmControlPtr priv = ctl->privData;
+virAdmServerPtr *srvs = NULL;
+int nsrvs = 0;
+size_t i = 0;
+char **ret = NULL;
+
+virCheckFlags(0, NULL);
+
+if (!priv->conn || virAdmConnectIsAlive(priv->conn) <= 0)
+return NULL;
+
+/* Obtain a list of available servers on the daemon */
+if ((nsrvs = virAdmConnectListServers(priv->conn, &srvs, 0)) < 0)
+return NULL;
+
+if (VIR_ALLOC_N(ret, nsrvs + 1) < 0)
+goto error;
+
+for (i = 0; i < nsrvs; i++) {
+const char *name = virAdmServerGetName(srvs[i]);
+
+if (VIR_STRDUP(ret[i], name) < 0)
+goto error;
+
+virAdmServerFree(srvs[i]);
+}
+VIR_FREE(srvs);
+
+return ret;
+
+ error:
+for (; i < nsrvs; i++)
+virAdmServerFree(srvs[i]);
+VIR_FREE(srvs);
+for (i = 0; i < nsrvs; i++)
+VIR_FREE(ret[i]);
+VIR_FREE(ret);
+return ret;
+}
diff --git a/tools/virt-admin-completer.h b/tools/virt-admin-completer.h
new file mode 100644
index 0..7507b95c1
--- /dev/null
+++ b/tools/virt-admin-completer.h
@@ -0,0 +1,33 @@
+/*
+ * virt-admin-completer.h: virt-admin completer callbacks
+ *
+ * Copyright (C) 2017 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ *
+ *  Michal Privoznik 
+ *
+ */
+
+#ifndef VIRT_ADMIN_COMPLETER
+# define VIRT_ADMIN_COMPLETER
+
+# include "vsh.h"
+
+char **
+vshAdmServerCompleter(vshControl *ctl,
+  const vshCmd *cmd,
+  unsigned int flags);
+#endif
diff --git a/tools/virt-admin.c b/tools/virt-admin.c
index c24ed95c0..41b945e49 100644
--- a/tools/virt-admin.c
+++ b/tools/virt-admin.c
@@ -39,6 +39,7 @@
 #include "virthread.h"
 #include "virgettext.h"
 #include "virtime.h"
+#include "virt-admin-completer.h"
 
 /* Gnulib doesn't guarantee SA_SIGINFO support.  */
 #ifndef SA_SIGINFO
@@ -432,6 +433,7 @@ static const vshCmdOptDef opts_srv_threadpool_info[] = {
 {.name = "server",
  .type = VSH_OT_DATA,
  .flags = VSH_OFLAG_REQ,
+ .completer = vshAdmServerCompleter,
 

[libvirt] [PATCH 03/11] vsh: Add @silent to vshConnectionHook

[Michal Privoznik]

In near future we will want to not report error when connecting
fails. In order to achieve that we have to pass a boolean that
suppresses error messages.

Signed-off-by: Michal Privoznik 
---
 tools/virsh-domain.c |  2 +-
 tools/virsh.c| 67 
 tools/virsh.h|  5 +++-
 tools/virt-admin.c   | 49 +-
 tools/vsh.c  |  2 +-
 tools/vsh.h  |  3 ++-
 6 files changed, 76 insertions(+), 52 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 42d552637..cf612f73e 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -10931,7 +10931,7 @@ cmdMigrate(vshControl *ctl, const vshCmd *cmd)
 if (vshCommandOptStringReq(ctl, cmd, "desturi", &desturi) < 0)
 goto cleanup;
 
-dconn = virshConnect(ctl, desturi, false);
+dconn = virshConnect(ctl, desturi, false, false);
 if (!dconn)
 goto cleanup;
 
diff --git a/tools/virsh.c b/tools/virsh.c
index d0c135016..7d6dc2620 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -137,7 +137,7 @@ virshCatchDisconnect(virConnectPtr conn,
 /* Main Function which should be used for connecting.
  * This function properly handles keepalive settings. */
 virConnectPtr
-virshConnect(vshControl *ctl, const char *uri, bool readonly)
+virshConnect(vshControl *ctl, const char *uri, bool readonly, bool silent)
 {
 virConnectPtr c = NULL;
 int interval = 5; /* Default */
@@ -191,15 +191,19 @@ virshConnect(vshControl *ctl, const char *uri, bool 
readonly)
 if (interval > 0 &&
 virConnectSetKeepAlive(c, interval, count) != 0) {
 if (keepalive_forced) {
-vshError(ctl, "%s",
- _("Cannot setup keepalive on connection "
-   "as requested, disconnecting"));
+if (!silent) {
+vshError(ctl, "%s",
+ _("Cannot setup keepalive on connection "
+   "as requested, disconnecting"));
+}
 virConnectClose(c);
 c = NULL;
 goto cleanup;
 }
-vshDebug(ctl, VSH_ERR_INFO, "%s",
- _("Failed to setup keepalive on connection\n"));
+if (!silent) {
+vshDebug(ctl, VSH_ERR_INFO, "%s",
+ _("Failed to setup keepalive on connection\n"));
+}
 }
 
  cleanup:
@@ -214,7 +218,11 @@ virshConnect(vshControl *ctl, const char *uri, bool 
readonly)
  *
  */
 static int
-virshReconnect(vshControl *ctl, const char *name, bool readonly, bool force)
+virshReconnect(vshControl *ctl,
+   const char *name,
+   bool readonly,
+   bool force,
+   bool silent)
 {
 bool connected = false;
 virshControlPtr priv = ctl->privData;
@@ -232,20 +240,25 @@ virshReconnect(vshControl *ctl, const char *name, bool 
readonly, bool force)
 
 virConnectUnregisterCloseCallback(priv->conn, virshCatchDisconnect);
 ret = virConnectClose(priv->conn);
-if (ret < 0)
-vshError(ctl, "%s", _("Failed to disconnect from the hypervisor"));
-else if (ret > 0)
-vshError(ctl, "%s", _("One or more references were leaked after "
-  "disconnect from the hypervisor"));
+if (!silent) {
+if (ret < 0)
+vshError(ctl, "%s", _("Failed to disconnect from the 
hypervisor"));
+else if (ret > 0)
+vshError(ctl, "%s", _("One or more references were leaked 
after "
+  "disconnect from the hypervisor"));
+}
 }
 
-priv->conn = virshConnect(ctl, name ? name : ctl->connname, readonly);
+priv->conn = virshConnect(ctl, name ? name : ctl->connname,
+  readonly, silent);
 
 if (!priv->conn) {
-if (disconnected)
-vshError(ctl, "%s", _("Failed to reconnect to the hypervisor"));
-else
-vshError(ctl, "%s", _("failed to connect to the hypervisor"));
+if (!silent) {
+if (disconnected)
+vshError(ctl, "%s", _("Failed to reconnect to the 
hypervisor"));
+else
+vshError(ctl, "%s", _("Failed to connect to the hypervisor"));
+}
 return -1;
 } else {
 if (name) {
@@ -255,8 +268,9 @@ virshReconnect(vshControl *ctl, const char *name, bool 
readonly, bool force)
 }
 if (virConnectRegisterCloseCallback(priv->conn, virshCatchDisconnect,
 ctl, NULL) < 0)
-vshError(ctl, "%s", _("Unable to register disconnect callback"));
-if (connected && !force)
+if (!silent)
+vshError(ctl, "%s", _("Unable to register disconnect 
callback"));
+if (connected && !force && !silent)
 vshError(ctl, "%s", _("Reconnected to the h

[libvirt] [PATCH 02/11] vshCommandOpt: Relax check for valid options

[Michal Privoznik]

When trying to get an opt for command typed on the command line
we first check if command has such option. Because if it doesn't
it is a programming error. For instance: vshCommandOptBool(cmd,
"config") called from say cmdStart() doesn't make sense since
there's no --config for the start command. However, we will want
to have generic completers which are going to check if various
options are set. And so it can happen that we will check for
non-existent option for given command. Therefore, we need to
relax the check otherwise we will hit the assert() and don't get
anywhere.

Signed-off-by: Michal Privoznik 
---
 tools/vsh.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/vsh.c b/tools/vsh.c
index eca312b4b..24ea45aa4 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -815,8 +815,7 @@ vshCommandFree(vshCmd *cmd)
  * Look up an option passed to CMD by NAME.  Returns 1 with *OPT set
  * to the option if found, 0 with *OPT set to NULL if the name is
  * valid and the option is not required, -1 with *OPT set to NULL if
- * the option is required but not present, and assert if NAME is not
- * valid (which indicates a programming error).  No error messages are
+ * the option is required but not present. No error messages are
  * issued if a value is returned.
  */
 static int
@@ -829,8 +828,7 @@ vshCommandOpt(const vshCmd *cmd, const char *name, 
vshCmdOpt **opt,
 
 /* See if option is valid and/or required.  */
 *opt = NULL;
-while (valid) {
-assert(valid->name);
+while (valid && valid->name) {
 if (STREQ(name, valid->name))
 break;
 valid++;
-- 
2.13.6

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

[libvirt] [PATCH 10/11] virsh: Introduce virshDomainInterfaceCompleter

[Michal Privoznik]

Signed-off-by: Michal Privoznik 
---
 tools/virsh-completer.c  | 60 
 tools/virsh-completer.h  |  8 ++
 tools/virsh-domain-monitor.c |  3 +++
 tools/virsh-domain.c |  4 +++
 4 files changed, 75 insertions(+)

diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c
index 4e32b882b..25c3aaa0d 100644
--- a/tools/virsh-completer.c
+++ b/tools/virsh-completer.c
@@ -29,6 +29,7 @@
 #include "internal.h"
 #include "viralloc.h"
 #include "virstring.h"
+#include "virxml.h"
 
 
 char **
@@ -88,3 +89,62 @@ virshDomainNameCompleter(vshControl *ctl,
 VIR_FREE(ret);
 return NULL;
 }
+
+
+char **
+virshDomainInterfaceCompleter(vshControl *ctl,
+  const vshCmd *cmd,
+  unsigned int flags)
+{
+virshControlPtr priv = ctl->privData;
+xmlDocPtr xmldoc = NULL;
+xmlXPathContextPtr ctxt = NULL;
+int ninterfaces;
+xmlNodePtr *interfaces = NULL;
+size_t i;
+unsigned int domainXMLFlags = 0;
+char **ret = NULL;
+
+virCheckFlags(VIRSH_DOMAIN_INTERFACE_COMPLETER_MAC, NULL);
+
+if (!priv->conn || virConnectIsAlive(priv->conn) <= 0)
+return NULL;
+
+if (vshCommandOptBool(cmd, "config"))
+domainXMLFlags = VIR_DOMAIN_XML_INACTIVE;
+
+if (virshDomainGetXML(ctl, cmd, domainXMLFlags, &xmldoc, &ctxt) < 0)
+goto error;
+
+ninterfaces = virXPathNodeSet("./devices/interface", ctxt, &interfaces);
+if (ninterfaces < 0)
+goto error;
+
+if (VIR_ALLOC_N(ret, ninterfaces + 1) < 0)
+goto error;
+
+for (i = 0; i < ninterfaces; i++) {
+ctxt->node = interfaces[i];
+
+if (!(flags & VIRSH_DOMAIN_INTERFACE_COMPLETER_MAC) &&
+(ret[i] = virXPathString("string(./target/@dev)", ctxt)))
+continue;
+
+/* In case we are dealing with inactive domain XML there's no
+ * . Offer MAC addresses then. */
+if (!(ret[i] = virXPathString("string(./mac/@address)", ctxt)))
+goto error;
+}
+
+VIR_FREE(interfaces);
+xmlFreeDoc(xmldoc);
+xmlXPathFreeContext(ctxt);
+return ret;
+
+ error:
+VIR_FREE(interfaces);
+xmlFreeDoc(xmldoc);
+xmlXPathFreeContext(ctxt);
+virStringListFree(ret);
+return NULL;
+}
diff --git a/tools/virsh-completer.h b/tools/virsh-completer.h
index 288e17909..680cd12ff 100644
--- a/tools/virsh-completer.h
+++ b/tools/virsh-completer.h
@@ -30,4 +30,12 @@ char ** virshDomainNameCompleter(vshControl *ctl,
  const vshCmd *cmd,
  unsigned int flags);
 
+enum {
+VIRSH_DOMAIN_INTERFACE_COMPLETER_MAC = 1 << 1, /* Return just MACs */
+};
+
+char ** virshDomainInterfaceCompleter(vshControl *ctl,
+  const vshCmd *cmd,
+  unsigned int flags);
+
 #endif
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index d0edb177d..dd89f0758 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -659,6 +659,7 @@ static const vshCmdOptDef opts_domif_getlink[] = {
 {.name = "interface",
  .type = VSH_OT_DATA,
  .flags = VSH_OFLAG_REQ,
+ .completer = virshDomainInterfaceCompleter,
  .help = N_("interface device (MAC Address)")
 },
 {.name = "persistent",
@@ -995,6 +996,7 @@ static const vshCmdOptDef opts_domifstat[] = {
 {.name = "interface",
  .type = VSH_OT_DATA,
  .flags = VSH_OFLAG_REQ,
+ .completer = virshDomainInterfaceCompleter,
  .help = N_("interface device")
 },
 {.name = NULL}
@@ -2149,6 +2151,7 @@ static const vshCmdOptDef opts_domifaddr[] = {
 {.name = "interface",
  .type = VSH_OT_STRING,
  .flags = VSH_OFLAG_NONE,
+ .completer = virshDomainInterfaceCompleter,
  .help = N_("network interface name")},
 {.name = "full",
  .type = VSH_OT_BOOL,
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 101a5f647..f977832aa 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -2977,6 +2977,7 @@ static const vshCmdOptDef opts_domif_setlink[] = {
 {.name = "interface",
  .type = VSH_OT_DATA,
  .flags = VSH_OFLAG_REQ,
+ .completer = virshDomainInterfaceCompleter,
  .help = N_("interface device (MAC Address)")
 },
 {.name = "state",
@@ -3147,6 +3148,7 @@ static const vshCmdOptDef opts_domiftune[] = {
 {.name = "interface",
  .type = VSH_OT_DATA,
  .flags = VSH_OFLAG_REQ,
+ .completer = virshDomainInterfaceCompleter,
  .help = N_("interface device (MAC Address)")
 },
 {.name = "inbound",
@@ -11983,6 +11985,8 @@ static const vshCmdOptDef opts_detach_interface[] = {
 },
 {.name = "mac",
  .type = VSH_OT_STRING,
+ .completer = virshDomainInterfaceCompleter,
+ .completer_flags = VIRSH_DOMAIN_INTERFACE_COMPLETER_MAC,
  .help = N_("MAC address")
 },
 VIRSH_COMMON_OPT_DOMAIN_PE

[libvirt] [PATCH 00/11] Make auto completion better

[Michal Privoznik]

After initial RFC [1] I had some time to work on this and here is
the result.


What's implemented?
===
Auto completion for both interactive and non-interactive
virsh/virt-admin.


Known limitations
=
Currently, just options completers work. I mean, to bring up list
of domains you have to:

  virsh # start --domain 

Doing bare:

  virsh # start 

brings up list of --options. I am working on this meanwhile. But
one can argue that having to use --option is not that much of a
problem since we have good completion now.

The other known limitation - and actually room for others to join
and help - is missing completers. I mean, the last 3 patches give
an example how to do that. But that is still very few. We need
more completers.


How does this work?
===
The basic idea is simple, when defining opts for command two new
struct members can be set:

 {.name = "mac",
  .type = VSH_OT_STRING,
+ .completer = virshDomainInterfaceCompleter,
+ .completer_flags = VIRSH_DOMAIN_INTERFACE_COMPLETER_MAC,
  .help = N_("MAC address")
 },

@completer is the callback that is used when user wants to bring
up list of possible strings. @completer_flags can then be used to
refine return value of @completer. In this specific example,
virshDomainInterfaceCompleter() brings up list of interfaces for
given domain. It tries to return /interface/target/@dev but if
not found (e.g. because domain is not running),
/interface/mac/@address is returned otherwise. However, in one
specific case we are interested only in MAC addresses (regardless
of domain state) and thus VIRSH_DOMAIN_INTERFACE_COMPLETER_MAC
flag is specified which alters @completer behaviour.

For non-interactive virsh/virt-admin there's
tools/bash-completion/vsh script which does no more than calling:

  $1 complete $USER_INPUT

(where $1 is name of the binary user intents to run).


How to test this?
=

Interactive completion should work out of the box. Just make sure
you're connected. Completers don't connect! You certainly don't
want ssh's 'Password:' prompt show on , do you?
Non-interactive completers do connect, but in order to avoid any
password prompts, /dev/stdin is closed before anything else is
done. In order to test it, just:

  libvirt.git $ source tools/bash-completion/vsh

Now, bash completion should work:

  libvirt.git $ ./tools/virsh -c qemu:///system start --domain 


Notes
=
This is that part of vsh that nobody wants to touch, but with
these patches in we have the framework to just write small
functions (as can be seen in examples). User would benefit from
this!

As usual, you can find all the patches on my github [2].

Michal

1: https://www.redhat.com/archives/libvir-list/2017-October/msg01374.html
2: https://github.com/zippy2/libvirt/tree/bash_autocomplete

Michal Privoznik (11):
  vshCommandStringParse: Allow retrieving partial result
  vshCommandOpt: Relax check for valid options
  vsh: Add @silent to vshConnectionHook
  vsh: Fix vshCompleter signature
  vsh: Call vshCmdOptDef.completer properly
  vshCompleter: Pass partially parsed command
  vsh: Introduce complete command
  tools: Provide bash autompletion file
  virsh: Introduce virshDomainNameCompleter
  virsh: Introduce virshDomainInterfaceCompleter
  virt-admin: Introduce vshAdmServerCompleter

 configure.ac |   3 +
 libvirt.spec.in  |   2 +
 m4/virt-bash-completion.m4   |  74 
 tools/Makefile.am|  40 -
 tools/bash-completion/vsh|  73 
 tools/virsh-completer.c  | 150 +
 tools/virsh-completer.h  |  41 +
 tools/virsh-domain-monitor.c |  33 
 tools/virsh-domain.c | 188 +
 tools/virsh-snapshot.c   |  24 +++---
 tools/virsh.c|  72 +---
 tools/virsh.h|  12 ++-
 tools/virt-admin-completer.c |  76 +
 tools/virt-admin-completer.h |  33 
 tools/virt-admin.c   |  62 --
 tools/vsh.c  | 195 ---
 tools/vsh.h  |  23 -
 17 files changed, 873 insertions(+), 228 deletions(-)
 create mode 100644 m4/virt-bash-completion.m4
 create mode 100644 tools/bash-completion/vsh
 create mode 100644 tools/virsh-completer.c
 create mode 100644 tools/virsh-completer.h
 create mode 100644 tools/virt-admin-completer.c
 create mode 100644 tools/virt-admin-completer.h

-- 
2.13.6

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

[libvirt] [PATCH 06/11] vshCompleter: Pass partially parsed command

[Michal Privoznik]

The callback we're calling might need to make decisions on
already passed arguments. For instance, a completer that is
supposed to bring up list of domain's interfaces might want to
see what domain user wants to work with.

Signed-off-by: Michal Privoznik 
---
 tools/vsh.c | 7 ++-
 tools/vsh.h | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/vsh.c b/tools/vsh.c
index dd2f06ada..121669574 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -2859,9 +2859,14 @@ vshReadlineParse(const char *text, int state)
 }
 
 if (!complete_opts && complete_data) {
-if (!completed_list && opt && opt->completer)
+if (!completed_list && opt && opt->completer) {
+vshCmd *partial = NULL;
+vshCommandStringParse(autoCompleteOpaque, rl_line_buffer, 
&partial);
 completed_list = opt->completer(autoCompleteOpaque,
+partial,
 opt->completer_flags);
+vshCommandFree(partial);
+}
 if (completed_list) {
 while ((completed_name = 
completed_list[completed_list_index])) {
 completed_list_index++;
diff --git a/tools/vsh.h b/tools/vsh.h
index 1ebd5c11a..c5a62e593 100644
--- a/tools/vsh.h
+++ b/tools/vsh.h
@@ -124,6 +124,7 @@ typedef struct _vshCmdOptDef vshCmdOptDef;
 typedef struct _vshControl vshControl;
 
 typedef char **(*vshCompleter)(vshControl *ctl,
+   const vshCmd *cmd,
unsigned int flags);
 
 /*
-- 
2.13.6

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

[libvirt] [PATCH 05/11] vsh: Call vshCmdOptDef.completer properly

[Michal Privoznik]

The idea is that .completer for vshCmdOptDef would be called if
the last token on the input is a cmd opt. For instance:

virsh # start --domain

However, with current code that's not happening.

Signed-off-by: Michal Privoznik 
---
 tools/vsh.c | 45 ++---
 1 file changed, 26 insertions(+), 19 deletions(-)

diff --git a/tools/vsh.c b/tools/vsh.c
index cbab6f7d0..dd2f06ada 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -2687,7 +2687,7 @@ vshReadlineParse(const char *text, int state)
 uint64_t opts_seen;
 size_t opt_index;
 static bool cmd_exists, opts_filled, opt_exists;
-static bool non_bool_opt_exists, data_complete;
+static bool non_bool_opt_exists, complete_data, complete_opts;
 
 if (!state) {
 parser.pos = rl_line_buffer;
@@ -2734,7 +2734,7 @@ vshReadlineParse(const char *text, int state)
 cmd_exists = false;
 opts_filled = false;
 non_bool_opt_exists = false;
-data_complete = false;
+complete_data = false;
 
 const_opts_need_arg = 0;
 const_opts_required = 0;
@@ -2800,7 +2800,7 @@ vshReadlineParse(const char *text, int state)
 }
 if (STREQ(tkdata, sanitized_text)) {
 /* auto-complete non-bool option arg */
-data_complete = true;
+complete_data = true;
 break;
 }
 non_bool_opt_exists = false;
@@ -2847,27 +2847,34 @@ vshReadlineParse(const char *text, int state)
 virSkipSpaces((const char**)&tkdata);
 }
 VIR_FREE(const_tkdata);
+complete_opts = opts_filled && !non_bool_opt_exists;
 }
 
 if (!cmd_exists) {
 res = vshReadlineCommandGenerator(sanitized_text, state);
-} else if (opts_filled && !non_bool_opt_exists) {
-res = vshReadlineOptionsGenerator(sanitized_text, state, cmd);
-} else if (non_bool_opt_exists && data_complete && opt && opt->completer) {
-if (!completed_list)
-completed_list = opt->completer(autoCompleteOpaque,
-opt->completer_flags);
-if (completed_list) {
-while ((completed_name = completed_list[completed_list_index])) {
-completed_list_index++;
-if (!STRPREFIX(completed_name, sanitized_text))
-continue;
-res = vshStrdup(NULL, completed_name);
-return res;
+} else {
+if (complete_opts) {
+res = vshReadlineOptionsGenerator(sanitized_text, state, cmd);
+complete_opts = !!res;
+}
+
+if (!complete_opts && complete_data) {
+if (!completed_list && opt && opt->completer)
+completed_list = opt->completer(autoCompleteOpaque,
+opt->completer_flags);
+if (completed_list) {
+while ((completed_name = 
completed_list[completed_list_index])) {
+completed_list_index++;
+if (!STRPREFIX(completed_name, sanitized_text))
+continue;
+res = vshStrdup(NULL, completed_name);
+return res;
+}
+res = NULL;
+virStringListFree(completed_list);
+completed_list = NULL;
+completed_list_index = 0;
 }
-res = NULL;
-virStringListFree(completed_list);
-completed_list_index = 0;
 }
 }
 
-- 
2.13.6

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

[libvirt] [PATCH 04/11] vsh: Fix vshCompleter signature

[Michal Privoznik]

The first argument passed to this function is vshControl *.
There's no need to use void pointer.

Signed-off-by: Michal Privoznik 
---
 tools/vsh.c | 2 +-
 tools/vsh.h | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/vsh.c b/tools/vsh.c
index 83c96e1a8..cbab6f7d0 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -66,7 +66,7 @@
 
 #ifdef WITH_READLINE
 /* For autocompletion */
-void *autoCompleteOpaque;
+vshControl *autoCompleteOpaque;
 #endif
 
 /* NOTE: It would be much nicer to have these two as part of vshControl
diff --git a/tools/vsh.h b/tools/vsh.h
index c411c2ca4..1ebd5c11a 100644
--- a/tools/vsh.h
+++ b/tools/vsh.h
@@ -123,7 +123,8 @@ typedef struct _vshCmdOpt vshCmdOpt;
 typedef struct _vshCmdOptDef vshCmdOptDef;
 typedef struct _vshControl vshControl;
 
-typedef char **(*vshCompleter)(void *opaque, unsigned int flags);
+typedef char **(*vshCompleter)(vshControl *ctl,
+   unsigned int flags);
 
 /*
  * vshCmdInfo -- name/value pair for information about command
-- 
2.13.6

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

[libvirt] [PATCH 09/11] virsh: Introduce virshDomainNameCompleter

[Michal Privoznik]

Signed-off-by: Michal Privoznik 
---
 tools/Makefile.am|   9 +++
 tools/virsh-completer.c  |  90 +
 tools/virsh-completer.h  |  33 
 tools/virsh-domain-monitor.c |  30 +++
 tools/virsh-domain.c | 182 ++-
 tools/virsh-snapshot.c   |  24 +++---
 tools/virsh.h|   7 +-
 7 files changed, 256 insertions(+), 119 deletions(-)
 create mode 100644 tools/virsh-completer.c
 create mode 100644 tools/virsh-completer.h

diff --git a/tools/Makefile.am b/tools/Makefile.am
index 34a81e69c..8eddc7bbe 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -231,6 +231,15 @@ virsh_SOURCES = \
virsh-volume.c virsh-volume.h \
$(NULL)
 
+VIRSH_COMPLETER = \
+   virsh-completer.c virsh-completer.h
+
+if WITH_READLINE
+virsh_SOURCES += $(VIRSH_COMPLETER)
+else ! WITH_READLINE
+EXTRA_DIST += $(VIRSH_COMPLETER)
+endif ! WITH_READLINE
+
 virsh_LDFLAGS = \
$(AM_LDFLAGS) \
$(PIE_LDFLAGS) \
diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c
new file mode 100644
index 0..4e32b882b
--- /dev/null
+++ b/tools/virsh-completer.c
@@ -0,0 +1,90 @@
+/*
+ * virsh-completer.c: virsh completer callbacks
+ *
+ * Copyright (C) 2017 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ *
+ *  Michal Privoznik 
+ *
+ */
+
+#include 
+
+#include "virsh-completer.h"
+#include "virsh.h"
+#include "virsh-util.h"
+#include "internal.h"
+#include "viralloc.h"
+#include "virstring.h"
+
+
+char **
+virshDomainNameCompleter(vshControl *ctl,
+ const vshCmd *cmd ATTRIBUTE_UNUSED,
+ unsigned int flags)
+{
+virshControlPtr priv = ctl->privData;
+virDomainPtr *domains = NULL;
+int ndomains = 0;
+size_t i = 0;
+char **ret = NULL;
+
+virCheckFlags(VIR_CONNECT_LIST_DOMAINS_ACTIVE |
+  VIR_CONNECT_LIST_DOMAINS_INACTIVE |
+  VIR_CONNECT_LIST_DOMAINS_PERSISTENT |
+  VIR_CONNECT_LIST_DOMAINS_TRANSIENT |
+  VIR_CONNECT_LIST_DOMAINS_RUNNING |
+  VIR_CONNECT_LIST_DOMAINS_PAUSED |
+  VIR_CONNECT_LIST_DOMAINS_SHUTOFF |
+  VIR_CONNECT_LIST_DOMAINS_OTHER |
+  VIR_CONNECT_LIST_DOMAINS_MANAGEDSAVE |
+  VIR_CONNECT_LIST_DOMAINS_NO_MANAGEDSAVE |
+  VIR_CONNECT_LIST_DOMAINS_AUTOSTART |
+  VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART |
+  VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT |
+  VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT,
+  NULL);
+
+if (!priv->conn || virConnectIsAlive(priv->conn) <= 0)
+return NULL;
+
+if ((ndomains = virConnectListAllDomains(priv->conn, &domains, flags)) < 0)
+return NULL;
+
+if (VIR_ALLOC_N(ret, ndomains + 1) < 0)
+goto error;
+
+for (i = 0; i < ndomains; i++) {
+const char *name = virDomainGetName(domains[i]);
+
+if (VIR_STRDUP(ret[i], name) < 0)
+goto error;
+
+virshDomainFree(domains[i]);
+}
+VIR_FREE(domains);
+
+return ret;
+ error:
+
+for (; i < ndomains; i++)
+virshDomainFree(domains[i]);
+VIR_FREE(domains);
+for (i = 0; i < ndomains; i++)
+VIR_FREE(ret[i]);
+VIR_FREE(ret);
+return NULL;
+}
diff --git a/tools/virsh-completer.h b/tools/virsh-completer.h
new file mode 100644
index 0..288e17909
--- /dev/null
+++ b/tools/virsh-completer.h
@@ -0,0 +1,33 @@
+/*
+ * virsh-completer.h: virsh completer callbacks
+ *
+ * Copyright (C) 2017 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * 

[libvirt] [PATCH 01/11] vshCommandStringParse: Allow retrieving partial result

[Michal Privoznik]

In the future, this function is going to be called from
vshReadlineParse() to provide parsed input for completer
callbacks. The idea is to allow the callbacks to provide more
specific data. For instance, for the following input:

  virsh # domifaddr --domain fedora --interface 

the --interface completer callback is going to be called. Now, it
is more user friendly if the completer offers only those
interfaces found in 'fedora' domain. But in order to do that it
needs to be able to retrieve partially parsed result.

Signed-off-by: Michal Privoznik 
---
 tools/virsh.c  |  4 ++--
 tools/virt-admin.c |  4 ++--
 tools/vsh.c| 67 ++
 tools/vsh.h|  2 +-
 4 files changed, 47 insertions(+), 30 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c
index d1789f03a..d0c135016 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -815,7 +815,7 @@ virshParseArgv(vshControl *ctl, int argc, char **argv)
 ctl->imode = false;
 if (argc - optind == 1) {
 vshDebug(ctl, VSH_ERR_INFO, "commands: \"%s\"\n", argv[optind]);
-return vshCommandStringParse(ctl, argv[optind]);
+return vshCommandStringParse(ctl, argv[optind], NULL);
 } else {
 return vshCommandArgvParse(ctl, argc - optind, argv + optind);
 }
@@ -952,7 +952,7 @@ main(int argc, char **argv)
 #if WITH_READLINE
 add_history(ctl->cmdstr);
 #endif
-if (vshCommandStringParse(ctl, ctl->cmdstr))
+if (vshCommandStringParse(ctl, ctl->cmdstr, NULL))
 vshCommandRun(ctl, ctl->cmd);
 }
 VIR_FREE(ctl->cmdstr);
diff --git a/tools/virt-admin.c b/tools/virt-admin.c
index e529a2891..b8b33af19 100644
--- a/tools/virt-admin.c
+++ b/tools/virt-admin.c
@@ -1335,7 +1335,7 @@ vshAdmParseArgv(vshControl *ctl, int argc, char **argv)
 ctl->imode = false;
 if (argc - optind == 1) {
 vshDebug(ctl, VSH_ERR_INFO, "commands: \"%s\"\n", argv[optind]);
-return vshCommandStringParse(ctl, argv[optind]);
+return vshCommandStringParse(ctl, argv[optind], NULL);
 } else {
 return vshCommandArgvParse(ctl, argc - optind, argv + optind);
 }
@@ -1555,7 +1555,7 @@ main(int argc, char **argv)
 #if WITH_READLINE
 add_history(ctl->cmdstr);
 #endif
-if (vshCommandStringParse(ctl, ctl->cmdstr))
+if (vshCommandStringParse(ctl, ctl->cmdstr, NULL))
 vshCommandRun(ctl, ctl->cmd);
 }
 VIR_FREE(ctl->cmdstr);
diff --git a/tools/vsh.c b/tools/vsh.c
index 10a65c39f..eca312b4b 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -1387,26 +1387,27 @@ struct _vshCommandParser {
 };
 
 static bool
-vshCommandParse(vshControl *ctl, vshCommandParser *parser)
+vshCommandParse(vshControl *ctl, vshCommandParser *parser, vshCmd **partial)
 {
 char *tkdata = NULL;
 vshCmd *clast = NULL;
 vshCmdOpt *first = NULL;
+const vshCmdDef *cmd = NULL;
 
-if (ctl->cmd) {
+if (!partial && ctl->cmd) {
 vshCommandFree(ctl->cmd);
 ctl->cmd = NULL;
 }
 
 while (1) {
 vshCmdOpt *last = NULL;
-const vshCmdDef *cmd = NULL;
 vshCommandToken tk;
 bool data_only = false;
 uint64_t opts_need_arg = 0;
 uint64_t opts_required = 0;
 uint64_t opts_seen = 0;
 
+cmd = NULL;
 first = NULL;
 
 while (1) {
@@ -1425,7 +1426,8 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser)
 if (cmd == NULL) {
 /* first token must be command name */
 if (!(cmd = vshCmddefSearch(tkdata))) {
-vshError(ctl, _("unknown command: '%s'"), tkdata);
+if (!partial)
+vshError(ctl, _("unknown command: '%s'"), tkdata);
 goto syntaxError;   /* ... or ignore this command only? */
 }
 
@@ -1437,9 +1439,10 @@ vshCommandParse(vshControl *ctl, vshCommandParser 
*parser)
 }
 if (vshCmddefOptParse(cmd, &opts_need_arg,
   &opts_required) < 0) {
-vshError(ctl,
- _("internal error: bad options in command: '%s'"),
- tkdata);
+if (!partial)
+vshError(ctl,
+ _("internal error: bad options in command: 
'%s'"),
+ tkdata);
 goto syntaxError;
 }
 VIR_FREE(tkdata);
@@ -1474,11 +1477,12 @@ vshCommandParse(vshControl *ctl, vshCommandParser 
*parser)
 if (tk == VSH_TK_ERROR)
 goto syntaxError;
 if (tk != VSH_TK_ARG) {
-vshError(ctl,
- 

[libvirt] [PATCH 07/11] vsh: Introduce complete command

[Michal Privoznik]

This command is going to be called from bash completion script in
the following form:

  virsh complete "start --domain"

Its only purpose is to return list of possible strings for
completion. Note that this is a 'hidden', unlisted command and
therefore there's no documentation to it.

Signed-off-by: Michal Privoznik 
---
 tools/virsh.c  |  1 +
 tools/virt-admin.c |  1 +
 tools/vsh.c| 68 ++
 tools/vsh.h| 14 +++
 4 files changed, 84 insertions(+)

diff --git a/tools/virsh.c b/tools/virsh.c
index 7d6dc2620..f830331f6 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -846,6 +846,7 @@ static const vshCmdDef virshCmds[] = {
 VSH_CMD_PWD,
 VSH_CMD_QUIT,
 VSH_CMD_SELF_TEST,
+VSH_CMD_COMPLETE,
 {.name = "connect",
  .handler = cmdConnect,
  .opts = opts_connect,
diff --git a/tools/virt-admin.c b/tools/virt-admin.c
index 5d7ef7988..c24ed95c0 100644
--- a/tools/virt-admin.c
+++ b/tools/virt-admin.c
@@ -1356,6 +1356,7 @@ static const vshCmdDef vshAdmCmds[] = {
 VSH_CMD_PWD,
 VSH_CMD_QUIT,
 VSH_CMD_SELF_TEST,
+VSH_CMD_COMPLETE,
 {.name = "uri",
  .handler = cmdURI,
  .opts = NULL,
diff --git a/tools/vsh.c b/tools/vsh.c
index 121669574..136acb0ab 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -3419,3 +3419,71 @@ cmdSelfTest(vshControl *ctl ATTRIBUTE_UNUSED,
 
 return true;
 }
+
+/* --
+ * Autocompletion command
+ * -- */
+
+const vshCmdOptDef opts_complete[] = {
+{.name = "string",
+ .type = VSH_OT_ARGV,
+ .flags = VSH_OFLAG_EMPTY_OK,
+ .help = N_("partial string to autocomplete")
+},
+{.name = NULL}
+};
+
+const vshCmdInfo info_complete[] = {
+{.name = "help",
+ .data = N_("internal command for autocompletion")
+},
+{.name = "desc",
+ .data = N_("internal use only")
+},
+{.name = NULL}
+};
+
+bool
+cmdComplete(vshControl *ctl, const vshCmd *cmd)
+{
+bool ret = false;
+#ifdef WITH_READLINE
+const vshClientHooks *hooks = ctl->hooks;
+int stdin_fileno = STDIN_FILENO;
+const char *arg = NULL;
+char **matches = NULL, *tmp = NULL, **iter;
+
+if (vshCommandOptStringQuiet(ctl, cmd, "string", &arg) <= 0)
+goto cleanup;
+
+/* This command is flagged VSH_CMD_FLAG_NOCONNECT because we
+ * need to prevent auth hooks reading any input. Therefore we
+ * have to close stdin and then connect ourselves. */
+VIR_FORCE_CLOSE(stdin_fileno);
+
+if (!(hooks && hooks->connHandler && hooks->connHandler(ctl, true)))
+goto cleanup;
+
+autoCompleteOpaque = ctl;
+
+rl_basic_word_break_characters = " \t\n\\`@$><=;|&{(";
+if (VIR_STRDUP(rl_line_buffer, arg) < 0)
+goto cleanup;
+
+while ((tmp = strpbrk(arg, rl_basic_word_break_characters)))
+arg = tmp + 1;
+
+if (!(matches = vshReadlineCompletion(arg, 0, 0)))
+goto cleanup;
+
+for (iter = matches; *iter; iter++)
+printf("%s\n", *iter);
+
+ret = true;
+ cleanup:
+for (iter = matches; iter && *iter; iter++)
+VIR_FREE(*iter);
+VIR_FREE(matches);
+#endif /* WITH_READLINE */
+return ret;
+}
diff --git a/tools/vsh.h b/tools/vsh.h
index c5a62e593..f55ba86f0 100644
--- a/tools/vsh.h
+++ b/tools/vsh.h
@@ -382,6 +382,8 @@ extern const vshCmdInfo info_echo[];
 extern const vshCmdInfo info_pwd[];
 extern const vshCmdInfo info_quit[];
 extern const vshCmdInfo info_selftest[];
+extern const vshCmdOptDef opts_complete[];
+extern const vshCmdInfo info_complete[];
 
 bool cmdHelp(vshControl *ctl, const vshCmd *cmd);
 bool cmdCd(vshControl *ctl, const vshCmd *cmd);
@@ -389,6 +391,7 @@ bool cmdEcho(vshControl *ctl, const vshCmd *cmd);
 bool cmdPwd(vshControl *ctl, const vshCmd *cmd);
 bool cmdQuit(vshControl *ctl, const vshCmd *cmd);
 bool cmdSelfTest(vshControl *ctl, const vshCmd *cmd);
+bool cmdComplete(vshControl *ctl, const vshCmd *cmd);
 
 # define VSH_CMD_CD \
 { \
@@ -454,6 +457,17 @@ bool cmdSelfTest(vshControl *ctl, const vshCmd *cmd);
 .alias = "self-test" \
 }
 
+# define VSH_CMD_COMPLETE \
+{ \
+.name = "complete", \
+.handler = cmdComplete, \
+.opts = opts_complete, \
+.info = info_complete, \
+.flags = VSH_CMD_FLAG_NOCONNECT | VSH_CMD_FLAG_ALIAS, \
+.alias = "complete" \
+}
+
+
 
 /* readline */
 char * vshReadline(vshControl *ctl, const char *prompt);
-- 
2.13.6

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

Re: [libvirt] Libvirt config converter can't handle file not ending with new line

[Wei Liu]

On Mon, Nov 06, 2017 at 09:41:01PM -0700, Jim Fehlig wrote:
> On 10/30/2017 06:17 AM, Wei Liu wrote:
> > Hi Jim
> > 
> > I discover a problem when using xen_xl converter. When the file in
> > question doesn't end with a new line, I get the following error:
> > 
> >error: configuration file syntax error: memory conf:53: expecting a value
> 
> I'm not able to reproduce this issue. The libvirt.git tree I tried was a bit
> dated, but even after updating to latest master I can't reproduce.
> 
> > After digging a bit (but haven't read libvirt code), it appears that the
> > file didn't end with a new line.
> 
> I tried several files without ending new lines, going both directions
> (domxml-to-native and domxml-from-native), but didn't see the mentioned
> error. Perhaps your config is revealing another bug which is being
> improperly reported. Can you provide an example of the problematic config?
> 

I tried to get the exact file that caused the problem but it is already
destroyed by osstest.

A similar file:

http://logs.test-lab.xenproject.org/osstest/logs/115436/test-amd64-amd64-libvirt-pair/debian.guest.osstest.cfg

If you hexdump -C it, you can see the last character is 0a. Remove it and
feed the file into the converter.

Wei.

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

Re: [libvirt] [PATCH REPOST 0/8] Privatize _virStoragePoolObj and _virStorageVolDefList (Batch #3)

[Erik Skultety]

On Fri, Oct 06, 2017 at 10:42:54AM -0400, John Ferlan wrote:
> Since the original series (19 patches):
>
> https://www.redhat.com/archives/libvir-list/2017-September/msg00594.html
>
> didn't garner any attention, I'm going with smaller patch piles to make
> forward progress.
>
> This is the last half of the storage backends plus one missed merge
> (because I broke things up )
>
> John Ferlan (8):
>   storage: Use virStoragePoolObjGetDef accessor for iSCSI backend
>   storage: Use virStoragePoolObjGetDef accessor for MPATH backend
>   storage: Use virStoragePoolObjGetDef accessor for RBD backend
>   storage: Use virStoragePoolObjGetDef accessor for SCSI backend
>   storage: Use virStoragePoolObjGetDef accessor for VSTORAGE backend
>   storage: Use virStoragePoolObjGetDef accessor for ZFS backend
>   storage: Use virStoragePoolObjGetDef accessor for new driver events
>   storage: Privatize virStoragePoolObj and virStorageVolDefList
>
>  src/conf/storage_conf.h|  4 ---
>  src/conf/virstorageobj.c   | 20 +++
>  src/conf/virstorageobj.h   | 15 
>  src/storage/storage_backend_iscsi.c| 41 --
>  src/storage/storage_backend_mpath.c|  8 +++--
>  src/storage/storage_backend_rbd.c  | 64 
> ++
>  src/storage/storage_backend_scsi.c | 30 +---
>  src/storage/storage_backend_vstorage.c | 31 
>  src/storage/storage_backend_zfs.c  | 39 -
>  src/storage/storage_driver.c   |  8 ++---
>  10 files changed, 144 insertions(+), 116 deletions(-)

Reviewed-by: Erik Skultety 

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

Re: [libvirt] [PATCH 0/7] x86: Rework KVM-defaults compat code, enable kvm_pv_unhalt by default

[Paolo Bonzini]

On 14/10/2017 01:56, Eduardo Habkost wrote:
> Now, I don't know yet what's the best default for a guest that
> has CONFIG_PARAVIRT_SPINLOCK when it sees a host that supports
> kvm_pv_unhalt.  But I'm arguing that it's the guest
> responsibility to choose what to do when it detects such a host,
> instead of expecting the host to hide features from the guest.
> The guest and the guest administrator have more information to
> choose what's best.
> 
> In other words, if exposing kvm_pv_unhalt on CPUID really makes
> some guests behave poorly, can we fix the guests instead?

Waiman just did it. :)

https://marc.info/?l=linux-kernel&m=150972337909996

Paolo

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

Re: [libvirt] [PATCH v5 3/3] libvirtd: fix crash on termination

[Martin Kletzander]

On Mon, Nov 06, 2017 at 03:20:13PM -0500, John Ferlan wrote:



On 11/03/2017 05:20 AM, Nikolay Shirokovskiy wrote:



On 03.11.2017 11:42, Martin Kletzander wrote:

On Fri, Nov 03, 2017 at 11:07:36AM +0300, Nikolay Shirokovskiy wrote:

On 02.11.2017 19:32, Martin Kletzander wrote:

This just makes the window of opportunity (between daemonServerClose()
and the actual removal of the virNetServerPtr from the hash) smaller.
That's why I don't see it as a fix, rather as a workaround.

What I think should be done is the order of what's done with services,
servers, drivers, daemon, workers, etc.

I read the other threds and I think we're on the same note here, it's
just that there is lot of confusion and we're all approaching it
differently.


Trying to catch up, but this thread has gone beyond where I was
previously. I still have to push the first two patches of this series...

Yes, libvirtd cleanup path is awful and there's quite a few "issues"
that have cropped up because previous patches didn't take care of
refcnt'ing properly and just "added" cleanup "somewhere" in the list of
things to be cleaned up... Fortunately to a degree I think this is still
all edge condition type stuff, but getting it right would be good...

For me theory has always been to cleanup in the inverse order that
something was created - for sure libvirtd doesn't follow that at all.
For libvirtd the shutdown/cleanup logic just seemed too haphazard and
fixing never made it to the top of the list of things to think about.

Another "caveat" in libvirtd cleanup logic is that driversInitialized is
set as part of a thread that could be running or have run if the code
enters the cleanup: path as a result of virNetlinkEventServiceStart
failure before we enter the event loop (e.g. virNetDaemonRun).

Still like Erik - I do like Martin's suggestion and would like to see it
as a formal patch - although perhaps with a few tweaks.

IIRC, this particular patch "worked" because by removing the
dmn->servers refcnt earlier made sure that we didn't have to wait for
virObjectUnref(dmn) to occur after virStateCleanup.



Would it be to daring to ask you guys to send one series that includes all the
related patchsets?  We can then discuss there, it would be easier to follow as
well.  I don't want anyone to do any extra work, but we might save some review
bandwidth by resending those patches in one series.  Or maybe it's just me and
I have a hard time keeping up with everything at the same time =)



So first let's agree on what should be done when virNetDaemonClose() is
called and the loop in virNetDaemonRun() exits.  My opinion it is:

1) Stop accepting new calls and services:
   virNetServerServiceToggle(false);


This not really necessary as this has effect only if loop is running.



Sure, good point.



2) Wait for all the workers to finish:
   First part of virThreadPoolFree()


I was thinking of splitting this function to finish/free too.
After finish it is not really usable because there are no
more threads in pool anymore so we have to introduce state
finished and check it in every thread pool api function to
be safe. So this will introduce a bit of complexity only
for the sake of some scheme IMO.



Yeah, it's similar to the daemon when you are between close and dispose, but for
the daemon we need it.

What function do you think we would need to check this for?  The finish would
end all the threads and since the event loop is not running no new jobs could be
posted.


virThreadPoolSendJob is already good in this respect as it checks @quit at the 
beginning.
Looks like we need only to add same check to virThreadPoolSetParameters. So
not so much complexity.

I can imagine thread pool function being called from another
thread pools thread. AFAIK there are not such situation though. So just to be 
on a safe side.





3) Kill off clients:
   virNetServerClientClose()

4) Clean up drivers:
   virStateCleanup()


I think it is better for net daemon/servers not to know about
hypervisor drivers directly.



Sure, I was only talking about the libvirtd for the sake of simplicity.



5) Clean up services, servers, daemon, etc.
   including the second part of virThreadPoolFree()

The last thing is what should be done in virNetDaemonDispose(), but
unfortunately we (mis)use it for more than that.  Numbers 1-4 should be,
IMO in virNet{Daemon,Server}Close().

Would this solve your problem?  I mean I'm not against more reference
counting, it should be done properly, but I think the above would do the
trick and also make sense from the naming point of view.

Since both the daemon and server will probably not ever be consistent
after calling the Close function, it might make sense to just include
virNetDaemonClose() into virNetDaemonDispose() and I'm not against that.
But the order of the steps should still be as described above IMO.


This won't work IMO. If we are already in virNetDaemonDispose then
@dmn object is invalid but we don't yet call virStateCleanup yet
(i

Re: [libvirt] [PATCH 1/2] storage: Resolve storage driver crash

[Ján Tomko]

On Mon, Nov 06, 2017 at 03:53:08PM -0500, John Ferlan wrote:

Resolve a storage driver crash as a result of a long running
storageVolCreateXML when the virStorageVolPoolRefreshThread is
run as a result of a storageVolUpload complete and ran the
virStoragePoolObjClearVols without checking if the creation
code was currently processing a buildVol after incrementing
the driver->asyncjob count.


I thought the whole point of refreshing the pool in a thread
was to have the storage driver usable for other things.
This effectively disables pool refresh during long-running
jobs.



The refreshThread needs to wait until all creation threads
are completed so as to not alter the volume list while the
volume is being built.

Crash from valgrind is as follows (with a bit of editing):

==21309== Invalid read of size 8
==21309==at 0x153E47AF: storageBackendUpdateVolTargetInfo
==21309==by 0x153E4C30: virStorageBackendUpdateVolInfo
==21309==by 0x153E52DE: virStorageBackendVolRefreshLocal
==21309==by 0x153DE29E: storageVolCreateXML


This is a fault of storageVolCreateXML for using invalid objects
after dropping the locks.

Jan


==21309==by 0x562035B: virStorageVolCreateXML
==21309==by 0x147366: remoteDispatchStorageVolCreateXML
...
==21309==  Address 0x2590a720 is 64 bytes inside a block of size 336 free'd
==21309==at 0x4C2F2BB: free
==21309==by 0x54CB9FA: virFree
==21309==by 0x55BC800: virStorageVolDefFree
==21309==by 0x55BF1D8: virStoragePoolObjClearVols
==21309==by 0x153D967E: virStorageVolPoolRefreshThread
...
==21309==  Block was alloc'd at
==21309==at 0x4C300A5: calloc
==21309==by 0x54CB483: virAlloc
==21309==by 0x55BDC1F: virStorageVolDefParseXML
==21309==by 0x55BDC1F: virStorageVolDefParseNode
==21309==by 0x55BE5A4: virStorageVolDefParse
==21309==by 0x153DDFF1: storageVolCreateXML
==21309==by 0x562035B: virStorageVolCreateXML
==21309==by 0x147366: remoteDispatchStorageVolCreateXML
...

Signed-off-by: John Ferlan 
---
src/storage/storage_driver.c | 9 +
1 file changed, 9 insertions(+)


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2] vz: allow to start vz driver without host cache info

[Martin Kletzander]

On Tue, Nov 07, 2017 at 12:02:05PM +0300, Mikhail Feoktistov wrote:


On 11.07.2017 15:55, Martin Kletzander wrote:

On Tue, Jul 11, 2017 at 03:20:38PM +0300, Mikhail Feoktistov wrote:


On 11.07.2017 12:22, Martin Kletzander wrote:

On Tue, Jul 11, 2017 at 04:59:05AM -0400, Mikhail Feoktistov wrote:

Show warning message instead of fail operation.
It happens if kernel or cpu doesn't support reporting cpu cache info.
In case of Virtuozzo file "id" doesn't exist.


What is your kernel version?


 3.10.0-514.16.1.vz7.30.10

Is there another way of getting the information about the cache ID?
Maybe we need to parse the name of the cache directory 'index2'
would be
id 2 maybe?  If there is no other way, then this fix is fine (as most
drivers do the same thing), but I would rather fix it if that's
possible.  Unfortunately the cache information is structured stupidly
compared to other kernel-provided topology-related information.

Only kernel 4.11 reports cache ID (maybe 4.10, but not checked)
4.9 doesn't report ID


Oh, so that is just older kernel.  OK, then.

Reviewed-by: Martin Kletzander 


ID doesn't match to indexN



Yeah, in vanilla it is number per level and easily deductible, but not
really something we want to add into the code.

Martin, do you agree with this patch?



As I said, if there is no other way, then just not populating the info is
perfectly fine, we don't need to error out.

Have a nice day,
Martin

signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/2] scsi: Check for long running create in FCRefreshThread

[Cedric Bosdonnat]

On Mon, 2017-11-06 at 15:53 -0500, John Ferlan wrote:
> Similar to a recent patch in virStorageVolPoolRefreshThread to ensure
> that there were no pool AsyncJobs (e.g. nothing being created at the
> time in a long running buildVol job), modify virStoragePoolFCRefreshThread
> to check for async jobs before calling virStoragePoolObjClearVols and
> refreshing the volumes defined in the pool.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/storage/storage_backend_scsi.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/storage/storage_backend_scsi.c 
> b/src/storage/storage_backend_scsi.c
> index 02fd4b643c..63a9154102 100644
> --- a/src/storage/storage_backend_scsi.c
> +++ b/src/storage/storage_backend_scsi.c
> @@ -159,6 +159,7 @@ virStoragePoolFCRefreshThread(void *opaque)
>  pool->def->allocation = pool->def->capacity = pool->def->available = 
> 0;
>  
>  if (virStoragePoolObjIsActive(pool) &&
> +virStoragePoolObjGetAsyncjobs(pool) == 0 &&
>  virSCSIHostGetNumber(fchost_name, &host) == 0 &&
>  virStorageBackendSCSITriggerRescan(host) == 0) {
>  virStoragePoolObjClearVols(pool);

ACK
--
Cedric

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

Re: [libvirt] [PATCH 1/2] storage: Resolve storage driver crash

[Cedric Bosdonnat]

On Mon, 2017-11-06 at 15:53 -0500, John Ferlan wrote:
> Resolve a storage driver crash as a result of a long running
> storageVolCreateXML when the virStorageVolPoolRefreshThread is
> run as a result of a storageVolUpload complete and ran the
> virStoragePoolObjClearVols without checking if the creation
> code was currently processing a buildVol after incrementing
> the driver->asyncjob count.
> 
> The refreshThread needs to wait until all creation threads
> are completed so as to not alter the volume list while the
> volume is being built.
> 
> Crash from valgrind is as follows (with a bit of editing):
> 
> ==21309== Invalid read of size 8
> ==21309==at 0x153E47AF: storageBackendUpdateVolTargetInfo
> ==21309==by 0x153E4C30: virStorageBackendUpdateVolInfo
> ==21309==by 0x153E52DE: virStorageBackendVolRefreshLocal
> ==21309==by 0x153DE29E: storageVolCreateXML
> ==21309==by 0x562035B: virStorageVolCreateXML
> ==21309==by 0x147366: remoteDispatchStorageVolCreateXML
> ...
> ==21309==  Address 0x2590a720 is 64 bytes inside a block of size 336 free'd
> ==21309==at 0x4C2F2BB: free
> ==21309==by 0x54CB9FA: virFree
> ==21309==by 0x55BC800: virStorageVolDefFree
> ==21309==by 0x55BF1D8: virStoragePoolObjClearVols
> ==21309==by 0x153D967E: virStorageVolPoolRefreshThread
> ...
> ==21309==  Block was alloc'd at
> ==21309==at 0x4C300A5: calloc
> ==21309==by 0x54CB483: virAlloc
> ==21309==by 0x55BDC1F: virStorageVolDefParseXML
> ==21309==by 0x55BDC1F: virStorageVolDefParseNode
> ==21309==by 0x55BE5A4: virStorageVolDefParse
> ==21309==by 0x153DDFF1: storageVolCreateXML
> ==21309==by 0x562035B: virStorageVolCreateXML
> ==21309==by 0x147366: remoteDispatchStorageVolCreateXML
> ...
> 
> Signed-off-by: John Ferlan 
> ---
>  src/storage/storage_driver.c | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index b0edf9f885..5e920fc14c 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -2257,6 +2257,7 @@ virStorageVolPoolRefreshThread(void *opaque)
>  virStorageBackendPtr backend;
>  virObjectEventPtr event = NULL;
>  
> + retry:
>  storageDriverLock();
>  if (cbdata->vol_path) {
>  if (virStorageBackendPloopRestoreDesc(cbdata->vol_path) < 0)
> @@ -2270,6 +2271,14 @@ virStorageVolPoolRefreshThread(void *opaque)
>  if (!(backend = virStorageBackendForType(def->type)))
>  goto cleanup;
>  
> +/* Some thread is creating a new volume in the pool, we need to retry */
> +if (virStoragePoolObjGetAsyncjobs(obj) > 0) {
> +virStoragePoolObjUnlock(obj);
> +storageDriverUnlock();
> +usleep(100 * 1000);
> +goto retry;
> +}
> +
>  virStoragePoolObjClearVols(obj);
>  if (backend->refreshPool(NULL, obj) < 0)
>  VIR_DEBUG("Failed to refresh storage pool");

ACK, does the job here. I'm rather surprised to see you implementing it
with sleep, while you pointed me towards virCondWait yesterday. But using
sleep is a way less intrusive change.

--
Cedric

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

Re: [libvirt] Interim fix for exposing VMware firmware=bios|efi in libvirt XML

[Daniel P. Berrange]

On Tue, Nov 07, 2017 at 09:09:07AM +, Richard W.M. Jones wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1217444
> 
> VMware guests can use either BIOS or UEFI firmware.  VMware itself
> exposes this information in the VMX file and it's available to the ESX
> driver in libvirt.  virt-v2v wants to consume this information.
> 
> Unfortunately after a few years and iterations we've not come up with
> an acceptable patch to expose “has UEFI” in the libvirt XML.  (Latest
> patch was:
> https://www.redhat.com/archives/libvir-list/2016-October/msg00045.html )
> 
> Could we instead add a temporary addition to the XML generated by the
> ESX driver?  I'm thinking something like the existing 
> and  fields:
> 
>  xmlns:vmware='http://libvirt.org/schemas/domain/vmware/1.0'>
>   Fedora
> ...
>   ha-datacenter
>   2
>   uefi  
> 
> 
> This field would be informational, ie. the ESX driver would create it
> but not read it when creating new VMs.
> 
> This would solve our immediate problem in virt-v2v and is pretty
> simple to implement.  Also it doesn't close off any future general
> solution.

I don't see a need todo that. The problems with the patch I sent previously
were all in the QEMU driver support for it. We can easily merge the XML
config bit (my patch #2), and then do an ESX impl, and figure out QEMU impl
later.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[libvirt] Interim fix for exposing VMware firmware=bios|efi in libvirt XML

[Richard W.M. Jones]

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

VMware guests can use either BIOS or UEFI firmware.  VMware itself
exposes this information in the VMX file and it's available to the ESX
driver in libvirt.  virt-v2v wants to consume this information.

Unfortunately after a few years and iterations we've not come up with
an acceptable patch to expose “has UEFI” in the libvirt XML.  (Latest
patch was:
https://www.redhat.com/archives/libvir-list/2016-October/msg00045.html )

Could we instead add a temporary addition to the XML generated by the
ESX driver?  I'm thinking something like the existing 
and  fields:


  Fedora
...
  ha-datacenter
  2
  uefi  


This field would be informational, ie. the ESX driver would create it
but not read it when creating new VMs.

This would solve our immediate problem in virt-v2v and is pretty
simple to implement.  Also it doesn't close off any future general
solution.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

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

Re: [libvirt] [PATCH v2] vz: allow to start vz driver without host cache info

[Mikhail Feoktistov]

On 11.07.2017 15:55, Martin Kletzander wrote:

On Tue, Jul 11, 2017 at 03:20:38PM +0300, Mikhail Feoktistov wrote:


On 11.07.2017 12:22, Martin Kletzander wrote:

On Tue, Jul 11, 2017 at 04:59:05AM -0400, Mikhail Feoktistov wrote:

Show warning message instead of fail operation.
It happens if kernel or cpu doesn't support reporting cpu cache info.
In case of Virtuozzo file "id" doesn't exist.


What is your kernel version?


 3.10.0-514.16.1.vz7.30.10

Is there another way of getting the information about the cache ID?
Maybe we need to parse the name of the cache directory 'index2' 
would be

id 2 maybe?  If there is no other way, then this fix is fine (as most
drivers do the same thing), but I would rather fix it if that's
possible.  Unfortunately the cache information is structured stupidly
compared to other kernel-provided topology-related information.

Only kernel 4.11 reports cache ID (maybe 4.10, but not checked)
4.9 doesn't report ID


Oh, so that is just older kernel.  OK, then.

Reviewed-by: Martin Kletzander 


ID doesn't match to indexN



Yeah, in vanilla it is number per level and easily deductible, but not
really something we want to add into the code.

Martin, do you agree with this patch?

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