Re: [libvirt] [Qemu-devel] [PATCH v4 2/4] block/rbd: Attempt to parse legacy filenames

[Markus Armbruster]

Jeff Cody  writes:

> When we converted rbd to get rid of the older key/value-centric
> encoding format, we broke compatibility with image files with backing
> file strings encoded in the old format.
>
> This leaves a bit of an ugly conundrum, and a hacky solution.
>
> If the initial attempt to parse the "proper" options fails, it assumes
> that we may have an older key/value encoded filename.  Fall back to
> attempting to parse the filename, and extract the required options from
> it.  If that fails, pass along the original error message.
>
> We do not support mixed modern usage alongside legacy keyvalue pair
> usage.
>
> A deprecation warning has been added, although care should be taken
> when actually deprecating since the impact is not limited to
> commandline or qapi usage, but also opening existing images.
>
> Reviewed-by: Eric Blake 
> Signed-off-by: Jeff Cody 
> ---
>  block/rbd.c | 53 +++--
>  1 file changed, 51 insertions(+), 2 deletions(-)
>
> diff --git a/block/rbd.c b/block/rbd.c
> index b199450f9f..5090e4f662 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -678,6 +678,33 @@ static int qemu_rbd_convert_options(QDict *options, 
> BlockdevOptionsRbd **opts,
>  return 0;
>  }
>  
> +static int qemu_rbd_attempt_legacy_options(QDict *options,
> +   BlockdevOptionsRbd **opts,
> +   char **keypairs)
> +{
> +char *filename;
> +int r;
> +
> +filename = g_strdup(qdict_get_try_str(options, "filename"));
> +if (!filename) {
> +return -EINVAL;
> +}
> +qdict_del(options, "filename");
> +
> +qemu_rbd_parse_filename(filename, options, NULL);
> +
> +/* keypairs freed by caller */
> +*keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
> +if (*keypairs) {
> +qdict_del(options, "=keyvalue-pairs");
> +}
> +
> +r = qemu_rbd_convert_options(options, opts, NULL);
> +
> +g_free(filename);
> +return r;
> +}
> +
>  static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>   Error **errp)
>  {
> @@ -700,8 +727,30 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  
>  r = qemu_rbd_convert_options(options, &opts, &local_err);
>  if (local_err) {
> -error_propagate(errp, local_err);
> -goto out;
> +/* If keypairs are present, that means some options are present in
> + * the modern option format.  Don't attempt to parse legacy option
> + * formats, as we won't support mixed usage. */
> +if (keypairs) {
> +error_propagate(errp, local_err);
> +goto out;
> +}
> +
> +/* If the initial attempt to convert and process the options failed,
> + * we may be attempting to open an image file that has the rbd 
> options
> + * specified in the older format consisting of all key/value pairs
> + * encoded in the filename.  Go ahead and attempt to parse the
> + * filename, and see if we can pull out the required options. */
> +r = qemu_rbd_attempt_legacy_options(options, &opts, &keypairs);
> +if (r < 0) {
> +error_propagate(errp, local_err);
> +goto out;

This reports the error from qemu_rbd_convert_options(), as you commit
message explains.  Would explaining it in a comment help future readers?

> +}
> +/* Take care whenever deciding to actually deprecate; once this 
> ability
> + * is removed, we will not be able to open any images with 
> legacy-styled
> + * backing image strings. */
> +error_report("RBD options encoded in the filename as keyvalue pairs "
> + "is deprecated.  Future versions may cease to parse "
> + "these options in the future.");

"Future versions may ... in the future": you're serious about this
happening only in the future, aren't you?  ;)

Quote error_report()'s contract: "The resulting message should be a
single phrase, with no newline or trailing punctuation."

Let's scratch everything from the first period on.  

>  }
>  
>  /* Remove the processed options from the QDict (the visitor processes

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

Re: [libvirt] [PATCH 0/2] tests: Add QEMU 3.0.0 capability tests for s390x

[Bjoern Walk]

John Ferlan  [2018-09-21, 07:46AM -0400]:
> 
> 
> On 09/21/2018 01:13 AM, Bjoern Walk wrote:
> > John Ferlan  [2018-09-20, 04:30PM -0400]:
> >>
> >>
> >> On 09/20/2018 06:20 AM, Boris Fiuczynski wrote:
> >>> Add capability and domcaps tests for QEMU 3.0.0 on s390x.
> >>>
> >>> Boris Fiuczynski (2):
> >>>   tests: Add capabilities data for QEMU 3.0.0 on s390x
> >>>   tests: domaincaps: Add QEMU 3.0 for s390x
> >>>
> >>>  .../domaincapsschemadata/qemu_3.0.0.s390x.xml |   183 +
> >>>  tests/domaincapstest.c| 4 +
> >>>  .../caps_3.0.0.s390x.replies  | 20530 
> >>>  .../qemucapabilitiesdata/caps_3.0.0.s390x.xml |  2691 ++
> >>>  tests/qemucapabilitiestest.c  | 1 +
> >>>  5 files changed, 23409 insertions(+)
> >>>  create mode 100644 tests/domaincapsschemadata/qemu_3.0.0.s390x.xml
> >>>  create mode 100644 tests/qemucapabilitiesdata/caps_3.0.0.s390x.replies
> >>>  create mode 100644 tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml
> >>>
> >>
> >> Reviewed-by: John Ferlan 
> >> (series)
> >>
> >> and pushed,
> > 
> > Thanks, but just for interest, tracking Reviewed-by and other tags is a
> > thing in libvirt or not?
> > 
> 
>  - totally forgot to do that on this series - my apologies...
> I still haven't succumbed to figuring out a way to make the R-B add
> happen automagically And our hook only cares to see the S-O-B.

No worries. Normally we review our stuff internally first, so the rev-bys
are added to the upstream patch. For more complex patches I think we
will continue this way.

> To answer the question - there's no "official" tracking. Some people
> still use ACK while others use R-By. It was hard enough to jump the
> hurdle for S-O-B.

Ok, good to know.

-- 
IBM Systems
Linux on Z & Virtualization Development

IBM Deutschland Research & Development GmbH
Schönaicher Str. 220, 71032 Böblingen
Phone: +49 7031 16 1819

Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294 


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

Re: [libvirt] [PATCH 0/4] Couple of metadata locking fixes

[Bjoern Walk]

Michal Privoznik  [2018-09-21, 11:29AM +0200]:
> @Bjoern: I'm still unable to reproduce the issue you reported. However,
> whilst trying to do so I've came across this bug. However, my gut
> feeling is that this might help you.

Quick check, same issue. I will start debugging next week to get at
least an idea what might happen.

-- 
IBM Systems
Linux on Z & Virtualization Development

IBM Deutschland Research & Development GmbH
Schönaicher Str. 220, 71032 Böblingen
Phone: +49 7031 16 1819

Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294 


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

Re: [libvirt] [PATCH 02/13] virsh: Implement vshTable API to net-list and net-dhcp-leases

[Michal Privoznik]

On 09/21/2018 03:35 PM, Erik Skultety wrote:
> On Wed, Sep 19, 2018 at 11:26:27AM +0200, Michal Privoznik wrote:
>> On 09/18/2018 04:21 PM, Simon Kobyda wrote:
>>> Signed-off-by: Simon Kobyda 
>>> ---
>>>  tools/virsh-network.c | 55 +--
>>>  1 file changed, 37 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/tools/virsh-network.c b/tools/virsh-network.c
>>> index ca07fb568f..0f975b8899 100644
>>> --- a/tools/virsh-network.c
>>> +++ b/tools/virsh-network.c
>>> @@ -33,6 +33,7 @@
>>>  #include "virstring.h"
>>>  #include "virtime.h"
>>>  #include "conf/network_conf.h"
>>> +#include "vsh-table.h"
>>>
>>>  #define VIRSH_COMMON_OPT_NETWORK(_helpstr, cflags) \
>>>  {.name = "network", \
>>> @@ -677,6 +678,7 @@ cmdNetworkList(vshControl *ctl, const vshCmd *cmd 
>>> ATTRIBUTE_UNUSED)
>>>  bool optUUID = vshCommandOptBool(cmd, "uuid");
>>>  char uuid[VIR_UUID_STRING_BUFLEN];
>>>  unsigned int flags = VIR_CONNECT_LIST_NETWORKS_ACTIVE;
>>> +vshTablePtr table = NULL;
>>>
>>>  if (vshCommandOptBool(cmd, "inactive"))
>>>  flags = VIR_CONNECT_LIST_NETWORKS_INACTIVE;
>>> @@ -705,10 +707,10 @@ cmdNetworkList(vshControl *ctl, const vshCmd *cmd 
>>> ATTRIBUTE_UNUSED)
>>>  return false;
>>>
>>>  if (optTable) {
>>> -vshPrintExtra(ctl, " %-20s %-10s %-13s %s\n", _("Name"), 
>>> _("State"),
>>> -  _("Autostart"), _("Persistent"));
>>> -vshPrintExtra(ctl,
>>> -  
>>> "--\n");
>>> +table = vshTableNew("Name", "State", "Autostart",
>>> +"Persistent", NULL);
>>> +if (!table)
>>> +goto cleanup;
>>>  }
>>>
>>>  for (i = 0; i < list->nnets; i++) {
>>> @@ -722,11 +724,15 @@ cmdNetworkList(vshControl *ctl, const vshCmd *cmd 
>>> ATTRIBUTE_UNUSED)
>>>  else
>>>  autostartStr = is_autostart ? _("yes") : _("no");
>>>
>>> -vshPrint(ctl, " %-20s %-10s %-13s %s\n",
>>> - virNetworkGetName(network),
>>> - virNetworkIsActive(network) ? _("active") : 
>>> _("inactive"),
>>> - autostartStr,
>>> - virNetworkIsPersistent(network) ? _("yes") : _("no"));
>>> +if (vshTableRowAppend(table,
>>> +  virNetworkGetName(network),
>>> +  virNetworkIsActive(network) ?
>>> +  _("active") : _("inactive"),
>>> +  autostartStr,
>>> +  virNetworkIsPersistent(network) ?
>>> +  _("yes") : _("no"),
>>> +  NULL) < 0)
>>> +goto cleanup;
>>>  } else if (optUUID) {
>>>  if (virNetworkGetUUIDString(network, uuid) < 0) {
>>>  vshError(ctl, "%s", _("Failed to get network's UUID"));
>>> @@ -738,8 +744,12 @@ cmdNetworkList(vshControl *ctl, const vshCmd *cmd 
>>> ATTRIBUTE_UNUSED)
>>>  }
>>>  }
>>>
>>> +if (optTable)
>>> +vshTablePrintToStdout(table, ctl);
>>> +
>>>  ret = true;
>>>   cleanup:
>>> +vshTableFree(table);
>>>  virshNetworkListFree(list);
>>>  return ret;
>>>  }
>>> @@ -1351,6 +1361,7 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd 
>>> *cmd)
>>>  size_t i;
>>>  unsigned int flags = 0;
>>>  virNetworkPtr network = NULL;
>>> +vshTablePtr table = NULL;
>>>
>>>  if (vshCommandOptStringReq(ctl, cmd, "mac", &mac) < 0)
>>>  return false;
>>> @@ -1366,11 +1377,11 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd 
>>> *cmd)
>>>  /* Sort the list according to MAC Address/IAID */
>>>  qsort(leases, nleases, sizeof(*leases), virshNetworkDHCPLeaseSorter);
>>>
>>> -vshPrintExtra(ctl, " %-20s %-18s %-9s %-25s %-15s %s\n%s%s\n",
>>> -  _("Expiry Time"), _("MAC address"), _("Protocol"),
>>> -  _("IP address"), _("Hostname"), _("Client ID or DUID"),
>>> -  
>>> "--",
>>> -  
>>> "-");
>>> +table = vshTableNew("Expiry Time", "MAC address", "Protocol",
>>> +"IP address", "Hostname", "Client ID or DUID",
>>> +NULL);
>>> +if (!table)
>>> +goto cleanup;
>>>
>>>  for (i = 0; i < nleases; i++) {
>>>  const char *typestr = NULL;
>>> @@ -1390,17 +1401,25 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd 
>>> *cmd)
>>>  ignore_value(virAsprintf(&cidr_format, "%s/%d",
>>>   lease->ipaddr, lease->prefix));
>>>
>>> -vshPrint(ctl, " %-20s %-18s %-9s %-25s %-15s %s\n",
>>> - expirytime, EMPTYSTR(lease->mac),
>>> - EMPTYST

[libvirt] [PATCH v2 09/13] virsh: Implement vshTable API to domiflist

[Simon Kobyda]

Signed-off-by: Simon Kobyda 
---
 tools/virsh-domain-monitor.c | 41 ++--
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index b887bb48d9..4bba8438af 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -693,6 +693,7 @@ cmdDomiflist(vshControl *ctl, const vshCmd *cmd)
 int ninterfaces;
 xmlNodePtr *interfaces = NULL;
 size_t i;
+vshTablePtr table = NULL;
 
 if (vshCommandOptBool(cmd, "inactive"))
 flags |= VIR_DOMAIN_XML_INACTIVE;
@@ -704,16 +705,17 @@ cmdDomiflist(vshControl *ctl, const vshCmd *cmd)
 if (ninterfaces < 0)
 goto cleanup;
 
-vshPrintExtra(ctl, "%-10s %-10s %-10s %-11s %s\n", _("Interface"),
-  _("Type"), _("Source"), _("Model"), _("MAC"));
-vshPrintExtra(ctl, 
"---\n");
+table = vshTableNew(_("Interface"), _("Type"),
+_("Source"), _("Model"), _("MAC"), NULL);
+if (!table)
+goto cleanup;
 
 for (i = 0; i < ninterfaces; i++) {
-char *type = NULL;
-char *source = NULL;
-char *target = NULL;
-char *model = NULL;
-char *mac = NULL;
+VIR_AUTOFREE(char *) type = NULL;
+VIR_AUTOFREE(char *) source = NULL;
+VIR_AUTOFREE(char *) target = NULL;
+VIR_AUTOFREE(char *) model = NULL;
+VIR_AUTOFREE(char *) mac = NULL;
 
 ctxt->node = interfaces[i];
 type = virXPathString("string(./@type)", ctxt);
@@ -727,23 +729,22 @@ cmdDomiflist(vshControl *ctl, const vshCmd *cmd)
 model = virXPathString("string(./model/@type)", ctxt);
 mac = virXPathString("string(./mac/@address)", ctxt);
 
-vshPrint(ctl, "%-10s %-10s %-10s %-11s %-10s\n",
- target ? target : "-",
- type,
- source ? source : "-",
- model ? model : "-",
- mac ? mac : "-");
-
-VIR_FREE(type);
-VIR_FREE(source);
-VIR_FREE(target);
-VIR_FREE(model);
-VIR_FREE(mac);
+if (vshTableRowAppend(table,
+  target ? target : "-",
+  type,
+  source ? source : "-",
+  model ? model : "-",
+  mac ? mac : "-",
+  NULL) < 0)
+goto cleanup;
 }
 
+vshTablePrintToStdout(table, ctl);
+
 ret = true;
 
  cleanup:
+vshTableFree(table);
 VIR_FREE(interfaces);
 xmlFreeDoc(xmldoc);
 xmlXPathFreeContext(ctxt);
-- 
2.17.1

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

[libvirt] [PATCH v2 11/13] virsh: Implement vshTable API to pool-list

[Simon Kobyda]

Local lengthy unicode-unreliable table formatting was replaced by new
API. Great example of how new API saves space and time.
Removed a lot of string lenght canculation used by the local table.

Signed-off-by: Simon Kobyda 
---
 tools/virsh-pool.c | 162 +
 1 file changed, 31 insertions(+), 131 deletions(-)

diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c
index 89206a48f5..75ec572af2 100644
--- a/tools/virsh-pool.c
+++ b/tools/virsh-pool.c
@@ -33,6 +33,7 @@
 #include "conf/storage_conf.h"
 #include "virstring.h"
 #include "virtime.h"
+#include "vsh-table.h"
 
 #define VIRSH_COMMON_OPT_POOL_FULL(cflags) \
 VIRSH_COMMON_OPT_POOL(N_("pool name or uuid"), cflags)
@@ -1113,10 +1114,6 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
 virStoragePoolInfo info;
 size_t i;
 bool ret = false;
-size_t stringLength = 0, nameStrLength = 0;
-size_t autostartStrLength = 0, persistStrLength = 0;
-size_t stateStrLength = 0, capStrLength = 0;
-size_t allocStrLength = 0, availStrLength = 0;
 struct poolInfoText {
 char *state;
 char *autostart;
@@ -1133,7 +1130,7 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
 bool inactive, all;
 bool uuid = false;
 bool name = false;
-char *outputStr = NULL;
+vshTablePtr table = NULL;
 
 inactive = vshCommandOptBool(cmd, "inactive");
 all = vshCommandOptBool(cmd, "all");
@@ -1260,11 +1257,6 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
 else
 poolInfoTexts[i].persistent = vshStrdup(ctl, persistent ?
  _("yes") : _("no"));
-
-/* Keep the length of persistent string if longest so far */
-stringLength = strlen(poolInfoTexts[i].persistent);
-if (stringLength > persistStrLength)
-persistStrLength = stringLength;
 }
 
 /* Collect further extended information about the pool */
@@ -1310,21 +1302,6 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
 poolInfoTexts[i].allocation = vshStrdup(ctl, _("-"));
 poolInfoTexts[i].available = vshStrdup(ctl, _("-"));
 }
-
-/* Keep the length of capacity string if longest so far */
-stringLength = strlen(poolInfoTexts[i].capacity);
-if (stringLength > capStrLength)
-capStrLength = stringLength;
-
-/* Keep the length of allocation string if longest so far */
-stringLength = strlen(poolInfoTexts[i].allocation);
-if (stringLength > allocStrLength)
-allocStrLength = stringLength;
-
-/* Keep the length of available string if longest so far */
-stringLength = strlen(poolInfoTexts[i].available);
-if (stringLength > availStrLength)
-availStrLength = stringLength;
 } else {
 /* --details option was not specified, only active/inactive
  * state strings are used */
@@ -1334,21 +1311,6 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
 poolInfoTexts[i].state = vshStrdup(ctl, _("inactive"));
}
 }
-
-/* Keep the length of name string if longest so far */
-stringLength = strlen(virStoragePoolGetName(list->pools[i]));
-if (stringLength > nameStrLength)
-nameStrLength = stringLength;
-
-/* Keep the length of state string if longest so far */
-stringLength = strlen(poolInfoTexts[i].state);
-if (stringLength > stateStrLength)
-stateStrLength = stringLength;
-
-/* Keep the length of autostart string if longest so far */
-stringLength = strlen(poolInfoTexts[i].autostart);
-if (stringLength > autostartStrLength)
-autostartStrLength = stringLength;
 }
 
 /* If the --details option wasn't selected, we output the pool
@@ -1376,19 +1338,23 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
 }
 
 /* Output old style header */
-vshPrintExtra(ctl, " %-20s %-10s %-10s\n", _("Name"), _("State"),
-  _("Autostart"));
-vshPrintExtra(ctl, "---\n");
+table = vshTableNew(_("Name"), _("State"), _("Autostart"), NULL);
+if (!table)
+goto cleanup;
 
 /* Output old style pool info */
 for (i = 0; i < list->npools; i++) {
 const char *name_str = virStoragePoolGetName(list->pools[i]);
-vshPrint(ctl, " %-20s %-10s %-10s\n",
- name_str,
- poolInfoTexts[i].state,
- poolInfoTexts[i].autostart);
+if (vshTableRowAppend(table,
+ 

[libvirt] [PATCH v2 08/13] virsh: Implement vshTable API to domblklist

[Simon Kobyda]

Signed-off-by: Simon Kobyda 
---
 tools/virsh-domain-monitor.c | 21 ++---
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index 80c7e8a99e..b887bb48d9 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -589,6 +589,7 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd)
 char *device = NULL;
 char *target = NULL;
 char *source = NULL;
+vshTablePtr table = NULL;
 
 if (vshCommandOptBool(cmd, "inactive"))
 flags |= VIR_DOMAIN_XML_INACTIVE;
@@ -603,12 +604,12 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd)
 goto cleanup;
 
 if (details)
-vshPrintExtra(ctl, "%-10s %-10s %-10s %s\n", _("Type"),
-  _("Device"), _("Target"), _("Source"));
+table = vshTableNew(_("Type"), _("Device"), _("Target"), _("Source"), 
NULL);
 else
-vshPrintExtra(ctl, "%-10s %s\n", _("Target"), _("Source"));
+table = vshTableNew(_("Target"), _("Source"), NULL);
 
-vshPrintExtra(ctl, "\n");
+if (!table)
+goto cleanup;
 
 for (i = 0; i < ndisks; i++) {
 ctxt->node = disks[i];
@@ -633,10 +634,13 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd)
 "|./source/@name"
 "|./source/@volume)", ctxt);
 if (details) {
-vshPrint(ctl, "%-10s %-10s %-10s %s\n", type, device,
- target, source ? source : "-");
+if (vshTableRowAppend(table, type, device, target,
+  source ? source : "-", NULL) < 0)
+goto cleanup;
 } else {
-vshPrint(ctl, "%-10s %s\n", target, source ? source : "-");
+if (vshTableRowAppend(table, target,
+  source ? source : "-", NULL) < 0)
+goto cleanup;
 }
 
 VIR_FREE(source);
@@ -645,9 +649,12 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd)
 VIR_FREE(type);
 }
 
+vshTablePrintToStdout(table, ctl);
+
 ret = true;
 
  cleanup:
+vshTableFree(table);
 VIR_FREE(source);
 VIR_FREE(target);
 VIR_FREE(device);
-- 
2.17.1

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

[libvirt] [PATCH v2 12/13] virsh: Implement vshTable API to vol-list

[Simon Kobyda]

Local lengthy unicode-unreliable table formatting was replaced by new
API. Great example of how new API saves space and time.
Removed a lot of string lenght calculation used by the local table.

Signed-off-by: Simon Kobyda 
---
 tools/virsh-volume.c | 129 ++-
 1 file changed, 28 insertions(+), 101 deletions(-)

diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c
index 42d11701ec..6cd989446e 100644
--- a/tools/virsh-volume.c
+++ b/tools/virsh-volume.c
@@ -42,6 +42,7 @@
 #include "virsh-pool.h"
 #include "virxml.h"
 #include "virstring.h"
+#include "vsh-table.h"
 
 #define VIRSH_COMMON_OPT_POOL_FULL \
 VIRSH_COMMON_OPT_POOL(N_("pool name or uuid"), \
@@ -1382,16 +1383,11 @@ cmdVolList(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
 {
 virStorageVolInfo volumeInfo;
 virStoragePoolPtr pool;
-char *outputStr = NULL;
 const char *unit;
 double val;
 bool details = vshCommandOptBool(cmd, "details");
 size_t i;
 bool ret = false;
-int stringLength = 0;
-size_t allocStrLength = 0, capStrLength = 0;
-size_t nameStrLength = 0, pathStrLength = 0;
-size_t typeStrLength = 0;
 struct volInfoText {
 char *allocation;
 char *capacity;
@@ -1400,6 +1396,7 @@ cmdVolList(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
 };
 struct volInfoText *volInfoTexts = NULL;
 virshStorageVolListPtr list = NULL;
+vshTablePtr table = NULL;
 
 /* Look up the pool information given to us by the user */
 if (!(pool = virshCommandOptPool(ctl, cmd, "pool", NULL)))
@@ -1446,36 +1443,6 @@ cmdVolList(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
 "%.2lf %s", val, unit) < 0)
 goto cleanup;
 }
-
-/* Remember the largest length for each output string.
- * This lets us displaying header and volume information rows
- * using a single, properly sized, printf style output string.
- */
-
-/* Keep the length of name string if longest so far */
-stringLength = strlen(virStorageVolGetName(list->vols[i]));
-if (stringLength > nameStrLength)
-nameStrLength = stringLength;
-
-/* Keep the length of path string if longest so far */
-stringLength = strlen(volInfoTexts[i].path);
-if (stringLength > pathStrLength)
-pathStrLength = stringLength;
-
-/* Keep the length of type string if longest so far */
-stringLength = strlen(volInfoTexts[i].type);
-if (stringLength > typeStrLength)
-typeStrLength = stringLength;
-
-/* Keep the length of capacity string if longest so far */
-stringLength = strlen(volInfoTexts[i].capacity);
-if (stringLength > capStrLength)
-capStrLength = stringLength;
-
-/* Keep the length of allocation string if longest so far */
-stringLength = strlen(volInfoTexts[i].allocation);
-if (stringLength > allocStrLength)
-allocStrLength = stringLength;
 }
 }
 
@@ -1487,14 +1454,20 @@ cmdVolList(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
 /* Output basic info then return if --details option not selected */
 if (!details) {
 /* The old output format */
-vshPrintExtra(ctl, " %-20s %-40s\n", _("Name"), _("Path"));
-vshPrintExtra(ctl, "---"
-   "---\n");
+table = vshTableNew(_("Name"), _("Path"), NULL);
+if (!table)
+goto cleanup;
+
 for (i = 0; i < list->nvols; i++) {
-vshPrint(ctl, " %-20s %-40s\n", 
virStorageVolGetName(list->vols[i]),
- volInfoTexts[i].path);
+if (vshTableRowAppend(table,
+  virStorageVolGetName(list->vols[i]),
+  volInfoTexts[i].path,
+  NULL) < 0)
+goto cleanup;
 }
 
+vshTablePrintToStdout(table, ctl);
+
 /* Cleanup and return */
 ret = true;
 goto cleanup;
@@ -1502,75 +1475,30 @@ cmdVolList(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
 
 /* We only get here if the --details option was selected. */
 
-/* Use the length of name header string if it's longest */
-stringLength = strlen(_("Name"));
-if (stringLength > nameStrLength)
-nameStrLength = stringLength;
-
-/* Use the length of path header string if it's longest */
-stringLength = strlen(_("Path"));
-if (stringLength > pathStrLength)
-pathStrLength = stringLength;
-
-/* Use the length of type header string if it's longest */
-stringLength = strlen(_("Type"));
-if (stringLength > typeStrLength)
-typeStrLength = st

[libvirt] [PATCH v2 02/13] virsh: Implement vshTable API to net-list and net-dhcp-leases

[Simon Kobyda]

Signed-off-by: Simon Kobyda 
---
 tools/virsh-network.c | 59 ---
 1 file changed, 38 insertions(+), 21 deletions(-)

diff --git a/tools/virsh-network.c b/tools/virsh-network.c
index ca07fb568f..440b23d8a8 100644
--- a/tools/virsh-network.c
+++ b/tools/virsh-network.c
@@ -33,6 +33,7 @@
 #include "virstring.h"
 #include "virtime.h"
 #include "conf/network_conf.h"
+#include "vsh-table.h"
 
 #define VIRSH_COMMON_OPT_NETWORK(_helpstr, cflags) \
 {.name = "network", \
@@ -677,6 +678,7 @@ cmdNetworkList(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
 bool optUUID = vshCommandOptBool(cmd, "uuid");
 char uuid[VIR_UUID_STRING_BUFLEN];
 unsigned int flags = VIR_CONNECT_LIST_NETWORKS_ACTIVE;
+vshTablePtr table = NULL;
 
 if (vshCommandOptBool(cmd, "inactive"))
 flags = VIR_CONNECT_LIST_NETWORKS_INACTIVE;
@@ -705,10 +707,10 @@ cmdNetworkList(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
 return false;
 
 if (optTable) {
-vshPrintExtra(ctl, " %-20s %-10s %-13s %s\n", _("Name"), _("State"),
-  _("Autostart"), _("Persistent"));
-vshPrintExtra(ctl,
-  
"--\n");
+table = vshTableNew(_("Name"), _("State"), _("Autostart"),
+_("Persistent"), NULL);
+if (!table)
+goto cleanup;
 }
 
 for (i = 0; i < list->nnets; i++) {
@@ -722,11 +724,15 @@ cmdNetworkList(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
 else
 autostartStr = is_autostart ? _("yes") : _("no");
 
-vshPrint(ctl, " %-20s %-10s %-13s %s\n",
- virNetworkGetName(network),
- virNetworkIsActive(network) ? _("active") : _("inactive"),
- autostartStr,
- virNetworkIsPersistent(network) ? _("yes") : _("no"));
+if (vshTableRowAppend(table,
+  virNetworkGetName(network),
+  virNetworkIsActive(network) ?
+  _("active") : _("inactive"),
+  autostartStr,
+  virNetworkIsPersistent(network) ?
+  _("yes") : _("no"),
+  NULL) < 0)
+goto cleanup;
 } else if (optUUID) {
 if (virNetworkGetUUIDString(network, uuid) < 0) {
 vshError(ctl, "%s", _("Failed to get network's UUID"));
@@ -738,8 +744,12 @@ cmdNetworkList(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
 }
 }
 
+if (optTable)
+vshTablePrintToStdout(table, ctl);
+
 ret = true;
  cleanup:
+vshTableFree(table);
 virshNetworkListFree(list);
 return ret;
 }
@@ -1351,6 +1361,7 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd *cmd)
 size_t i;
 unsigned int flags = 0;
 virNetworkPtr network = NULL;
+vshTablePtr table = NULL;
 
 if (vshCommandOptStringReq(ctl, cmd, "mac", &mac) < 0)
 return false;
@@ -1366,15 +1377,15 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd *cmd)
 /* Sort the list according to MAC Address/IAID */
 qsort(leases, nleases, sizeof(*leases), virshNetworkDHCPLeaseSorter);
 
-vshPrintExtra(ctl, " %-20s %-18s %-9s %-25s %-15s %s\n%s%s\n",
-  _("Expiry Time"), _("MAC address"), _("Protocol"),
-  _("IP address"), _("Hostname"), _("Client ID or DUID"),
-  "--",
-  "-");
+table = vshTableNew(_("Expiry Time"), _("MAC address"), _("Protocol"),
+_("IP address"), _("Hostname"), _("Client ID or DUID"),
+NULL);
+if (!table)
+goto cleanup;
 
 for (i = 0; i < nleases; i++) {
 const char *typestr = NULL;
-char *cidr_format = NULL;
+VIR_AUTOFREE(char *) cidr_format = NULL;
 virNetworkDHCPLeasePtr lease = leases[i];
 time_t expirytime_tmp = lease->expirytime;
 struct tm ts;
@@ -1390,17 +1401,23 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd *cmd)
 ignore_value(virAsprintf(&cidr_format, "%s/%d",
  lease->ipaddr, lease->prefix));
 
-vshPrint(ctl, " %-20s %-18s %-9s %-25s %-15s %s\n",
- expirytime, EMPTYSTR(lease->mac),
- EMPTYSTR(typestr), cidr_format,
- EMPTYSTR(lease->hostname), EMPTYSTR(lease->clientid));
-
-VIR_FREE(cidr_format);
+if (vshTableRowAppend(table,
+  expirytime,
+  EMPTYSTR(lease->mac),
+  EMPTYSTR(typestr),
+  EMPTYSTR(cidr_format),
+  

[libvirt] [PATCH v2 01/13] virsh: Implement vsh-table to iface-list

[Simon Kobyda]

Signed-off-by: Simon Kobyda 
---
 tools/virsh-interface.c | 27 +++
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c
index 50518c667b..1eb1a27ac7 100644
--- a/tools/virsh-interface.c
+++ b/tools/virsh-interface.c
@@ -48,6 +48,7 @@
 #include "virutil.h"
 #include "virxml.h"
 #include "virstring.h"
+#include "vsh-table.h"
 
 virInterfacePtr
 virshCommandOptInterfaceBy(vshControl *ctl, const vshCmd *cmd,
@@ -356,6 +357,8 @@ cmdInterfaceList(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
 unsigned int flags = VIR_CONNECT_LIST_INTERFACES_ACTIVE;
 virshInterfaceListPtr list = NULL;
 size_t i;
+bool ret = false;
+vshTablePtr table = NULL;
 
 if (inactive)
 flags = VIR_CONNECT_LIST_INTERFACES_INACTIVE;
@@ -366,21 +369,29 @@ cmdInterfaceList(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
 if (!(list = virshInterfaceListCollect(ctl, flags)))
 return false;
 
-vshPrintExtra(ctl, " %-20s %-10s %s\n", _("Name"), _("State"),
-  _("MAC Address"));
-vshPrintExtra(ctl, 
"---\n");
+table = vshTableNew(_("Name"), _("State"), _("MAC Address"), NULL);
+if (!table)
+goto cleanup;
 
 for (i = 0; i < list->nifaces; i++) {
 virInterfacePtr iface = list->ifaces[i];
 
-vshPrint(ctl, " %-20s %-10s %s\n",
- virInterfaceGetName(iface),
- virInterfaceIsActive(iface) ? _("active") : _("inactive"),
- virInterfaceGetMACString(iface));
+if (vshTableRowAppend(table,
+  virInterfaceGetName(iface),
+  virInterfaceIsActive(iface) ? _("active")
+  : _("inactive"),
+  virInterfaceGetMACString(iface),
+  NULL) < 0)
+goto cleanup;
 }
 
+vshTablePrintToStdout(table, ctl);
+
+ret = true;
+ cleanup:
+vshTableFree(table);
 virshInterfaceListFree(list);
-return true;
+return ret;
 }
 
 /*
-- 
2.17.1

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

[libvirt] [PATCH v2 04/13] virsh: Implement vshTable API to nwfilter-list and nwfilterbinding-list

[Simon Kobyda]

Signed-off-by: Simon Kobyda 
---
 tools/virsh-nwfilter.c | 47 +-
 1 file changed, 33 insertions(+), 14 deletions(-)

diff --git a/tools/virsh-nwfilter.c b/tools/virsh-nwfilter.c
index 1cdbe5053a..b680ea082c 100644
--- a/tools/virsh-nwfilter.c
+++ b/tools/virsh-nwfilter.c
@@ -31,6 +31,7 @@
 #include "viralloc.h"
 #include "virfile.h"
 #include "virutil.h"
+#include "vsh-table.h"
 
 virNWFilterPtr
 virshCommandOptNWFilterBy(vshControl *ctl, const vshCmd *cmd,
@@ -359,26 +360,35 @@ cmdNWFilterList(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
 {
 size_t i;
 char uuid[VIR_UUID_STRING_BUFLEN];
+bool ret = false;
 virshNWFilterListPtr list = NULL;
+vshTablePtr table = NULL;
 
 if (!(list = virshNWFilterListCollect(ctl, 0)))
 return false;
 
-vshPrintExtra(ctl, " %-36s  %-20s \n", _("UUID"), _("Name"));
-vshPrintExtra(ctl, "-"
-   "-\n");
+table = vshTableNew(_("UUID"), _("Name"), NULL);
+if (!table)
+goto cleanup;
 
 for (i = 0; i < list->nfilters; i++) {
 virNWFilterPtr nwfilter = list->filters[i];
 
 virNWFilterGetUUIDString(nwfilter, uuid);
-vshPrint(ctl, " %-36s  %-20s\n",
- uuid,
- virNWFilterGetName(nwfilter));
+if (vshTableRowAppend(table,
+  uuid,
+  virNWFilterGetName(nwfilter),
+  NULL) < 0)
+goto cleanup;
 }
 
+vshTablePrintToStdout(table, ctl);
+
+ret = true;
+ cleanup:
+vshTableFree(table);
 virshNWFilterListFree(list);
-return true;
+return ret;
 }
 
 /*
@@ -714,25 +724,34 @@ static bool
 cmdNWFilterBindingList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED)
 {
 size_t i;
+bool ret = false;
 virshNWFilterBindingListPtr list = NULL;
+vshTablePtr table = NULL;
 
 if (!(list = virshNWFilterBindingListCollect(ctl, 0)))
 return false;
 
-vshPrintExtra(ctl, " %-20s  %-20s \n", _("Port Dev"), _("Filter"));
-vshPrintExtra(ctl, "-"
-   "-\n");
+table = vshTableNew(_("Port Dev"), _("Filter"), NULL);
+if (!table)
+goto cleanup;
 
 for (i = 0; i < list->nbindings; i++) {
 virNWFilterBindingPtr binding = list->bindings[i];
 
-vshPrint(ctl, " %-20s  %-20s\n",
- virNWFilterBindingGetPortDev(binding),
- virNWFilterBindingGetFilterName(binding));
+if (vshTableRowAppend(table,
+  virNWFilterBindingGetPortDev(binding),
+  virNWFilterBindingGetFilterName(binding),
+  NULL) < 0)
+goto cleanup;
 }
 
+vshTablePrintToStdout(table, ctl);
+
+ret = true;
+ cleanup:
+vshTableFree(table);
 virshNWFilterBindingListFree(list);
-return true;
+return ret;
 }
 
 
-- 
2.17.1

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

[libvirt] [PATCH v2 13/13] virt-admin: Implement vshTable API to server-list and client-list

[Simon Kobyda]

Signed-off-by: Simon Kobyda 
---
 tools/virt-admin.c | 47 ++
 1 file changed, 35 insertions(+), 12 deletions(-)

diff --git a/tools/virt-admin.c b/tools/virt-admin.c
index 63822bc13e..d119e4d960 100644
--- a/tools/virt-admin.c
+++ b/tools/virt-admin.c
@@ -40,6 +40,7 @@
 #include "virgettext.h"
 #include "virtime.h"
 #include "virt-admin-completer.h"
+#include "vsh-table.h"
 
 /* Gnulib doesn't guarantee SA_SIGINFO support.  */
 #ifndef SA_SIGINFO
@@ -382,6 +383,7 @@ cmdSrvList(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
 char *uri = NULL;
 virAdmServerPtr *srvs = NULL;
 vshAdmControlPtr priv = ctl->privData;
+vshTablePtr table = NULL;
 
 /* Obtain a list of available servers on the daemon */
 if ((nsrvs = virAdmConnectListServers(priv->conn, &srvs, 0)) < 0) {
@@ -391,13 +393,27 @@ cmdSrvList(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
 goto cleanup;
 }
 
-vshPrintExtra(ctl, " %-5s %-15s\n", "Id", "Name");
-vshPrintExtra(ctl, "---\n");
-for (i = 0; i < nsrvs; i++)
-vshPrint(ctl, " %-5zu %-15s\n", i, virAdmServerGetName(srvs[i]));
+table = vshTableNew(_("Id"), _("Name"), NULL);
+if (!table)
+goto cleanup;
+
+for (i = 0; i < nsrvs; i++) {
+VIR_AUTOFREE(char *) idStr = NULL;
+if (virAsprintf(&idStr, "%lu", i) < 0)
+goto cleanup;
+
+if (vshTableRowAppend(table,
+  idStr,
+  virAdmServerGetName(srvs[i]),
+  NULL) < 0)
+goto cleanup;
+}
+
+vshTablePrintToStdout(table, ctl);
 
 ret = true;
  cleanup:
+vshTableFree(table);
 if (srvs) {
 for (i = 0; i < nsrvs; i++)
 virAdmServerFree(srvs[i]);
@@ -613,10 +629,10 @@ cmdSrvClientsList(vshControl *ctl, const vshCmd *cmd)
 const char *srvname = NULL;
 unsigned long long id;
 virClientTransport transport;
-char *timestr = NULL;
 virAdmServerPtr srv = NULL;
 virAdmClientPtr *clts = NULL;
 vshAdmControlPtr priv = ctl->privData;
+vshTablePtr table = NULL;
 
 if (vshCommandOptStringReq(ctl, cmd, "server", &srvname) < 0)
 return false;
@@ -631,12 +647,13 @@ cmdSrvClientsList(vshControl *ctl, const vshCmd *cmd)
 goto cleanup;
 }
 
-vshPrintExtra(ctl, " %-5s %-15s %-15s\n%s\n", _("Id"), _("Transport"),
-  _("Connected since"),
-  "-"
-  "-");
+table = vshTableNew(_("Id"), _("Transport"), _("Connected sice"), NULL);
+if (!table)
+goto cleanup;
 
 for (i = 0; i < nclts; i++) {
+VIR_AUTOFREE(char *) timestr = NULL;
+VIR_AUTOFREE(char *) idStr = NULL;
 virAdmClientPtr client = clts[i];
 id = virAdmClientGetID(client);
 transport = virAdmClientGetTransport(client);
@@ -644,14 +661,20 @@ cmdSrvClientsList(vshControl *ctl, const vshCmd *cmd)
  ×tr) < 0)
 goto cleanup;
 
-vshPrint(ctl, " %-5llu %-15s %-15s\n",
- id, vshAdmClientTransportToString(transport), timestr);
-VIR_FREE(timestr);
+if (virAsprintf(&idStr, "%llu", id) < 0)
+goto cleanup;
+if (vshTableRowAppend(table, idStr,
+  vshAdmClientTransportToString(transport),
+  timestr, NULL) < 0)
+goto cleanup;
 }
 
+vshTablePrintToStdout(table, ctl);
+
 ret = true;
 
  cleanup:
+vshTableFree(table);
 if (clts) {
 for (i = 0; i < nclts; i++)
 virAdmClientFree(clts[i]);
-- 
2.17.1

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

[libvirt] [PATCH v2 03/13] virsh: Implement vshTable API to secret-list

[Simon Kobyda]

Signed-off-by: Simon Kobyda 
---
 tools/virsh-secret.c | 28 
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c
index 670beea706..87239ff60b 100644
--- a/tools/virsh-secret.c
+++ b/tools/virsh-secret.c
@@ -35,6 +35,7 @@
 #include "virsecret.h"
 #include "virstring.h"
 #include "virtime.h"
+#include "vsh-table.h"
 
 static virSecretPtr
 virshCommandOptSecret(vshControl *ctl, const vshCmd *cmd, const char **name)
@@ -507,6 +508,7 @@ cmdSecretList(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
 virshSecretListPtr list = NULL;
 bool ret = false;
 unsigned int flags = 0;
+vshTablePtr table = NULL;
 
 if (vshCommandOptBool(cmd, "ephemeral"))
 flags |= VIR_CONNECT_LIST_SECRETS_EPHEMERAL;
@@ -523,15 +525,17 @@ cmdSecretList(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
 if (!(list = virshSecretListCollect(ctl, flags)))
 return false;
 
-vshPrintExtra(ctl, " %-36s  %s\n", _("UUID"), _("Usage"));
-vshPrintExtra(ctl, ""
-   "\n");
+table = vshTableNew(_("UUID"), _("Usage"), NULL);
+if (!table)
+goto cleanup;
 
 for (i = 0; i < list->nsecrets; i++) {
 virSecretPtr sec = list->secrets[i];
 int usageType = virSecretGetUsageType(sec);
 const char *usageStr = virSecretUsageTypeToString(usageType);
 char uuid[VIR_UUID_STRING_BUFLEN];
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+VIR_AUTOFREE(char *) usage = NULL;
 
 if (virSecretGetUUIDString(sec, uuid) < 0) {
 vshError(ctl, "%s", _("Failed to get uuid of secret"));
@@ -539,18 +543,26 @@ cmdSecretList(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
 }
 
 if (usageType) {
-vshPrint(ctl, " %-36s  %s %s\n",
- uuid, usageStr,
- virSecretGetUsageID(sec));
+virBufferStrcat(&buf, usageStr, " ",
+virSecretGetUsageID(sec), NULL);
+usage = virBufferContentAndReset(&buf);
+if (!usage)
+goto cleanup;
+
+if (vshTableRowAppend(table, uuid, usage, NULL) < 0)
+goto cleanup;
 } else {
-vshPrint(ctl, " %-36s  %s\n",
- uuid, _("Unused"));
+if (vshTableRowAppend(table, uuid, _("Unused"), NULL) < 0)
+goto cleanup;
 }
 }
 
+vshTablePrintToStdout(table, ctl);
+
 ret = true;
 
  cleanup:
+vshTableFree(table);
 virshSecretListFree(list);
 return ret;
 }
-- 
2.17.1

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

[libvirt] [PATCH v2 06/13] virsh: Set up cmdDomblkinfo() and cmdDomblkinfoPrint() for vshTable API implementation

[Simon Kobyda]

I've moved all the printing from cmdDomblkinfoPrint() to cmdDomblkinfo(),
and renamed the cmdDomblkinfoPrint() to cmdDomblkinfoGet(), since nature of
that function changed from gathering and printing informations only to
gathering information. This I believe simplifies the functions and
makes the implementation of vshTable API simpler.

Signed-off-by: Simon Kobyda 
---
 tools/virsh-domain-monitor.c | 78 +---
 1 file changed, 36 insertions(+), 42 deletions(-)

diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index adc5bb1a7a..cb48f9a7be 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -406,33 +406,23 @@ static const vshCmdOptDef opts_domblkinfo[] = {
 {.name = NULL}
 };
 
-static void
-cmdDomblkinfoPrint(vshControl *ctl,
+static bool
+cmdDomblkinfoGet(vshControl *ctl,
const virDomainBlockInfo *info,
-   const char *device,
-   bool human, bool title)
+   char **cap,
+   char **alloc,
+   char **phy,
+   bool human)
 {
-char *cap = NULL;
-char *alloc = NULL;
-char *phy = NULL;
-
-if (title) {
-vshPrintExtra(ctl, "%-10s %-15s %-15s %-15s\n", _("Target"),
-  _("Capacity"), _("Allocation"), _("Physical"));
-vshPrintExtra(ctl, "-"
-  "\n");
-return;
-}
-
 if (info->capacity == 0 && info->allocation == 0 && info->physical == 0) {
-cap = vshStrdup(ctl, "-");
-alloc = vshStrdup(ctl, "-");
-phy = vshStrdup(ctl, "-");
+*cap = vshStrdup(ctl, "-");
+*alloc = vshStrdup(ctl, "-");
+*phy = vshStrdup(ctl, "-");
 } else if (!human) {
-if (virAsprintf(&cap, "%llu", info->capacity) < 0 ||
-virAsprintf(&alloc, "%llu", info->allocation) < 0 ||
-virAsprintf(&phy, "%llu", info->physical) < 0)
-goto cleanup;
+if (virAsprintf(cap, "%llu", info->capacity) < 0 ||
+virAsprintf(alloc, "%llu", info->allocation) < 0 ||
+virAsprintf(phy, "%llu", info->physical) < 0)
+return false;
 } else {
 double val_cap, val_alloc, val_phy;
 const char *unit_cap, *unit_alloc, *unit_phy;
@@ -441,24 +431,13 @@ cmdDomblkinfoPrint(vshControl *ctl,
 val_alloc = vshPrettyCapacity(info->allocation, &unit_alloc);
 val_phy = vshPrettyCapacity(info->physical, &unit_phy);
 
-if (virAsprintf(&cap, "%.3lf %s", val_cap, unit_cap) < 0 ||
-virAsprintf(&alloc, "%.3lf %s", val_alloc, unit_alloc) < 0 ||
-virAsprintf(&phy, "%.3lf %s", val_phy, unit_phy) < 0)
-goto cleanup;
-}
-
-if (device) {
-vshPrint(ctl, "%-10s %-15s %-15s %-15s\n", device, cap, alloc, phy);
-} else {
-vshPrint(ctl, "%-15s %s\n", _("Capacity:"), cap);
-vshPrint(ctl, "%-15s %s\n", _("Allocation:"), alloc);
-vshPrint(ctl, "%-15s %s\n", _("Physical:"), phy);
+if (virAsprintf(cap, "%.3lf %s", val_cap, unit_cap) < 0 ||
+virAsprintf(alloc, "%.3lf %s", val_alloc, unit_alloc) < 0 ||
+virAsprintf(phy, "%.3lf %s", val_phy, unit_phy) < 0)
+return false;
 }
 
- cleanup:
-VIR_FREE(cap);
-VIR_FREE(alloc);
-VIR_FREE(phy);
+return true;
 }
 
 
@@ -478,6 +457,9 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
 xmlNodePtr *disks = NULL;
 char *target = NULL;
 char *protocol = NULL;
+char *cap = NULL;
+char *alloc = NULL;
+char *phy = NULL;
 
 if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
 return false;
@@ -502,7 +484,10 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
 goto cleanup;
 
 /* print the title */
-cmdDomblkinfoPrint(ctl, NULL, NULL, false, true);
+vshPrintExtra(ctl, "%-10s %-15s %-15s %-15s\n", _("Target"),
+  _("Capacity"), _("Allocation"), _("Physical"));
+vshPrintExtra(ctl, "-"
+  "\n");
 
 for (i = 0; i < ndisks; i++) {
 ctxt->node = disks[i];
@@ -525,7 +510,9 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
 }
 }
 
-cmdDomblkinfoPrint(ctl, &info, target, human, false);
+if (!cmdDomblkinfoGet(ctl, &info, &cap, &alloc, &phy, human))
+goto cleanup;
+vshPrint(ctl, "%-10s %-15s %-15s %-15s\n", target, cap, alloc, 
phy);
 
 VIR_FREE(target);
 VIR_FREE(protocol);
@@ -534,12 +521,19 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
 if (virDomainGetBlockInfo(dom, device, &info, 0) < 0)
 goto cleanup;
 
-cmdDomblkinfoPrint(ctl, &info, NULL, human, false);
+if (!cmdDomblkinfoGet(ctl, &info, &cap, &alloc, &

[libvirt] [PATCH v2 07/13] virsh: Implement vshTable API to domblkinfo

[Simon Kobyda]

Signed-off-by: Simon Kobyda 
---
 tools/virsh-domain-monitor.c | 17 +++--
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index cb48f9a7be..80c7e8a99e 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -460,6 +460,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
 char *cap = NULL;
 char *alloc = NULL;
 char *phy = NULL;
+vshTablePtr table = NULL;
 
 if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
 return false;
@@ -483,11 +484,10 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
 if (ndisks < 0)
 goto cleanup;
 
-/* print the title */
-vshPrintExtra(ctl, "%-10s %-15s %-15s %-15s\n", _("Target"),
-  _("Capacity"), _("Allocation"), _("Physical"));
-vshPrintExtra(ctl, "-"
-  "\n");
+/* title */
+table = vshTableNew(_("Target"), _("Capacity"), _("Allocation"), 
_("Physical"), NULL);
+if (!table)
+goto cleanup;
 
 for (i = 0; i < ndisks; i++) {
 ctxt->node = disks[i];
@@ -512,11 +512,15 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
 
 if (!cmdDomblkinfoGet(ctl, &info, &cap, &alloc, &phy, human))
 goto cleanup;
-vshPrint(ctl, "%-10s %-15s %-15s %-15s\n", target, cap, alloc, 
phy);
+if (vshTableRowAppend(table, target, cap, alloc, phy, NULL) < 0)
+goto cleanup;
 
 VIR_FREE(target);
 VIR_FREE(protocol);
 }
+
+vshTablePrintToStdout(table, ctl);
+
 } else {
 if (virDomainGetBlockInfo(dom, device, &info, 0) < 0)
 goto cleanup;
@@ -531,6 +535,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
 ret = true;
 
  cleanup:
+vshTableFree(table);
 VIR_FREE(cap);
 VIR_FREE(alloc);
 VIR_FREE(phy);
-- 
2.17.1

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

[libvirt] [PATCH v2 05/13] virsh: Implement vshTable API to snapshot-list.

[Simon Kobyda]

Signed-off-by: Simon Kobyda 
---
 tools/virsh-snapshot.c | 33 -
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
index a4ea959230..73861957ba 100644
--- a/tools/virsh-snapshot.c
+++ b/tools/virsh-snapshot.c
@@ -41,6 +41,7 @@
 #include "virstring.h"
 #include "virxml.h"
 #include "conf/snapshot_conf.h"
+#include "vsh-table.h"
 
 /* Helper for snapshot-create and snapshot-create-as */
 static bool
@@ -1487,6 +1488,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd)
 char *parent_snap = NULL;
 virDomainSnapshotPtr start = NULL;
 virshSnapshotListPtr snaplist = NULL;
+vshTablePtr table = NULL;
 
 VSH_EXCLUSIVE_OPTIONS_VAR(tree, name);
 VSH_EXCLUSIVE_OPTIONS_VAR(parent, roots);
@@ -1547,15 +1549,12 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd)
 
 if (!tree && !name) {
 if (parent)
-vshPrintExtra(ctl, " %-20s %-25s %-15s %s",
-  _("Name"), _("Creation Time"), _("State"),
-  _("Parent"));
+table = vshTableNew(_("Name"), _("Creation Time"), _("State"), 
_("Parent"), NULL);
 else
-vshPrintExtra(ctl, " %-20s %-25s %s",
-  _("Name"), _("Creation Time"), _("State"));
-vshPrintExtra(ctl, "\n"
-   "--"
-   "--\n");
+table = vshTableNew(_("Name"), _("Creation Time"), _("State"), 
NULL);
+
+if (!table)
+goto cleanup;
 }
 
 if (tree) {
@@ -1614,13 +1613,20 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd)
 strftime(timestr, sizeof(timestr), "%Y-%m-%d %H:%M:%S %z",
  &time_info);
 
-if (parent)
-vshPrint(ctl, " %-20s %-25s %-15s %s\n",
- snap_name, timestr, state, parent_snap);
-else
-vshPrint(ctl, " %-20s %-25s %s\n", snap_name, timestr, state);
+if (parent) {
+if (vshTableRowAppend(table, snap_name, timestr, state, 
parent_snap,
+  NULL) < 0)
+goto cleanup;
+} else {
+if (vshTableRowAppend(table, snap_name, timestr, state,
+  NULL) < 0)
+goto cleanup;
+}
 }
 
+if (table)
+vshTablePrintToStdout(table, ctl);
+
 ret = true;
 
  cleanup:
@@ -1633,6 +1639,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd)
 xmlFreeDoc(xml);
 VIR_FREE(doc);
 virshDomainFree(dom);
+vshTableFree(table);
 
 return ret;
 }
-- 
2.17.1

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

[libvirt] [PATCH v2 10/13] virsh: Implement vshTable API to vcpupin, iothreadinfo, domfsinfo

[Simon Kobyda]

Signed-off-by: Simon Kobyda 
---
 tools/virsh-domain.c | 98 +++-
 1 file changed, 70 insertions(+), 28 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index c1cff9fe2d..2a416b919a 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -59,6 +59,7 @@
 #include "virxml.h"
 #include "virsh-nodedev.h"
 #include "viruri.h"
+#include "vsh-table.h"
 
 /* Gnulib doesn't guarantee SA_SIGINFO support.  */
 #ifndef SA_SIGINFO
@@ -6905,6 +6906,7 @@ virshVcpuPinQuery(vshControl *ctl,
 size_t i;
 int ncpus;
 bool ret = false;
+vshTablePtr table = NULL;
 
 if ((ncpus = virshCPUCountCollect(ctl, dom, countFlags, true)) < 0) {
 if (ncpus == -1) {
@@ -6913,7 +6915,7 @@ virshVcpuPinQuery(vshControl *ctl,
 else
 vshError(ctl, "%s", _("cannot get vcpupin for transient 
domain"));
 }
-return false;
+goto cleanup;
 }
 
 if (got_vcpu && vcpu >= ncpus) {
@@ -6927,28 +6929,39 @@ virshVcpuPinQuery(vshControl *ctl,
 vshError(ctl,
  _("vcpu %d is out of range of persistent cpu count %d"),
  vcpu, ncpus);
-return false;
+goto cleanup;
 }
 
 cpumaplen = VIR_CPU_MAPLEN(maxcpu);
 cpumap = vshMalloc(ctl, ncpus * cpumaplen);
 if ((ncpus = virDomainGetVcpuPinInfo(dom, ncpus, cpumap,
  cpumaplen, flags)) >= 0) {
-vshPrintExtra(ctl, "%s %s\n", _("VCPU:"), _("CPU Affinity"));
-vshPrintExtra(ctl, "--\n");
+table = vshTableNew(_("VCPU"), _("CPU Affinity"), NULL);
+if (!table)
+goto cleanup;
+
 for (i = 0; i < ncpus; i++) {
+VIR_AUTOFREE(char *) pinInfo = NULL;
+VIR_AUTOFREE(char *) vcpuStr = NULL;
 if (got_vcpu && i != vcpu)
 continue;
 
-vshPrint(ctl, "%4zu: ", i);
-ret = virshPrintPinInfo(ctl, VIR_GET_CPUMAP(cpumap, cpumaplen, i),
-cpumaplen);
-vshPrint(ctl, "\n");
-if (!ret)
-break;
+if (!(pinInfo = virBitmapDataFormat(cpumap, cpumaplen)))
+goto cleanup;
+
+if (virAsprintf(&vcpuStr, "%lu", i) < 0)
+goto cleanup;
+
+if (vshTableRowAppend(table, vcpuStr, pinInfo, NULL) < 0)
+goto cleanup;
 }
+
+vshTablePrintToStdout(table, ctl);
 }
 
+ret = true;
+ cleanup:
+vshTableFree(table);
 VIR_FREE(cpumap);
 return ret;
 }
@@ -7520,6 +7533,7 @@ cmdIOThreadInfo(vshControl *ctl, const vshCmd *cmd)
 int maxcpu;
 unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
 virshControlPtr priv = ctl->privData;
+vshTablePtr table = NULL;
 
 VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
 VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
@@ -7545,19 +7559,30 @@ cmdIOThreadInfo(vshControl *ctl, const vshCmd *cmd)
 goto cleanup;
 }
 
-vshPrintExtra(ctl, " %-15s %-15s\n",
-  _("IOThread ID"), _("CPU Affinity"));
-vshPrintExtra(ctl, 
"---\n");
+table = vshTableNew(_("IOThread ID"), _("CPU Affinity"), NULL);
+if (!table)
+goto cleanup;
+
 for (i = 0; i < niothreads; i++) {
+VIR_AUTOFREE(char *) pinInfo = NULL;
+VIR_AUTOFREE(char *) iothreadIdStr = NULL;
 
-vshPrint(ctl, " %-15u ", info[i]->iothread_id);
-ignore_value(virshPrintPinInfo(ctl, info[i]->cpumap, 
info[i]->cpumaplen));
-vshPrint(ctl, "\n");
-virDomainIOThreadInfoFree(info[i]);
+if (virAsprintf(&iothreadIdStr, "%u", info[i]->iothread_id) < 0)
+goto cleanup;
+
+ignore_value(pinInfo = virBitmapDataFormat(info[i]->cpumap, 
info[i]->cpumaplen));
+
+if (vshTableRowAppend(table, iothreadIdStr, pinInfo ? pinInfo : "", 
NULL) < 0)
+goto cleanup;
 }
-VIR_FREE(info);
+
+vshTablePrintToStdout(table, ctl);
 
  cleanup:
+for (i = 0; i < niothreads; i++)
+virDomainIOThreadInfoFree(info[i]);
+VIR_FREE(info);
+vshTableFree(table);
 virshDomainFree(dom);
 return niothreads >= 0;
 }
@@ -13778,6 +13803,7 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd)
 int ret = -1;
 size_t i, j;
 virDomainFSInfoPtr *info;
+vshTablePtr table = NULL;
 
 if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
 return false;
@@ -13793,25 +13819,41 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd)
 }
 
 if (info) {
-vshPrintExtra(ctl, "%-36s %-8s %-8s %s\n",
-  _("Mountpoint"), _("Name"), _("Type"), _("Target"));
-vshPrintExtra(ctl, 
"---\n");
+table = vshTableNew(_("Mountpoint"), _("Name"), _("Type"), 
_("Target"), NULL);
+if (!ta

[libvirt] [PATCH v2 00/13] Implement vsh-table API to virsh and virt-admin

[Simon Kobyda]

API is implemented in virsh and virt-admin. It fixes problems with
table-alignment, makes tables more readable and deals with unicode.
Table however could not be implemented in places, which do use simple
lists, or table implementation would change content of output,
which users might rely on.
If you know about other place where it could be implemented,
let me know.

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

Changes in v2:
- Implemented gettext function
- Fixed few possible leaks caused by inserting "goto cleanup" statements
- Implemented VIR_AUTOFREE where it's appropriate

Simon Kobyda (13):
  virsh: Implement vsh-table to iface-list
  virsh: Implement vshTable API to net-list and net-dhcp-leases
  virsh: Implement vshTable API to secret-list
  virsh: Implement vshTable API to nwfilter-list and
nwfilterbinding-list
  virsh: Implement vshTable API to snapshot-list.
  virsh: Set up cmdDomblkinfo() and cmdDomblkinfoPrint() for vshTable
API implementation
  virsh: Implement vshTable API to domblkinfo
  virsh: Implement vshTable API to domblklist
  virsh: Implement vshTable API to domiflist
  virsh: Implement vshTable API to vcpupin, iothreadinfo, domfsinfo
  virsh: Implement vshTable API to pool-list
  virsh: Implement vshTable API to vol-list
  virt-admin: Implement vshTable API to server-list and client-list

 tools/virsh-domain-monitor.c | 147 ---
 tools/virsh-domain.c |  98 +++--
 tools/virsh-interface.c  |  27 --
 tools/virsh-network.c|  59 -
 tools/virsh-nwfilter.c   |  47 +++---
 tools/virsh-pool.c   | 162 +++
 tools/virsh-secret.c |  28 --
 tools/virsh-snapshot.c   |  33 ---
 tools/virsh-volume.c | 129 ++--
 tools/virt-admin.c   |  47 +++---
 10 files changed, 371 insertions(+), 406 deletions(-)

-- 
2.17.1

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

Re: [libvirt] [PATCH 2/4] virNetSocket: Be more safe with fork() around virNetSocketDupFD()

[Marc Hartmayer]

On Fri, Sep 21, 2018 at 11:29 AM +0200, Michal Privoznik  
wrote:
> If there was a caller which would dup the client FD without
> CLOEXEC flag and later decided to change the flag it wouldn't be
> safe to do because fork() might have had occurred meantime.
> Switch to the other pattern - always dup FD with the flag set and
> let callers clear the flag if they need to do so.
>
> Signed-off-by: Michal Privoznik 
> ---

[…snip…]

>
>
> -int virNetSocketDupFD(virNetSocketPtr sock, bool cloexec)
> +int virNetSocketDupFD(virNetSocketPtr sock)

Might be useful to document this function.

>  {
>  int fd;
>
> -if (cloexec)
> -fd = fcntl(sock->fd, F_DUPFD_CLOEXEC, 0);
> -else
> -fd = dup(sock->fd);
> +fd = fcntl(sock->fd, F_DUPFD_CLOEXEC, 0);
>  if (fd < 0) {
>  virReportSystemError(errno, "%s",
>   _("Unable to copy socket file handle"));
> diff --git a/src/rpc/virnetsocket.h b/src/rpc/virnetsocket.h
> index de795af917..e6bac27566 100644
> --- a/src/rpc/virnetsocket.h
> +++ b/src/rpc/virnetsocket.h
> @@ -124,7 +124,7 @@ virNetSocketPtr 
> virNetSocketNewPostExecRestart(virJSONValuePtr object);
>  virJSONValuePtr virNetSocketPreExecRestart(virNetSocketPtr sock);
>
>  int virNetSocketGetFD(virNetSocketPtr sock);
> -int virNetSocketDupFD(virNetSocketPtr sock, bool cloexec);
> +int virNetSocketDupFD(virNetSocketPtr sock);
>
>  bool virNetSocketIsLocal(virNetSocketPtr sock);
>
> --
> 2.16.4
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>

Anyway, Reviewed-by: Marc Hartmayer 

--
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


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

Re: [libvirt] [PATCH 1/4] security: Grab a reference to virSecurityManager for transactions

[Marc Hartmayer]

On Fri, Sep 21, 2018 at 11:29 AM +0200, Michal Privoznik  
wrote:
> This shouldn't be needed per-se. Security manager shouldn't
> disappear during transactions - it's immutable. However, it
> doesn't hurt to grab a reference either - transaction code uses
> it after all.
>
> Signed-off-by: Michal Privoznik 
> ---

[…snip…]

> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index 056637e4cb..31e42afee7 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -156,6 +156,7 @@ virSecuritySELinuxContextListFree(void *opaque)
>  for (i = 0; i < list->nItems; i++)
>  virSecuritySELinuxContextItemFree(list->items[i]);
>

BTW, isn’t here VIR_FREE(list->items); missing? (see your commit
ca250269b0af24b319989807ee50f2a87b421922).

> +virObjectUnref(list->manager);
>  VIR_FREE(list);
>  }
>
> @@ -1054,12 +1055,12 @@ 
> virSecuritySELinuxTransactionStart(virSecurityManagerPtr mgr)
>  if (VIR_ALLOC(list) < 0)
>  return -1;
>
> -list->manager = mgr;
> +list->manager = virObjectRef(mgr);
>
>  if (virThreadLocalSet(&contextList, list) < 0) {
>  virReportSystemError(errno, "%s",
>   _("Unable to set thread local variable"));
> -VIR_FREE(list);
> +virSecuritySELinuxContextListFree(list);
>  return -1;
>  }
>
> --
> 2.16.4
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>

Reviewed-by: Marc Hartmayer 

--
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


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

Re: [libvirt] [PATCH 0/2] qemu: Drop QEMU_CAPS_DEVICE_SCSI_GENERIC

[Michal Privoznik]

On 09/18/2018 12:46 PM, Andrea Bolognani wrote:
> Andrea Bolognani (2):
>   tests: Fix duplicated capabilities
>   qemu: Drop QEMU_CAPS_DEVICE_SCSI_GENERIC
> 
>  src/qemu/qemu_capabilities.c  |  7 +--
>  src/qemu/qemu_capabilities.h  |  2 +-
>  src/qemu/qemu_command.c   | 61 ---
>  src/qemu/qemu_hotplug.c   | 12 
>  .../caps_1.5.3.x86_64.xml |  1 -
>  .../caps_1.6.0.x86_64.xml |  1 -
>  .../caps_1.7.0.x86_64.xml |  1 -
>  .../caps_2.1.1.x86_64.xml |  1 -
>  .../caps_2.10.0.aarch64.xml   |  1 -
>  .../caps_2.10.0.ppc64.xml |  1 -
>  .../caps_2.10.0.s390x.xml |  1 -
>  .../caps_2.10.0.x86_64.xml|  1 -
>  .../caps_2.11.0.s390x.xml |  1 -
>  .../caps_2.11.0.x86_64.xml|  1 -
>  .../caps_2.12.0.aarch64.xml   |  1 -
>  .../caps_2.12.0.ppc64.xml |  1 -
>  .../caps_2.12.0.s390x.xml |  1 -
>  .../caps_2.12.0.x86_64.xml|  1 -
>  .../caps_2.4.0.x86_64.xml |  1 -
>  .../caps_2.5.0.x86_64.xml |  1 -
>  .../caps_2.6.0.aarch64.xml|  1 -
>  .../qemucapabilitiesdata/caps_2.6.0.ppc64.xml |  1 -
>  .../caps_2.6.0.x86_64.xml |  1 -
>  .../qemucapabilitiesdata/caps_2.7.0.s390x.xml |  1 -
>  .../caps_2.7.0.x86_64.xml |  1 -
>  .../qemucapabilitiesdata/caps_2.8.0.s390x.xml |  1 -
>  .../caps_2.8.0.x86_64.xml |  1 -
>  .../qemucapabilitiesdata/caps_2.9.0.ppc64.xml |  1 -
>  .../qemucapabilitiesdata/caps_2.9.0.s390x.xml |  1 -
>  .../caps_2.9.0.x86_64.xml |  1 -
>  .../qemucapabilitiesdata/caps_3.0.0.ppc64.xml |  1 -
>  .../caps_3.0.0.riscv32.xml|  1 -
>  .../caps_3.0.0.riscv64.xml|  1 -
>  .../caps_3.0.0.x86_64.xml |  1 -
>  tests/qemuxml2argvtest.c  | 51 
>  tests/qemuxml2xmltest.c   | 55 -
>  36 files changed, 78 insertions(+), 140 deletions(-)
> 

ACK if you update qemucapabilitiesdata.

Michal

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

[libvirt] [PATCH libvirt-python v2 07/22] event-test.py: Simplify event ID lists

[Philipp Hahn]

by directly building the list with the IDs instead of appending them
explicitly.

Signed-off-by: Philipp Hahn 
---
 examples/event-test.py | 87 ++
 1 file changed, 46 insertions(+), 41 deletions(-)

diff --git a/examples/event-test.py b/examples/event-test.py
index 2dcdee3..91a7cb7 100755
--- a/examples/event-test.py
+++ b/examples/event-test.py
@@ -722,47 +722,52 @@ def main():
 
 #Add 2 lifecycle callbacks to prove this works with more than just one
 vc.domainEventRegister(myDomainEventCallback, 1)
-domcallbacks = []
-domcallbacks.append(vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_LIFECYCLE, myDomainEventCallback, 2))
-domcallbacks.append(vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_REBOOT, myDomainEventRebootCallback, None))
-domcallbacks.append(vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_RTC_CHANGE, myDomainEventRTCChangeCallback, None))
-domcallbacks.append(vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_WATCHDOG, myDomainEventWatchdogCallback, None))
-domcallbacks.append(vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_IO_ERROR, myDomainEventIOErrorCallback, None))
-domcallbacks.append(vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_GRAPHICS, myDomainEventGraphicsCallback, None))
-domcallbacks.append(vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_IO_ERROR_REASON, 
myDomainEventIOErrorReasonCallback, None))
-domcallbacks.append(vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_CONTROL_ERROR, myDomainEventControlErrorCallback, 
None))
-domcallbacks.append(vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_BLOCK_JOB, myDomainEventBlockJobCallback, None))
-domcallbacks.append(vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_DISK_CHANGE, myDomainEventDiskChangeCallback, None))
-domcallbacks.append(vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_TRAY_CHANGE, myDomainEventTrayChangeCallback, None))
-domcallbacks.append(vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_PMWAKEUP, myDomainEventPMWakeupCallback, None))
-domcallbacks.append(vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_PMSUSPEND, myDomainEventPMSuspendCallback, None))
-domcallbacks.append(vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_BALLOON_CHANGE, myDomainEventBalloonChangeCallback, 
None))
-domcallbacks.append(vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_PMSUSPEND_DISK, myDomainEventPMSuspendDiskCallback, 
None))
-domcallbacks.append(vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED, myDomainEventDeviceRemovedCallback, 
None))
-domcallbacks.append(vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2, myDomainEventBlockJob2Callback, None))
-domcallbacks.append(vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_TUNABLE, myDomainEventTunableCallback, None))
-domcallbacks.append(vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_AGENT_LIFECYCLE, 
myDomainEventAgentLifecycleCallback, None))
-domcallbacks.append(vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_DEVICE_ADDED, myDomainEventDeviceAddedCallback, 
None))
-domcallbacks.append(vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_MIGRATION_ITERATION, 
myDomainEventMigrationIteration, None))
-domcallbacks.append(vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_JOB_COMPLETED, myDomainEventJobCompletedCallback, 
None))
-domcallbacks.append(vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED, 
myDomainEventDeviceRemovalFailedCallback, None))
-domcallbacks.append(vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_METADATA_CHANGE, 
myDomainEventMetadataChangeCallback, None))
-domcallbacks.append(vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_BLOCK_THRESHOLD, 
myDomainEventBlockThresholdCallback, None))
-
-netcallbacks = []
-netcallbacks.append(vc.networkEventRegisterAny(None, 
libvirt.VIR_NETWORK_EVENT_ID_LIFECYCLE, myNetworkEventLifecycleCallback, None))
-
-poolcallbacks = []
-poolcallbacks.append(vc.storagePoolEventRegisterAny(None, 
libvirt.VIR_STORAGE_POOL_EVENT_ID_LIFECYCLE, 
myStoragePoolEventLifecycleCallback, None))
-poolcallbacks.append(vc.storagePoolEventRegisterAny(None, 
libvirt.VIR_STORAGE_POOL_EVENT_ID_REFRESH, myStoragePoolEventRefreshCallback, 
None))
-
-devcallbacks = []
-devcallbacks.append(vc.nodeDeviceEventRegisterAny(None, 
libvirt.VIR_NODE_DEVICE_EVENT_ID_LIFECYCLE, myNodeDeviceEventLifecycleCallback, 
None))
-devcallbacks.append(vc.nodeDeviceEventRegisterAny(None, 
libvirt.VIR_NODE_DEVICE_EVENT_ID_UPDATE, myNodeDeviceEventUpdateCallback, None))
-
-seccallbacks = []
-seccallbacks.append(vc.secretEventRegisterAny(None, 
libvirt.

[libvirt] [PATCH libvirt-python v2 21/22] event-test.py: Convert CONNECTION events

[Philipp Hahn]

to use new Description class

Signed-off-by: Philipp Hahn 
---
 examples/event-test.py | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/examples/event-test.py b/examples/event-test.py
index 5e3b884..1e94838 100755
--- a/examples/event-test.py
+++ b/examples/event-test.py
@@ -677,15 +677,16 @@ def mySecretEventValueChanged(conn, secret, opaque):
 ##
 
 run = True
+CONNECTION_EVENTS = Description("Error", "End-of-file", "Keepalive", "Client")
+
 
 def myConnectionCloseCallback(conn, reason, opaque):
-reasonStrings = (
-"Error", "End-of-file", "Keepalive", "Client",
-)
-print("myConnectionCloseCallback: %s: %s" % (conn.getURI(), 
reasonStrings[reason]))
+print("myConnectionCloseCallback: %s: %s" % (
+conn.getURI(), CONNECTION_EVENTS[reason]))
 global run
 run = False
 
+
 def usage():
 print("usage: %s [-hdl] [uri]" % (os.path.basename(__file__),))
 print("   uri will default to qemu:///system")
-- 
2.11.0

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

[libvirt] [PATCH libvirt-python v2 22/22] event-test.py: Fix blanks

[Philipp Hahn]

Closer to pep8

Signed-off-by: Philipp Hahn 
---
 examples/event-test.py | 97 --
 1 file changed, 71 insertions(+), 26 deletions(-)

diff --git a/examples/event-test.py b/examples/event-test.py
index 1e94838..540bf9b 100755
--- a/examples/event-test.py
+++ b/examples/event-test.py
@@ -30,11 +30,14 @@ import threading
 event_impl = "poll"
 
 do_debug = False
+
+
 def debug(msg):
 global do_debug
 if do_debug:
 print(msg)
 
+
 #
 # This general purpose event loop will support waiting for file handle
 # I/O and errors events, as well as scheduling repeatable timers with
@@ -100,7 +103,6 @@ class virEventLoopPoll:
 self.cb(self.timer,
 self.opaque)
 
-
 def __init__(self):
 self.poll = select.poll()
 self.pipetrick = os.pipe()
@@ -126,10 +128,9 @@ class virEventLoopPoll:
 # with the event loop for input events. When we need to force
 # the main thread out of a poll() sleep, we simple write a
 # single byte of data to the other end of the pipe.
-debug("Self pipe watch %d write %d" %(self.pipetrick[0], 
self.pipetrick[1]))
+debug("Self pipe watch %d write %d" % (self.pipetrick[0], 
self.pipetrick[1]))
 self.poll.register(self.pipetrick[0], select.POLLIN)
 
-
 # Calculate when the next timeout is due to occur, returning
 # the absolute timestamp for the next timeout, or 0 if there is
 # no timeout due
@@ -159,7 +160,6 @@ class virEventLoopPoll:
 return h
 return None
 
-
 # This is the heart of the event loop, performing one single
 # iteration. It asks when the next timeout is due, and then
 # calculates the maximum amount of time it is able to sleep
@@ -235,7 +235,6 @@ class virEventLoopPoll:
 finally:
 self.runningPoll = False
 
-
 # Actually run the event loop forever
 def run_loop(self):
 self.quit = False
@@ -247,7 +246,6 @@ class virEventLoopPoll:
 self.pendingWakeup = True
 os.write(self.pipetrick[1], 'c'.encode("UTF-8"))
 
-
 # Registers a new file handle 'fd', monitoring  for 'events' (libvirt
 # event constants), firing the callback  cb() when an event occurs.
 # Returns a unique integer identier for this handle, that should be
@@ -301,7 +299,7 @@ class virEventLoopPoll:
 h.set_interval(interval)
 self.interrupt()
 
-debug("Update timer %d interval %d"  % (timerID, interval))
+debug("Update timer %d interval %d" % (timerID, interval))
 break
 
 # Stop monitoring for events on the file handle
@@ -383,26 +381,32 @@ def virEventAddHandleImpl(fd, events, cb, opaque):
 global eventLoop
 return eventLoop.add_handle(fd, events, cb, opaque)
 
+
 def virEventUpdateHandleImpl(handleID, events):
 global eventLoop
 return eventLoop.update_handle(handleID, events)
 
+
 def virEventRemoveHandleImpl(handleID):
 global eventLoop
 return eventLoop.remove_handle(handleID)
 
+
 def virEventAddTimerImpl(interval, cb, opaque):
 global eventLoop
 return eventLoop.add_timer(interval, cb, opaque)
 
+
 def virEventUpdateTimerImpl(timerID, interval):
 global eventLoop
 return eventLoop.update_timer(timerID, interval)
 
+
 def virEventRemoveTimerImpl(timerID):
 global eventLoop
 return eventLoop.remove_timer(timerID)
 
+
 # This tells libvirt what event loop implementation it
 # should use
 def virEventLoopPollRegister():
@@ -413,20 +417,24 @@ def virEventLoopPollRegister():
  virEventUpdateTimerImpl,
  virEventRemoveTimerImpl)
 
+
 # Directly run the event loop in the current thread
 def virEventLoopPollRun():
 global eventLoop
 eventLoop.run_loop()
 
+
 def virEventLoopAIORun(loop):
 import asyncio
 asyncio.set_event_loop(loop)
 loop.run_forever()
 
+
 def virEventLoopNativeRun():
 while True:
 libvirt.virEventRunDefaultImpl()
 
+
 # Spawn a background thread to run the event loop
 def virEventLoopPollStart():
 global eventLoopThread
@@ -435,6 +443,7 @@ def virEventLoopPollStart():
 eventLoopThread.setDaemon(True)
 eventLoopThread.start()
 
+
 def virEventLoopAIOStart():
 global eventLoopThread
 import libvirtaio
@@ -445,6 +454,7 @@ def virEventLoopAIOStart():
 eventLoopThread.setDaemon(True)
 eventLoopThread.start()
 
+
 def virEventLoopNativeStart():
 global eventLoopThread
 libvirt.virEventRegisterDefaultImpl()
@@ -509,10 +519,13 @@ def myDomainEventCallback(conn, dom, event, detail, 
opaque):
 
 
 def myDomainEventRebootCallback(conn, dom, opaque):
-print("myDomainEventRebootCallback: Domain %s(%s)" % (dom.name(), 
dom.ID()))
+print("myDomainEventRebootCallback: Domain %s(%s)" % (
+dom.name(), dom.ID()))
+
 
 def myDomainEventRTCChangeCallback(conn, dom, utcoffset, opaq

[libvirt] [PATCH libvirt-python v2 13/22] event-test.py: Convert AGENT events

[Philipp Hahn]

to use new Description class

Signed-off-by: Philipp Hahn 
---
 examples/event-test.py | 17 -
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/examples/event-test.py b/examples/event-test.py
index 4a801b7..b559ede 100755
--- a/examples/event-test.py
+++ b/examples/event-test.py
@@ -496,14 +496,8 @@ BLOCK_JOB_TYPES = Description("unknown", "Pull", "Copy", 
"Commit", "ActiveCommit
 BLOCK_JOB_STATUS = Description("Completed", "Failed", "Canceled", "Ready")
 WATCHDOG_ACTIONS = Description("none", "Pause", "Reset", "Poweroff", 
"Shutdown", "Debug", "Inject NMI")
 ERROR_EVENTS = Description("None", "Pause", "Report")
-
-def agentLifecycleStateToString(state):
-agentStates = ( "unknown", "connected", "disconnected", )
-return agentStates[state]
-
-def agentLifecycleReasonToString(reason):
-agentReasons = ( "unknown", "domain started", "channel event", )
-return agentReasons[reason]
+AGENT_STATES = Description("unknown", "connected", "disconnected")
+AGENT_REASONS = Description("unknown", "domain started", "channel event")
 
 
 def myDomainEventCallback(conn, dom, event, detail, opaque):
@@ -572,8 +566,13 @@ def myDomainEventBlockJob2Callback(conn, dom, disk, type, 
status, opaque):
 
 def myDomainEventTunableCallback(conn, dom, params, opaque):
 print("myDomainEventTunableCallback: Domain %s(%s) %s" % (dom.name(), 
dom.ID(), params))
+
+
 def myDomainEventAgentLifecycleCallback(conn, dom, state, reason, opaque):
-print("myDomainEventAgentLifecycleCallback: Domain %s(%s) %s %s" % 
(dom.name(), dom.ID(), agentLifecycleStateToString(state), 
agentLifecycleReasonToString(reason)))
+print("myDomainEventAgentLifecycleCallback: Domain %s(%s) %s %s" % (
+dom.name(), dom.ID(), AGENT_STATES[state], AGENT_REASONS[reason]))
+
+
 def myDomainEventDeviceAddedCallback(conn, dom, dev, opaque):
 print("myDomainEventDeviceAddedCallback: Domain %s(%s) device added: %s" % 
(
 dom.name(), dom.ID(), dev))
-- 
2.11.0

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

[libvirt] [PATCH libvirt-python v2 08/22] event-test.py: Add class for event descriptions

[Philipp Hahn]

Signed-off-by: Philipp Hahn 
---
 examples/event-test.py | 25 +
 1 file changed, 25 insertions(+)

diff --git a/examples/event-test.py b/examples/event-test.py
index 91a7cb7..d2d2c60 100755
--- a/examples/event-test.py
+++ b/examples/event-test.py
@@ -456,6 +456,31 @@ def virEventLoopNativeStart():
 ##
 # Everything that now follows is a simple demo of domain lifecycle events
 ##
+class Description(object):
+__slots__ = ('desc', 'args')
+
+def __init__(self, *args, **kwargs):
+self.desc = kwargs.get('desc')
+self.args = args
+
+def __str__(self):  # type: () -> str
+return self.desc
+
+def __getitem__(self, item):  # type: (int) -> str
+try:
+data = self.args[item]
+except IndexError:
+return self.__class__(desc=str(item))
+
+if isinstance(data, str):
+return self.__class__(desc=data)
+elif isinstance(data, (list, tuple)):
+desc, args = data
+return self.__class__(*args, desc=desc)
+
+raise TypeError(args)
+
+
 def domEventToString(event):
 domEventStrings = ( "Defined",
  "Undefined",
-- 
2.11.0

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

[libvirt] [PATCH libvirt-python v2 16/22] event-test.py: Convert TRAY events

[Philipp Hahn]

to use new Description class

Signed-off-by: Philipp Hahn 
---
 examples/event-test.py | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/examples/event-test.py b/examples/event-test.py
index 218103d..5426ecd 100755
--- a/examples/event-test.py
+++ b/examples/event-test.py
@@ -500,6 +500,7 @@ AGENT_STATES = Description("unknown", "connected", 
"disconnected")
 AGENT_REASONS = Description("unknown", "domain started", "channel event")
 GRAPHICS_PHASES = Description("Connect", "Initialize", "Disconnect")
 DISK_EVENTS = Description("Change missing on start", "Drop missing on start")
+TRAY_EVENTS = Description("Opened", "Closed")
 
 
 def myDomainEventCallback(conn, dom, event, detail, opaque):
@@ -549,7 +550,9 @@ def myDomainEventDiskChangeCallback(conn, dom, oldSrcPath, 
newSrcPath, devAlias,
 
 def myDomainEventTrayChangeCallback(conn, dom, devAlias, reason, opaque):
 print("myDomainEventTrayChangeCallback: Domain %s(%s) tray change 
devAlias: %s reason: %s" % (
-dom.name(), dom.ID(), devAlias, reason))
+dom.name(), dom.ID(), devAlias, TRAY_EVENTS[reason]))
+
+
 def myDomainEventPMWakeupCallback(conn, dom, reason, opaque):
 print("myDomainEventPMWakeupCallback: Domain %s(%s) system pmwakeup" % (
 dom.name(), dom.ID()))
-- 
2.11.0

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

[libvirt] [PATCH libvirt-python v2 12/22] event-test.py: Convert ERROR events

[Philipp Hahn]

to use new Description class

Signed-off-by: Philipp Hahn 
---
 examples/event-test.py | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/examples/event-test.py b/examples/event-test.py
index 004a263..4a801b7 100755
--- a/examples/event-test.py
+++ b/examples/event-test.py
@@ -495,6 +495,7 @@ DOM_EVENTS = Description(
 BLOCK_JOB_TYPES = Description("unknown", "Pull", "Copy", "Commit", 
"ActiveCommit")
 BLOCK_JOB_STATUS = Description("Completed", "Failed", "Canceled", "Ready")
 WATCHDOG_ACTIONS = Description("none", "Pause", "Reset", "Poweroff", 
"Shutdown", "Debug", "Inject NMI")
+ERROR_EVENTS = Description("None", "Pause", "Report")
 
 def agentLifecycleStateToString(state):
 agentStates = ( "unknown", "connected", "disconnected", )
@@ -524,8 +525,13 @@ def myDomainEventWatchdogCallback(conn, dom, action, 
opaque):
 
 def myDomainEventIOErrorCallback(conn, dom, srcpath, devalias, action, opaque):
 print("myDomainEventIOErrorCallback: Domain %s(%s) %s %s %d" % 
(dom.name(), dom.ID(), srcpath, devalias, action))
+
+
 def myDomainEventIOErrorReasonCallback(conn, dom, srcpath, devalias, action, 
reason, opaque):
-print("myDomainEventIOErrorReasonCallback: Domain %s(%s) %s %s %d %s" % 
(dom.name(), dom.ID(), srcpath, devalias, action, reason))
+print("myDomainEventIOErrorReasonCallback: Domain %s(%s) %s %s %d %s" % (
+dom.name(), dom.ID(), srcpath, devalias, action, ERROR_EVENTS[reason]))
+
+
 def myDomainEventGraphicsCallback(conn, dom, phase, localAddr, remoteAddr, 
authScheme, subject, opaque):
 print("myDomainEventGraphicsCallback: Domain %s(%s) %d %s" % (dom.name(), 
dom.ID(), phase, authScheme))
 def myDomainEventControlErrorCallback(conn, dom, opaque):
-- 
2.11.0

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

[libvirt] [PATCH libvirt-python v2 14/22] event-test.py: Convert GRAPHICS events

[Philipp Hahn]

to use new Description class

Signed-off-by: Philipp Hahn 
---
 examples/event-test.py | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/examples/event-test.py b/examples/event-test.py
index b559ede..d8ba8c9 100755
--- a/examples/event-test.py
+++ b/examples/event-test.py
@@ -498,6 +498,7 @@ WATCHDOG_ACTIONS = Description("none", "Pause", "Reset", 
"Poweroff", "Shutdown",
 ERROR_EVENTS = Description("None", "Pause", "Report")
 AGENT_STATES = Description("unknown", "connected", "disconnected")
 AGENT_REASONS = Description("unknown", "domain started", "channel event")
+GRAPHICS_PHASES = Description("Connect", "Initialize", "Disconnect")
 
 
 def myDomainEventCallback(conn, dom, event, detail, opaque):
@@ -527,7 +528,10 @@ def myDomainEventIOErrorReasonCallback(conn, dom, srcpath, 
devalias, action, rea
 
 
 def myDomainEventGraphicsCallback(conn, dom, phase, localAddr, remoteAddr, 
authScheme, subject, opaque):
-print("myDomainEventGraphicsCallback: Domain %s(%s) %d %s" % (dom.name(), 
dom.ID(), phase, authScheme))
+print("myDomainEventGraphicsCallback: Domain %s(%s) %s %s" % (
+dom.name(), dom.ID(), GRAPHICS_PHASES[phase], authScheme))
+
+
 def myDomainEventControlErrorCallback(conn, dom, opaque):
 print("myDomainEventControlErrorCallback: Domain %s(%s)" % (dom.name(), 
dom.ID()))
 
-- 
2.11.0

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

[libvirt] [PATCH libvirt-python v2 02/22] event-test.py: Remove extra parenthesis

[Philipp Hahn]

Signed-off-by: Philipp Hahn 
---
 examples/event-test.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/examples/event-test.py b/examples/event-test.py
index c17d2bb..a7c7054 100755
--- a/examples/event-test.py
+++ b/examples/event-test.py
@@ -224,7 +224,7 @@ class virEventLoopPoll:
 want = t.get_last_fired() + interval
 # Deduct 20ms, since scheduler timeslice
 # means we could be ever so slightly early
-if now >= (want-20):
+if now >= want - 20:
 debug("Dispatch timer %d now %s want %s" % (t.get_id(), 
str(now), str(want)))
 t.set_last_fired(now)
 t.dispatch()
-- 
2.11.0

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

[libvirt] [PATCH libvirt-python v2 15/22] event-test.py: Convert DISK events

[Philipp Hahn]

to use new Description class

Signed-off-by: Philipp Hahn 
---
 examples/event-test.py | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/examples/event-test.py b/examples/event-test.py
index d8ba8c9..218103d 100755
--- a/examples/event-test.py
+++ b/examples/event-test.py
@@ -499,6 +499,7 @@ ERROR_EVENTS = Description("None", "Pause", "Report")
 AGENT_STATES = Description("unknown", "connected", "disconnected")
 AGENT_REASONS = Description("unknown", "domain started", "channel event")
 GRAPHICS_PHASES = Description("Connect", "Initialize", "Disconnect")
+DISK_EVENTS = Description("Change missing on start", "Drop missing on start")
 
 
 def myDomainEventCallback(conn, dom, event, detail, opaque):
@@ -543,7 +544,9 @@ def myDomainEventBlockJobCallback(conn, dom, disk, type, 
status, opaque):
 
 def myDomainEventDiskChangeCallback(conn, dom, oldSrcPath, newSrcPath, 
devAlias, reason, opaque):
 print("myDomainEventDiskChangeCallback: Domain %s(%s) disk change 
oldSrcPath: %s newSrcPath: %s devAlias: %s reason: %s" % (
-dom.name(), dom.ID(), oldSrcPath, newSrcPath, devAlias, reason))
+dom.name(), dom.ID(), oldSrcPath, newSrcPath, devAlias, 
DISK_EVENTS[reason]))
+
+
 def myDomainEventTrayChangeCallback(conn, dom, devAlias, reason, opaque):
 print("myDomainEventTrayChangeCallback: Domain %s(%s) tray change 
devAlias: %s reason: %s" % (
 dom.name(), dom.ID(), devAlias, reason))
-- 
2.11.0

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

[libvirt] [PATCH libvirt-python v2 00/22] event-test.py fixes

[Philipp Hahn]

Am 21.09.18 um 13:14 schrieb Michal Privoznik:
> On 09/20/2018 08:10 AM, Philipp Hahn wrote:
>> event-test.py is bad at handling newer livecycle events.
>> Attached are two patches to improve that.
>>
>> Philipp Hahn (2):
>>   event-test.py: Sync list of domain lifecycle events
>>   event-test.py: Future proof lifecycle event handling
>>
>>  examples/event-test.py | 18 --
>>  1 file changed, 12 insertions(+), 6 deletions(-)
>
> ACK to the first patch, no doubt about that. However, the second one -
> I'm torn. One one hand it makes the code more future proof, on the other
> - seeing a traceback (which is easy to spot) might inspire users to post
> patches (because we are obviously not doing a good job in keeping
> event-test.py in sync with libvirt). What do you think?

Yes, I can understand it, but each time I use event-test.py (spaced months
apart) I get the next crash. So I really would like it to be more future poof.
If the number is annoying enough, any interested user is free so send a patch
to improve it.

As the pattern of printing some details is quiet common in that script, I did a
v2 and added a somehow hacky class 'Description' to simplify writing those
descriptive texts. Please have a look at the following series as an alternative.

I also found some other issues I fixed along the way.

Philipp Hahn (22):
  event-test.py: Handle closed connection
  event-test.py: Remove extra parenthesis
  event-test.py: Remove dead assignment
  event-test.py: Add missing globale statement
  event-test.py: Use __file__
  event-test.py: Merge livecycle callbacks
  event-test.py: Simplify event ID lists
  event-test.py: Add class for event descriptions
  event-test.py: Convert LIVECYCLE events
  event-test.py: Convert BLOCKJOB events
  event-test.py: Convert WATCHDOG events
  event-test.py: Convert ERROR events
  event-test.py: Convert AGENT events
  event-test.py: Convert GRAPHICS events
  event-test.py: Convert DISK events
  event-test.py: Convert TRAY events
  event-test.py: Convert NETWORK events
  event-test.py: Convert STORAGE events
  event-test.py: Convert DEVICE events
  event-test.py: Convert SECRET events
  event-test.py: Convert CONNECTION events
  event-test.py: Fix blanks

 examples/event-test.py | 446 +
 1 file changed, 262 insertions(+), 184 deletions(-)

-- 
2.11.0

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

[libvirt] [PATCH libvirt-python v2 17/22] event-test.py: Convert NETWORK events

[Philipp Hahn]

to use new Description class

Signed-off-by: Philipp Hahn 
---
 examples/event-test.py | 29 ++---
 1 file changed, 10 insertions(+), 19 deletions(-)

diff --git a/examples/event-test.py b/examples/event-test.py
index 5426ecd..2436827 100755
--- a/examples/event-test.py
+++ b/examples/event-test.py
@@ -604,27 +604,18 @@ def myDomainEventBlockThresholdCallback(conn, dom, dev, 
path, threshold, excess,
 ##
 # Network events
 ##
-def netEventToString(event):
-netEventStrings = ( "Defined",
- "Undefined",
- "Started",
- "Stopped",
-)
-return netEventStrings[event]
-
-def netDetailToString(event, detail):
-netEventStrings = (
-( "Added", ),
-( "Removed", ),
-( "Started", ),
-( "Stopped", ),
-)
-return netEventStrings[event][detail]
+NET_EVENTS = Description(
+("Defined", ("Added",)),
+("Undefined", ("Removed",)),
+("Started", ("Started",)),
+("Stopped", ("Stopped",)),
+)
+
 
 def myNetworkEventLifecycleCallback(conn, net, event, detail, opaque):
-print("myNetworkEventLifecycleCallback: Network %s %s %s" % (net.name(),
- 
netEventToString(event),
- 
netDetailToString(event, detail)))
+print("myNetworkEventLifecycleCallback: Network %s %s %s" % (
+net.name(), NET_EVENTS[event], NET_EVENTS[event][detail]))
+
 
 ##
 # Storage pool events
-- 
2.11.0

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

[libvirt] [PATCH libvirt-python v2 18/22] event-test.py: Convert STORAGE events

[Philipp Hahn]

to use new Description class

Signed-off-by: Philipp Hahn 
---
 examples/event-test.py | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/examples/event-test.py b/examples/event-test.py
index 2436827..499f434 100755
--- a/examples/event-test.py
+++ b/examples/event-test.py
@@ -620,20 +620,20 @@ def myNetworkEventLifecycleCallback(conn, net, event, 
detail, opaque):
 ##
 # Storage pool events
 ##
-def storageEventToString(event):
-storageEventStrings = ( "Defined",
-"Undefined",
-"Started",
-"Stopped",
-"Created",
-"Deleted",
-)
-return storageEventStrings[event]
+STORAGE_EVENTS = Description(
+("Defined", ()),
+("Undefined", ()),
+("Started", ()),
+("Stopped", ()),
+("Created", ()),
+("Deleted", ()),
+)
+
 
 def myStoragePoolEventLifecycleCallback(conn, pool, event, detail, opaque):
-print("myStoragePoolEventLifecycleCallback: Storage pool %s %s %d" % 
(pool.name(),
-  
storageEventToString(event),
-  
detail))
+print("myStoragePoolEventLifecycleCallback: Storage pool %s %s %s" % (
+pool.name(), STORAGE_EVENTS[event], STORAGE_EVENTS[event][detail]))
+
 
 def myStoragePoolEventRefreshCallback(conn, pool, opaque):
 print("myStoragePoolEventRefreshCallback: Storage pool %s" % pool.name())
-- 
2.11.0

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

[libvirt] [PATCH libvirt-python v2 06/22] event-test.py: Merge livecycle callbacks

[Philipp Hahn]

Registering the same function twice using the old domainEventRegister()
interface would not work, as the function reference is used for
un-registering.

But it is not a problem with the new interface domainEventRegisterAn(),
as that returns a unique ID.

While at it also demonstrate the 'opaque' mechanism.

Signed-off-by: Philipp Hahn 
---
 examples/event-test.py | 18 +++---
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/examples/event-test.py b/examples/event-test.py
index ab1da4a..2dcdee3 100755
--- a/examples/event-test.py
+++ b/examples/event-test.py
@@ -499,15 +499,11 @@ def agentLifecycleReasonToString(reason):
 agentReasons = ( "unknown", "domain started", "channel event", )
 return agentReasons[reason]
 
-def myDomainEventCallback1 (conn, dom, event, detail, opaque):
-print("myDomainEventCallback1 EVENT: Domain %s(%s) %s %s" % (dom.name(), 
dom.ID(),
- 
domEventToString(event),
- 
domDetailToString(event, detail)))
 
-def myDomainEventCallback2 (conn, dom, event, detail, opaque):
-print("myDomainEventCallback2 EVENT: Domain %s(%s) %s %s" % (dom.name(), 
dom.ID(),
- 
domEventToString(event),
- 
domDetailToString(event, detail)))
+def myDomainEventCallback(conn, dom, event, detail, opaque):
+print("myDomainEventCallback%d EVENT: Domain %s(%s) %s %s" % (
+opaque, dom.name(), dom.ID(), domEventToString(event), 
domDetailToString(event, detail)))
+
 
 def myDomainEventRebootCallback(conn, dom, opaque):
 print("myDomainEventRebootCallback: Domain %s(%s)" % (dom.name(), 
dom.ID()))
@@ -725,9 +721,9 @@ def main():
 vc.registerCloseCallback(myConnectionCloseCallback, None)
 
 #Add 2 lifecycle callbacks to prove this works with more than just one
-vc.domainEventRegister(myDomainEventCallback1,None)
+vc.domainEventRegister(myDomainEventCallback, 1)
 domcallbacks = []
-domcallbacks.append(vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_LIFECYCLE, myDomainEventCallback2, None))
+domcallbacks.append(vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_LIFECYCLE, myDomainEventCallback, 2))
 domcallbacks.append(vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_REBOOT, myDomainEventRebootCallback, None))
 domcallbacks.append(vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_RTC_CHANGE, myDomainEventRTCChangeCallback, None))
 domcallbacks.append(vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_WATCHDOG, myDomainEventWatchdogCallback, None))
@@ -784,7 +780,7 @@ def main():
 if not run:
 return
 
-vc.domainEventDeregister(myDomainEventCallback1)
+vc.domainEventDeregister(myDomainEventCallback)
 
 for id in seccallbacks:
 vc.secretEventDeregisterAny(id)
-- 
2.11.0

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

[libvirt] [PATCH libvirt-python v2 05/22] event-test.py: Use __file__

[Philipp Hahn]

instead of sys.argv[0]

Signed-off-by: Philipp Hahn 
---
 examples/event-test.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/examples/event-test.py b/examples/event-test.py
index 646ce50..ab1da4a 100755
--- a/examples/event-test.py
+++ b/examples/event-test.py
@@ -667,7 +667,7 @@ def myConnectionCloseCallback(conn, reason, opaque):
 run = False
 
 def usage():
-print("usage: "+os.path.basename(sys.argv[0])+" [-hdl] [uri]")
+print("usage: %s [-hdl] [uri]" % (os.path.basename(__file__),))
 print("   uri will default to qemu:///system")
 print("   --help, -h   Print this help message")
 print("   --debug, -d  Print debug output")
-- 
2.11.0

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

[libvirt] [PATCH libvirt-python v2 09/22] event-test.py: Convert LIVECYCLE events

[Philipp Hahn]

to use new Description class

Signed-off-by: Philipp Hahn 
---
 examples/event-test.py | 42 ++
 1 file changed, 14 insertions(+), 28 deletions(-)

diff --git a/examples/event-test.py b/examples/event-test.py
index d2d2c60..493828f 100755
--- a/examples/event-test.py
+++ b/examples/event-test.py
@@ -481,32 +481,18 @@ class Description(object):
 raise TypeError(args)
 
 
-def domEventToString(event):
-domEventStrings = ( "Defined",
- "Undefined",
- "Started",
- "Suspended",
- "Resumed",
- "Stopped",
- "Shutdown",
- "PMSuspended",
- "Crashed",
-)
-return domEventStrings[event]
-
-def domDetailToString(event, detail):
-domEventStrings = (
-( "Added", "Updated", "Renamed", "Snapshot" ),
-( "Removed", "Renamed", ),
-( "Booted", "Migrated", "Restored", "Snapshot", "Wakeup" ),
-( "Paused", "Migrated", "IOError", "Watchdog", "Restored", "Snapshot", 
"API error", "Postcopy", "Postcopy failed" ),
-( "Unpaused", "Migrated", "Snapshot", "Postcopy" ),
-( "Shutdown", "Destroyed", "Crashed", "Migrated", "Saved", "Failed", 
"Snapshot"),
-( "Finished", "On guest request", "On host request"),
-( "Memory", "Disk" ),
-( "Panicked", ),
-)
-return domEventStrings[event][detail]
+DOM_EVENTS = Description(
+("Defined", ("Added", "Updated", "Renamed", "Snapshot")),
+("Undefined", ("Removed", "Renamed")),
+("Started", ("Booted", "Migrated", "Restored", "Snapshot", "Wakeup")),
+("Suspended", ("Paused", "Migrated", "IOError", "Watchdog", "Restored", 
"Snapshot", "API error", "Postcopy", "Postcopy failed")),
+("Resumed", ("Unpaused", "Migrated", "Snapshot", "Postcopy")),
+("Stopped", ("Shutdown", "Destroyed", "Crashed", "Migrated", "Saved", 
"Failed", "Snapshot")),
+("Shutdown", ("Finished", "On guest request", "On host request")),
+("PMSuspended", ("Memory", "Disk")),
+("Crashed", ("Panicked",)),
+)
+
 
 def blockJobTypeToString(type):
 blockJobTypes = ( "unknown", "Pull", "Copy", "Commit", "ActiveCommit", )
@@ -526,8 +512,8 @@ def agentLifecycleReasonToString(reason):
 
 
 def myDomainEventCallback(conn, dom, event, detail, opaque):
-print("myDomainEventCallback%d EVENT: Domain %s(%s) %s %s" % (
-opaque, dom.name(), dom.ID(), domEventToString(event), 
domDetailToString(event, detail)))
+print("myDomainEventCallback%s EVENT: Domain %s(%s) %s %s" % (
+opaque, dom.name(), dom.ID(), DOM_EVENTS[event], 
DOM_EVENTS[event][detail]))
 
 
 def myDomainEventRebootCallback(conn, dom, opaque):
-- 
2.11.0

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

[libvirt] [PATCH libvirt-python v2 19/22] event-test.py: Convert DEVICE events

[Philipp Hahn]

to use new Description class

Signed-off-by: Philipp Hahn 
---
 examples/event-test.py | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/examples/event-test.py b/examples/event-test.py
index 499f434..0a1d06d 100755
--- a/examples/event-test.py
+++ b/examples/event-test.py
@@ -641,16 +641,16 @@ def myStoragePoolEventRefreshCallback(conn, pool, opaque):
 ##
 # Node device events
 ##
-def nodeDeviceEventToString(event):
-nodeDeviceEventStrings = ( "Created",
-   "Deleted",
-)
-return nodeDeviceEventStrings[event]
+DEVICE_EVENTS = Description(
+("Created", ()),
+("Deleted", ()),
+)
+
 
 def myNodeDeviceEventLifecycleCallback(conn, dev, event, detail, opaque):
-print("myNodeDeviceEventLifecycleCallback: Node device  %s %s %d" % 
(dev.name(),
-  
nodeDeviceEventToString(event),
-  
detail))
+print("myNodeDeviceEventLifecycleCallback: Node device  %s %s %s" % (
+dev.name(), DEVICE_EVENTS[event], DEVICE_EVENTS[event][detail]))
+
 
 def myNodeDeviceEventUpdateCallback(conn, dev, opaque):
 print("myNodeDeviceEventUpdateCallback: Node device %s" % dev.name())
-- 
2.11.0

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

[libvirt] [PATCH libvirt-python v2 20/22] event-test.py: Convert SECRET events

[Philipp Hahn]

to use new Description class

Signed-off-by: Philipp Hahn 
---
 examples/event-test.py | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/examples/event-test.py b/examples/event-test.py
index 0a1d06d..5e3b884 100755
--- a/examples/event-test.py
+++ b/examples/event-test.py
@@ -658,16 +658,16 @@ def myNodeDeviceEventUpdateCallback(conn, dev, opaque):
 ##
 # Secret events
 ##
-def secretEventToString(event):
-secretEventStrings = ( "Defined",
-   "Undefined",
-)
-return secretEventStrings[event]
+SECRET_EVENTS = Description(
+("Defined", ()),
+("Undefined", ()),
+)
+
 
 def mySecretEventLifecycleCallback(conn, secret, event, detail, opaque):
-print("mySecretEventLifecycleCallback: Secret %s %s %d" % 
(secret.UUIDString(),
-   
secretEventToString(event),
-   detail))
+print("mySecretEventLifecycleCallback: Secret %s %s %s" % (
+secret.UUIDString(), SECRET_EVENTS[event], 
SECRET_EVENTS[event][detail]))
+
 
 def mySecretEventValueChanged(conn, secret, opaque):
 print("mySecretEventValueChanged: Secret %s" % secret.UUIDString())
-- 
2.11.0

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

[libvirt] [PATCH libvirt-python v2 10/22] event-test.py: Convert BLOCKJOB events

[Philipp Hahn]

to use new Description class

Signed-off-by: Philipp Hahn 
---
 examples/event-test.py | 25 ++---
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/examples/event-test.py b/examples/event-test.py
index 493828f..46a8db8 100755
--- a/examples/event-test.py
+++ b/examples/event-test.py
@@ -492,15 +492,8 @@ DOM_EVENTS = Description(
 ("PMSuspended", ("Memory", "Disk")),
 ("Crashed", ("Panicked",)),
 )
-
-
-def blockJobTypeToString(type):
-blockJobTypes = ( "unknown", "Pull", "Copy", "Commit", "ActiveCommit", )
-return blockJobTypes[type]
-
-def blockJobStatusToString(status):
-blockJobStatus = ( "Completed", "Failed", "Canceled", "Ready", )
-return blockJobStatus[status]
+BLOCK_JOB_TYPES = Description("unknown", "Pull", "Copy", "Commit", 
"ActiveCommit")
+BLOCK_JOB_STATUS = Description("Completed", "Failed", "Canceled", "Ready")
 
 def agentLifecycleStateToString(state):
 agentStates = ( "unknown", "connected", "disconnected", )
@@ -533,8 +526,13 @@ def myDomainEventGraphicsCallback(conn, dom, phase, 
localAddr, remoteAddr, authS
 print("myDomainEventGraphicsCallback: Domain %s(%s) %d %s" % (dom.name(), 
dom.ID(), phase, authScheme))
 def myDomainEventControlErrorCallback(conn, dom, opaque):
 print("myDomainEventControlErrorCallback: Domain %s(%s)" % (dom.name(), 
dom.ID()))
+
+
 def myDomainEventBlockJobCallback(conn, dom, disk, type, status, opaque):
-print("myDomainEventBlockJobCallback: Domain %s(%s) %s on disk %s %s" % 
(dom.name(), dom.ID(), blockJobTypeToString(type), disk, 
blockJobStatusToString(status)))
+print("myDomainEventBlockJobCallback: Domain %s(%s) %s on disk %s %s" % (
+dom.name(), dom.ID(), BLOCK_JOB_TYPES[type], disk, 
BLOCK_JOB_STATUS[status]))
+
+
 def myDomainEventDiskChangeCallback(conn, dom, oldSrcPath, newSrcPath, 
devAlias, reason, opaque):
 print("myDomainEventDiskChangeCallback: Domain %s(%s) disk change 
oldSrcPath: %s newSrcPath: %s devAlias: %s reason: %s" % (
 dom.name(), dom.ID(), oldSrcPath, newSrcPath, devAlias, reason))
@@ -555,8 +553,13 @@ def myDomainEventPMSuspendDiskCallback(conn, dom, reason, 
opaque):
 def myDomainEventDeviceRemovedCallback(conn, dom, dev, opaque):
 print("myDomainEventDeviceRemovedCallback: Domain %s(%s) device removed: 
%s" % (
 dom.name(), dom.ID(), dev))
+
+
 def myDomainEventBlockJob2Callback(conn, dom, disk, type, status, opaque):
-print("myDomainEventBlockJob2Callback: Domain %s(%s) %s on disk %s %s" % 
(dom.name(), dom.ID(), blockJobTypeToString(type), disk, 
blockJobStatusToString(status)))
+print("myDomainEventBlockJob2Callback: Domain %s(%s) %s on disk %s %s" % (
+dom.name(), dom.ID(), BLOCK_JOB_TYPES[type], disk, 
BLOCK_JOB_STATUS[status]))
+
+
 def myDomainEventTunableCallback(conn, dom, params, opaque):
 print("myDomainEventTunableCallback: Domain %s(%s) %s" % (dom.name(), 
dom.ID(), params))
 def myDomainEventAgentLifecycleCallback(conn, dom, state, reason, opaque):
-- 
2.11.0

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

[libvirt] [PATCH libvirt-python v2 03/22] event-test.py: Remove dead assignment

[Philipp Hahn]

variable is unused

Signed-off-by: Philipp Hahn 
---
 examples/event-test.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/examples/event-test.py b/examples/event-test.py
index a7c7054..1f34930 100755
--- a/examples/event-test.py
+++ b/examples/event-test.py
@@ -207,7 +207,7 @@ class virEventLoopPoll:
 # the data just continue
 if fd == self.pipetrick[0]:
 self.pendingWakeup = False
-data = os.read(fd, 1)
+os.read(fd, 1)
 continue
 
 h = self.get_handle_by_fd(fd)
-- 
2.11.0

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

[libvirt] [PATCH libvirt-python v2 04/22] event-test.py: Add missing globale statement

[Philipp Hahn]

to fix loop termination on exit.

Signed-off-by: Philipp Hahn 
---
 examples/event-test.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/examples/event-test.py b/examples/event-test.py
index 1f34930..646ce50 100755
--- a/examples/event-test.py
+++ b/examples/event-test.py
@@ -663,6 +663,7 @@ def myConnectionCloseCallback(conn, reason, opaque):
 "Error", "End-of-file", "Keepalive", "Client",
 )
 print("myConnectionCloseCallback: %s: %s" % (conn.getURI(), 
reasonStrings[reason]))
+global run
 run = False
 
 def usage():
-- 
2.11.0

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

[libvirt] [PATCH libvirt-python v2 11/22] event-test.py: Convert WATCHDOG events

[Philipp Hahn]

to use new Description class

Signed-off-by: Philipp Hahn 
---
 examples/event-test.py | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/examples/event-test.py b/examples/event-test.py
index 46a8db8..004a263 100755
--- a/examples/event-test.py
+++ b/examples/event-test.py
@@ -494,6 +494,7 @@ DOM_EVENTS = Description(
 )
 BLOCK_JOB_TYPES = Description("unknown", "Pull", "Copy", "Commit", 
"ActiveCommit")
 BLOCK_JOB_STATUS = Description("Completed", "Failed", "Canceled", "Ready")
+WATCHDOG_ACTIONS = Description("none", "Pause", "Reset", "Poweroff", 
"Shutdown", "Debug", "Inject NMI")
 
 def agentLifecycleStateToString(state):
 agentStates = ( "unknown", "connected", "disconnected", )
@@ -515,8 +516,11 @@ def myDomainEventRebootCallback(conn, dom, opaque):
 def myDomainEventRTCChangeCallback(conn, dom, utcoffset, opaque):
 print("myDomainEventRTCChangeCallback: Domain %s(%s) %d" % (dom.name(), 
dom.ID(), utcoffset))
 
+
 def myDomainEventWatchdogCallback(conn, dom, action, opaque):
-print("myDomainEventWatchdogCallback: Domain %s(%s) %d" % (dom.name(), 
dom.ID(), action))
+print("myDomainEventWatchdogCallback: Domain %s(%s) %s" % (
+dom.name(), dom.ID(), WATCHDOG_ACTIONS[action]))
+
 
 def myDomainEventIOErrorCallback(conn, dom, srcpath, devalias, action, opaque):
 print("myDomainEventIOErrorCallback: Domain %s(%s) %s %s %d" % 
(dom.name(), dom.ID(), srcpath, devalias, action))
-- 
2.11.0

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

Re: [libvirt] [PATCH 02/13] virsh: Implement vshTable API to net-list and net-dhcp-leases

[Erik Skultety]

On Wed, Sep 19, 2018 at 11:26:27AM +0200, Michal Privoznik wrote:
> On 09/18/2018 04:21 PM, Simon Kobyda wrote:
> > Signed-off-by: Simon Kobyda 
> > ---
> >  tools/virsh-network.c | 55 +--
> >  1 file changed, 37 insertions(+), 18 deletions(-)
> >
> > diff --git a/tools/virsh-network.c b/tools/virsh-network.c
> > index ca07fb568f..0f975b8899 100644
> > --- a/tools/virsh-network.c
> > +++ b/tools/virsh-network.c
> > @@ -33,6 +33,7 @@
> >  #include "virstring.h"
> >  #include "virtime.h"
> >  #include "conf/network_conf.h"
> > +#include "vsh-table.h"
> >
> >  #define VIRSH_COMMON_OPT_NETWORK(_helpstr, cflags) \
> >  {.name = "network", \
> > @@ -677,6 +678,7 @@ cmdNetworkList(vshControl *ctl, const vshCmd *cmd 
> > ATTRIBUTE_UNUSED)
> >  bool optUUID = vshCommandOptBool(cmd, "uuid");
> >  char uuid[VIR_UUID_STRING_BUFLEN];
> >  unsigned int flags = VIR_CONNECT_LIST_NETWORKS_ACTIVE;
> > +vshTablePtr table = NULL;
> >
> >  if (vshCommandOptBool(cmd, "inactive"))
> >  flags = VIR_CONNECT_LIST_NETWORKS_INACTIVE;
> > @@ -705,10 +707,10 @@ cmdNetworkList(vshControl *ctl, const vshCmd *cmd 
> > ATTRIBUTE_UNUSED)
> >  return false;
> >
> >  if (optTable) {
> > -vshPrintExtra(ctl, " %-20s %-10s %-13s %s\n", _("Name"), 
> > _("State"),
> > -  _("Autostart"), _("Persistent"));
> > -vshPrintExtra(ctl,
> > -  
> > "--\n");
> > +table = vshTableNew("Name", "State", "Autostart",
> > +"Persistent", NULL);
> > +if (!table)
> > +goto cleanup;
> >  }
> >
> >  for (i = 0; i < list->nnets; i++) {
> > @@ -722,11 +724,15 @@ cmdNetworkList(vshControl *ctl, const vshCmd *cmd 
> > ATTRIBUTE_UNUSED)
> >  else
> >  autostartStr = is_autostart ? _("yes") : _("no");
> >
> > -vshPrint(ctl, " %-20s %-10s %-13s %s\n",
> > - virNetworkGetName(network),
> > - virNetworkIsActive(network) ? _("active") : 
> > _("inactive"),
> > - autostartStr,
> > - virNetworkIsPersistent(network) ? _("yes") : _("no"));
> > +if (vshTableRowAppend(table,
> > +  virNetworkGetName(network),
> > +  virNetworkIsActive(network) ?
> > +  _("active") : _("inactive"),
> > +  autostartStr,
> > +  virNetworkIsPersistent(network) ?
> > +  _("yes") : _("no"),
> > +  NULL) < 0)
> > +goto cleanup;
> >  } else if (optUUID) {
> >  if (virNetworkGetUUIDString(network, uuid) < 0) {
> >  vshError(ctl, "%s", _("Failed to get network's UUID"));
> > @@ -738,8 +744,12 @@ cmdNetworkList(vshControl *ctl, const vshCmd *cmd 
> > ATTRIBUTE_UNUSED)
> >  }
> >  }
> >
> > +if (optTable)
> > +vshTablePrintToStdout(table, ctl);
> > +
> >  ret = true;
> >   cleanup:
> > +vshTableFree(table);
> >  virshNetworkListFree(list);
> >  return ret;
> >  }
> > @@ -1351,6 +1361,7 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd 
> > *cmd)
> >  size_t i;
> >  unsigned int flags = 0;
> >  virNetworkPtr network = NULL;
> > +vshTablePtr table = NULL;
> >
> >  if (vshCommandOptStringReq(ctl, cmd, "mac", &mac) < 0)
> >  return false;
> > @@ -1366,11 +1377,11 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd 
> > *cmd)
> >  /* Sort the list according to MAC Address/IAID */
> >  qsort(leases, nleases, sizeof(*leases), virshNetworkDHCPLeaseSorter);
> >
> > -vshPrintExtra(ctl, " %-20s %-18s %-9s %-25s %-15s %s\n%s%s\n",
> > -  _("Expiry Time"), _("MAC address"), _("Protocol"),
> > -  _("IP address"), _("Hostname"), _("Client ID or DUID"),
> > -  
> > "--",
> > -  
> > "-");
> > +table = vshTableNew("Expiry Time", "MAC address", "Protocol",
> > +"IP address", "Hostname", "Client ID or DUID",
> > +NULL);
> > +if (!table)
> > +goto cleanup;
> >
> >  for (i = 0; i < nleases; i++) {
> >  const char *typestr = NULL;
> > @@ -1390,17 +1401,25 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd 
> > *cmd)
> >  ignore_value(virAsprintf(&cidr_format, "%s/%d",
> >   lease->ipaddr, lease->prefix));
> >
> > -vshPrint(ctl, " %-20s %-18s %-9s %-25s %-15s %s\n",
> > - expirytime, EMPTYSTR(lease->mac),
> > - EMPTYSTR(typestr), cidr_format,
> > - E

[libvirt] [PATCH libvirt-python v2 01/22] event-test.py: Handle closed connection

[Philipp Hahn]

If libvirtd terminates while event-test.py has an open connection to it,
it will crash with the following traceback:

> myConnectionCloseCallback: qemu:///session: Error
> Exception in thread libvirtEventLoop:
> Traceback (most recent call last):
>   File "/usr/lib/python2.7/threading.py", line 801, in __bootstrap_inner
> self.run()
>   File "/usr/lib/python2.7/threading.py", line 754, in run
> self.__target(*self.__args, **self.__kwargs)
>   File "examples/event-test.py", line 424, in virEventLoopPollRun
> eventLoop.run_loop()
>   File "examples/event-test.py", line 242, in run_loop
> self.run_once()
>   File "examples/event-test.py", line 187, in run_once
> libvirt.virEventInvokeFreeCallback(opaque)
> AttributeError: 'module' object has no attribute 'virEventInvokeFreeCallback'
>
> libvirt: XML-RPC error : internal error: client socket is closed
> Traceback (most recent call last):
>   File "examples/event-test.py", line 872, in 
> main()
>   File "examples/event-test.py", line 854, in main
> vc.secretEventDeregisterAny(id)
>   File "/usr/lib/python2.7/dist-packages/libvirt.py", line 4987, in 
> secretEventDeregisterAny
> if ret == -1: raise libvirtError ('virConnectSecretEventDeregisterAny() 
> failed', conn=self)
> libvirt.libvirtError: internal error: client socket is closed
> Closing qemu:///session

Skip unregistering the event callbacks and closing the connection if the
connection is already broken / closed.

Signed-off-by: Philipp Hahn 
---
 examples/event-test.py | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/examples/event-test.py b/examples/event-test.py
index 04310e1..c17d2bb 100755
--- a/examples/event-test.py
+++ b/examples/event-test.py
@@ -716,7 +716,8 @@ def main():
 old_exitfunc = getattr(sys, 'exitfunc', None)
 def exit():
 print("Closing " + vc.getURI())
-vc.close()
+if run:
+vc.close()
 if (old_exitfunc): old_exitfunc()
 sys.exitfunc = exit
 
@@ -777,6 +778,11 @@ def main():
 count = count + 1
 time.sleep(1)
 
+# If the connection was closed, we cannot unregister anything.
+# Just abort now.
+if not run:
+return
+
 vc.domainEventDeregister(myDomainEventCallback1)
 
 for id in seccallbacks:
-- 
2.11.0

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

Re: [libvirt] [PATCH 5/5] qemu: Avoid duplicate resume events and state changes

[Jiri Denemark]

On Wed, Sep 19, 2018 at 16:04:13 -0400, John Ferlan wrote:
> > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > index 825a9d399b..4771f26938 100644
> > --- a/src/qemu/qemu_migration.c
> > +++ b/src/qemu/qemu_migration.c
> 
> [...]
> 
> >  
> > @@ -5074,13 +5060,8 @@ qemuMigrationDstFinish(virQEMUDriverPtr driver,
> >  goto endjob;
> >  }
> >  
> > -if (inPostCopy) {
> > +if (inPostCopy)
> >  doKill = false;
> > -event = virDomainEventLifecycleNewFromObj(vm,
> > -VIR_DOMAIN_EVENT_RESUMED,
> > -VIR_DOMAIN_EVENT_RESUMED_POSTCOPY);
> > -virObjectEventStateQueue(driver->domainEventState, event);
> > -}
> >  }
> >  
> >  if (mig->jobInfo) {
> > @@ -5111,11 +5092,6 @@ qemuMigrationDstFinish(virQEMUDriverPtr driver,
> >  
> >  dom = virGetDomain(dconn, vm->def->name, vm->def->uuid, vm->def->id);
> >  
> > -event = virDomainEventLifecycleNewFromObj(vm,
> > -  VIR_DOMAIN_EVENT_RESUMED,
> > -  
> > VIR_DOMAIN_EVENT_RESUMED_MIGRATED);
> > -virObjectEventStateQueue(driver->domainEventState, event);
> > -
> >  if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) {
> >  virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, 
> > VIR_DOMAIN_PAUSED_USER);
> >  event = virDomainEventLifecycleNewFromObj(vm,
> 
> For this path 2 events were generated if @inPostCopy == True - one right
> after the qemuProcessStartCPUs for RESUMED_POSTCOPY and then one for
> SUSPENDED_PAUSED once qemuMigrationDstWaitForCompletion occurred.

I guess you meant s/SUSPENDED_PAUSED/RESUMED_MIGRATED/. Anyway, you made
me think about this and you're right. Or docs even say that two RESUMED
events are sent during post-copy migration. RESUMED_POSTCOPY when the
domain switches from source to the destination host and RESUMED_MIGRATED
once migration is complete... v2 is in progress.

> IIUC, the changes here when @inPostCopy == true would only generate the
> POSTCOPY event, but not the MIGRATED event. Or is there something subtle
> I'm missing. Will the post copy processing generate two resume events?
> If so, we perhaps should document that. If not, then perhaps this second
> event that's being deleted needs to be moved inside that inPostCopy
> condition just above with the note that this one case is "abnormal"

Yes.

> (or you could just do the *StartCPUs again with the RESUMED_MIGRATED
> and close your eyes ;-)).

This wouldn't work, the RESUME handler ignores the event if the domain
is not paused.

> Curious, for the future, would it be useful to change the FakeReboot
> path to generate a different detail reason? It's hard to distinguish
> between someone using virsh resume (or recovery from Save, Dump,
> Snapshot failure) from that fake reboot path - at least from just the
> event. I guess it seems VIR_DOMAIN_EVENT_RESUMED_UNPAUSED is just too
> generic since it's more than just a "normal resume due to admin
> unpause", but that I would think would be extra given the "scope" of
> this particular problem.

Yeah, a new resume reason would be nice in this case, but I don't think
it's a big issue. The guest-agent driven reboot is much better and
hopefully used more often than fake reboot.

> Second curiosity... would it be useful/easy to generate an event on the
> failure scenario for StartCPUs? Knowing from whence we were called, we
> should be able to generate a suspended event rather than having the
> callers decide.

Not really, if StartCPUs fails the vCPUs just remain paused as if
nothing happened. That is, there's no state change to signal using an
event. And I think "cont" can't really fail anyway. Normally it would
just succeed and another STOP event would be sent right away in case the
reason for pausing the CPUs (such as I/O error) is still valid.

Jirka

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

[libvirt] [PATCH 2/4] virdbus: Unref the D-Bus connection when closing

[Marc Hartmayer]

From: Marc Hartmayer 

As documented at
https://dbus.freedesktop.org/doc/api/html/group__DBusConnection.html#ga2522ac5075dfe0a1535471f6e045e1ee
the creator of a non-shared D-Bus connection has to release the last
reference after closing for freeing.

Signed-off-by: Marc Hartmayer 
Reviewed-by: Bjoern Walk 
Reviewed-by: Boris Fiuczynski 
Reviewed-by: Stefan Zimmermann 
---
 src/util/virdbus.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/util/virdbus.c b/src/util/virdbus.c
index d0e2c76e486f..1d1e39aae728 100644
--- a/src/util/virdbus.c
+++ b/src/util/virdbus.c
@@ -144,6 +144,7 @@ void virDBusCloseSystemBus(void)
 {
 if (systembus && !sharedBus) {
 dbus_connection_close(systembus);
+dbus_connection_unref(systembus);
 systembus = NULL;
 }
 }
-- 
2.17.0

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

[libvirt] [PATCH 3/4] virdbus: Report a debug message that dbus_watch_handle() has failed

[Marc Hartmayer]

From: Marc Hartmayer 

Report a debug message if dbus_watch_handle() returns FALSE.
dbus_watch_handle() returns FALSE if there wasn't enough memory for
reading or writing.

Signed-off-by: Marc Hartmayer 
Reviewed-by: Bjoern Walk 
Reviewed-by: Boris Fiuczynski 
Reviewed-by: Stefan Zimmermann 
---
 src/util/virdbus.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/util/virdbus.c b/src/util/virdbus.c
index 1d1e39aae728..01c67fbaf467 100644
--- a/src/util/virdbus.c
+++ b/src/util/virdbus.c
@@ -197,7 +197,8 @@ static void virDBusWatchCallback(int fdatch 
ATTRIBUTE_UNUSED,
 if (events & VIR_EVENT_HANDLE_HANGUP)
 dbus_flags |= DBUS_WATCH_HANGUP;
 
-(void)dbus_watch_handle(watch, dbus_flags);
+if (dbus_watch_handle(watch, dbus_flags) == FALSE)
+VIR_DEBUG("dbus_watch_handle() returned FALSE");
 
 dbus_connection_ref(info->bus);
 while (dbus_connection_dispatch(info->bus) == DBUS_DISPATCH_DATA_REMAINS)
-- 
2.17.0

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

[libvirt] [PATCH 4/4] virdbus: Use the mnemonic macros for dbus_bool_t values

[Marc Hartmayer]

From: Marc Hartmayer 

Use the mnemonic macros of libdbus for 1 (TRUE) and 0 (FALSE).

Signed-off-by: Marc Hartmayer 
Reviewed-by: Bjoern Walk 
Reviewed-by: Boris Fiuczynski 
Reviewed-by: Stefan Zimmermann 
---
 src/util/virdbus.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/util/virdbus.c b/src/util/virdbus.c
index 01c67fbaf467..0ace5b250cac 100644
--- a/src/util/virdbus.c
+++ b/src/util/virdbus.c
@@ -236,7 +236,7 @@ static dbus_bool_t virDBusAddWatch(DBusWatch *watch,
 struct virDBusWatch *info;
 
 if (VIR_ALLOC(info) < 0)
-return 0;
+return FALSE;
 
 if (dbus_watch_get_enabled(watch))
 flags = virDBusTranslateWatchFlags(dbus_watch_get_flags(watch));
@@ -253,10 +253,10 @@ static dbus_bool_t virDBusAddWatch(DBusWatch *watch,
 watch, NULL);
 if (info->watch < 0) {
 dbus_watch_set_data(watch, NULL, NULL);
-return 0;
+return FALSE;
 }
 
-return 1;
+return TRUE;
 }
 
 
-- 
2.17.0

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

[libvirt] [PATCH 0/4] Small changes to virdbus

[Marc Hartmayer]

Marc Hartmayer (4):
  virdbus: Grab a ref as long as the while loop is executed
  virdbus: Unref the D-Bus connection when closing
  virdbus: Report a debug message that dbus_watch_handle() has failed
  virdbus: Use the mnemonic macros for dbus_bool_t values

 src/util/virdbus.c | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

-- 
2.17.0

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

[libvirt] [PATCH 1/4] virdbus: Grab a ref as long as the while loop is executed

[Marc Hartmayer]

From: Marc Hartmayer 

Grab a ref for info->bus (a DBus connection) as long as the while loop
is running. With the grabbed reference it is ensured that info->bus
isn't freed as long as the while loop is executed. This is necessary
as it's allowed to drop the last ref for the bus connection in a
handler.

There was already a bug of this kind in libdbus itself:
https://bugs.freedesktop.org/show_bug.cgi?id=15635.

Signed-off-by: Marc Hartmayer 
Reviewed-by: Boris Fiuczynski 
---
 src/util/virdbus.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/util/virdbus.c b/src/util/virdbus.c
index ba8b684f17f1..d0e2c76e486f 100644
--- a/src/util/virdbus.c
+++ b/src/util/virdbus.c
@@ -198,8 +198,10 @@ static void virDBusWatchCallback(int fdatch 
ATTRIBUTE_UNUSED,
 
 (void)dbus_watch_handle(watch, dbus_flags);
 
+dbus_connection_ref(info->bus);
 while (dbus_connection_dispatch(info->bus) == DBUS_DISPATCH_DATA_REMAINS)
 /* keep dispatching while data remains */;
+dbus_connection_unref(info->bus);
 }
 
 
-- 
2.17.0

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

Re: [libvirt] [PATCH v2] virsh: Don't break loop of domblkinfo for disks

[John Ferlan]

On 09/21/2018 08:32 AM, Michal Privoznik wrote:
> On 08/22/2018 11:46 AM, Han Han wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1619625
>>
>> --all option is added to cmdDomblkinfo since commit 62c39193 allowing to
>> show all block devices info. Reset error when empty source in case error
>> breaks the loop of domblkinfo for disks.
>>
>> Signed-off-by: Han Han 
>> ---
>>  tools/virsh-domain-monitor.c | 13 +
>>  1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
>> index b9b4f9739b..576610f005 100644
>> --- a/tools/virsh-domain-monitor.c
>> +++ b/tools/virsh-domain-monitor.c
>> @@ -475,6 +475,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
>>  int ndisks;
>>  size_t i;
>>  xmlNodePtr *disks = NULL;
>> +char *source = NULL;
>>  char *target = NULL;
>>  char *protocol = NULL;
>>  
>> @@ -505,16 +506,18 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
>>  
>>  for (i = 0; i < ndisks; i++) {
>>  ctxt->node = disks[i];
>> +source = virXPathString("string(./source)", ctxt);
>>  protocol = virXPathString("string(./source/@protocol)", ctxt);
>>  target = virXPathString("string(./target/@dev)", ctxt);
>>  
>>  rc = virDomainGetBlockInfo(dom, target, &info, 0);
>>  
>>  if (rc < 0) {
>> -/* If protocol is present that's an indication of a 
>> networked
>> - * storage device which cannot provide statistics, so 
>> generate
>> - * 0 based data and get the next disk. */
>> -if (protocol && !active &&
>> +/* For the case of empty cdrom, networked disk which cannot
>> + * provide statistics, generate 0 based data and get the 
>> next
>> + * disk.
>> + */
>> +if (!source && protocol && !active &&
> 
> Shouldn't this look like:
> 
> if ((!source || protocol) && !active &&
> 
> I guess we want this to be:
> 
>   - either source is missing, or
>   - disk is a network one
> 
> Michal
> 

There's a v3 already:

https://www.redhat.com/archives/libvir-list/2018-August/msg01418.html

John

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

Re: [libvirt] [PATCH 2/2] virsh: Display vhostuser socket path in domiflist

[Andrea Bolognani]

On Fri, 2018-09-21 at 13:03 +0200, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1630164
> 
> The domiflist command is designed to show a brief information on
> domain interfaces. One piece of information that is shows is
> "Source" - source network, device, name, bridge. However, it's
> ignoring vhostuser for which we can show the unix socket it's
> associated with.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  tools/virsh-domain-monitor.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v2] virsh: Don't break loop of domblkinfo for disks

[Michal Privoznik]

On 08/22/2018 11:46 AM, Han Han wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1619625
> 
> --all option is added to cmdDomblkinfo since commit 62c39193 allowing to
> show all block devices info. Reset error when empty source in case error
> breaks the loop of domblkinfo for disks.
> 
> Signed-off-by: Han Han 
> ---
>  tools/virsh-domain-monitor.c | 13 +
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
> index b9b4f9739b..576610f005 100644
> --- a/tools/virsh-domain-monitor.c
> +++ b/tools/virsh-domain-monitor.c
> @@ -475,6 +475,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
>  int ndisks;
>  size_t i;
>  xmlNodePtr *disks = NULL;
> +char *source = NULL;
>  char *target = NULL;
>  char *protocol = NULL;
>  
> @@ -505,16 +506,18 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
>  
>  for (i = 0; i < ndisks; i++) {
>  ctxt->node = disks[i];
> +source = virXPathString("string(./source)", ctxt);
>  protocol = virXPathString("string(./source/@protocol)", ctxt);
>  target = virXPathString("string(./target/@dev)", ctxt);
>  
>  rc = virDomainGetBlockInfo(dom, target, &info, 0);
>  
>  if (rc < 0) {
> -/* If protocol is present that's an indication of a networked
> - * storage device which cannot provide statistics, so 
> generate
> - * 0 based data and get the next disk. */
> -if (protocol && !active &&
> +/* For the case of empty cdrom, networked disk which cannot
> + * provide statistics, generate 0 based data and get the next
> + * disk.
> + */
> +if (!source && protocol && !active &&

Shouldn't this look like:

if ((!source || protocol) && !active &&

I guess we want this to be:

  - either source is missing, or
  - disk is a network one

Michal

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

Re: [libvirt] [PATCH 1/2] qemu_hotplug: Fetch vhostuser ifname on hotplug

[Andrea Bolognani]

On Fri, 2018-09-21 at 13:03 +0200, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1630164
> 
> Since 2a13a0a1033 we are querying the vhostuser's interface name
> when building qemu command line. However, we forgot to do so on
> hotplug.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_hotplug.c | 5 +
>  1 file changed, 5 insertions(+)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH 00/11] qemu: Improve / cleanup QEMU binary handling

[Michal Privoznik]

On 09/20/2018 05:25 PM, Andrea Bolognani wrote:
> This is the output of 'virsh capabilities' on my laptop:
> 
>   
> hvm
> 
>   64
>   /usr/bin/qemu-system-x86_64
>   pc-i440fx-3.0
>   pc
>   pc-q35-3.0
>   q35
>   
>   
>   
> /usr/bin/qemu-kvm
> pc-i440fx-3.0
> pc
> pc-q35-3.0
> q35
> 
>   
> 
> 
>   
> 
> Notice how all machine types are listed twice, and how we report
> that qemu-system-x86_64 for TCG guests qemu-kvm must be used for
> KVM guests - which is inaccurate, since the former can run KVM
> guests just fine.
> 
> After this series, the output is much more reasonable:
> 
>   
> hvm
> 
>   64
>   /usr/bin/qemu-system-x86_64
>   pc-i440fx-3.0
>   pc
>   pc-q35-3.0
>   q35
>   
>   
>   
> 
> 
>   
> 
> As a bonus the code gets *simpler* in the process instead of more
> complicated, and we even get to shave off ~100 lines! Yay!
> 
> Andrea Bolognani (11):
>   qemu: Move comments to virQEMUCapsGuestIsNative()
>   qemu: Don't duplicate binary name in capabilities
>   qemu: Move armv7l-on-aarch64 special case
>   qemu: Stop looking after finding the first binary
>   qemu: Expect a single binary in virQEMUCapsInitGuest()
>   qemu: Remove unnecessary variables
>   qemu: Don't look for "qemu-kvm" and "kvm" binaries
>   qemu: Simplify QEMU binary search
>   qemu: Rename qemubinCaps => qemuCaps
>   qemu: Refactor virQEMUCapsCacheLookupByArch()
>   qemu: Prefer qemu-system-* binaries
> 
>  src/qemu/qemu_capabilities.c  | 170 +++---
>  src/qemu/qemu_capabilities.h  |   4 +-
>  .../qemucaps2xmloutdata/caps_1.5.3.x86_64.xml |   4 +-
>  .../qemucaps2xmloutdata/caps_1.6.0.x86_64.xml |   4 +-
>  .../qemucaps2xmloutdata/caps_1.7.0.x86_64.xml |   4 +-
>  .../qemucaps2xmloutdata/caps_2.1.1.x86_64.xml |   4 +-
>  .../caps_2.10.0.aarch64.xml   |   4 +-
>  .../qemucaps2xmloutdata/caps_2.10.0.ppc64.xml |   4 +-
>  .../qemucaps2xmloutdata/caps_2.10.0.s390x.xml |   4 +-
>  .../caps_2.10.0.x86_64.xml|   4 +-
>  .../qemucaps2xmloutdata/caps_2.11.0.s390x.xml |   4 +-
>  .../caps_2.11.0.x86_64.xml|   4 +-
>  .../caps_2.12.0.aarch64.xml   |   4 +-
>  .../qemucaps2xmloutdata/caps_2.12.0.ppc64.xml |   4 +-
>  .../qemucaps2xmloutdata/caps_2.12.0.s390x.xml |   4 +-
>  .../caps_2.12.0.x86_64.xml|   4 +-
>  .../qemucaps2xmloutdata/caps_2.4.0.x86_64.xml |   4 +-
>  .../qemucaps2xmloutdata/caps_2.5.0.x86_64.xml |   4 +-
>  .../caps_2.6.0.aarch64.xml|   4 +-
>  .../qemucaps2xmloutdata/caps_2.6.0.ppc64.xml  |   4 +-
>  .../qemucaps2xmloutdata/caps_2.6.0.x86_64.xml |   4 +-
>  .../qemucaps2xmloutdata/caps_2.7.0.s390x.xml  |   4 +-
>  .../qemucaps2xmloutdata/caps_2.7.0.x86_64.xml |   4 +-
>  .../qemucaps2xmloutdata/caps_2.8.0.s390x.xml  |   4 +-
>  .../qemucaps2xmloutdata/caps_2.8.0.x86_64.xml |   4 +-
>  .../qemucaps2xmloutdata/caps_2.9.0.ppc64.xml  |   4 +-
>  .../qemucaps2xmloutdata/caps_2.9.0.s390x.xml  |   4 +-
>  .../qemucaps2xmloutdata/caps_2.9.0.x86_64.xml |   4 +-
>  .../qemucaps2xmloutdata/caps_3.0.0.ppc64.xml  |   4 +-
>  .../qemucaps2xmloutdata/caps_3.0.0.x86_64.xml |   4 +-
>  tests/qemucaps2xmltest.c  |   2 -
>  31 files changed, 97 insertions(+), 191 deletions(-)
> 

ACK

Michal

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

Re: [libvirt] [PATCH 3/5] qemu: Pass running reason to RESUME event handler

[Jiri Denemark]

On Wed, Sep 19, 2018 at 11:12:26 -0400, John Ferlan wrote:
> 
> 
> On 09/12/2018 08:55 AM, Jiri Denemark wrote:
> > Whenever we get the RESUME event from QEMU, we change the state of the
> > affected domain to VIR_DOMAIN_RUNNING with VIR_DOMAIN_RUNNING_UNPAUSED
> > reason. This is fine if the domain is resumed unexpectedly, but when we
> > sent "cont" to QEMU we usually have a better reason for the state
> > change. The better reason is used in qemuProcessStartCPUs which also
> > sets the domain state to running if qemuMonitorStartCPUs reports
> > success. Thus we may end up with two state updates in a row, but the
> > final reason is correct.
> > 
> > This patch is a preparation for dropping the state change done in
> > qemuMonitorStartCPUs for which we need to pass the actual running reason
> > to the RESUME event handler and use it there instead of
> > VIR_DOMAIN_RUNNING_UNPAUSED.
> > 
> > Signed-off-by: Jiri Denemark 
> > ---
> >  src/qemu/qemu_domain.h  |  4 
> >  src/qemu/qemu_process.c | 23 +--
> >  2 files changed, 21 insertions(+), 6 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> > index 914c9a6a8d..3f3f7ccf18 100644
> > --- a/src/qemu/qemu_domain.h
> > +++ b/src/qemu/qemu_domain.h
> > @@ -366,6 +366,10 @@ struct _qemuDomainObjPrivate {
> >  
> >  /* counter for generating node names for qemu disks */
> >  unsigned long long nodenameindex;
> > +
> > +/* qemuProcessStartCPUs stores the reason for starting vCPUs here for 
> > the
> > + * RESUME event handler to use it */
> > +virDomainRunningReason runningReason;
> 
> So what happens in the libvirtd restart case/condition?  This isn't
> Format/Parse'd so it's lost or essentially set to RUNNING_UNKNOWN.

Right, when libvirtd is restarted just after sending "cont", I don't
think we can expect to see the "RESUME" event in the new process. Most
likely the previous libvirtd process already got the event and just
didn't have a chance to process it. Thus just updating
qemuDomainObjPrivateXML{Parse|Format} to store this in the status XML
wouldn't help. We could add a code which would look at the restored
runningReason when seeing vCPUs are running, but status XML says the
domain is paused. But it would be in a separate patch anyway.

Jirka

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

Re: [libvirt] [PATCH 2/3] lxc: Resolve memory leak

[John Ferlan]

On 09/21/2018 04:43 AM, Erik Skultety wrote:
> On Thu, Sep 20, 2018 at 05:34:37PM -0400, John Ferlan wrote:
>> Commit 40b5c99a modified the virConfGetValue callers to use
>> virConfGetValueString. However, using the virConfGetValueString
>> resulted in leaking the returned @value string in each case.
>> So, let's modify each instance to use the VIR_AUTOFREE(char *)
>> syntax. In some instances changing the variable name since
>> @value was used more than once.
>>
>> Found by Coverity
>>
>> Signed-off-by: John Ferlan 
>> ---
> ...
> 
> 
>>  static int
>>  lxcSetMemTune(virDomainDefPtr def, virConfPtr properties)
>>  {
>> -char *value = NULL;
>> +VIR_AUTOFREE(char *) hard_limit = NULL;
>> +VIR_AUTOFREE(char *) soft_limit = NULL;
>> +VIR_AUTOFREE(char *) swap_hard_limit = NULL;
>>  unsigned long long size = 0;
> 
> This is just a matter of taste, but I'd probably went with VIR_AUTOFREE(char 
> *)
> value within each of the 'if' blocks.
> 

In this instance, how?

>>
>>  if (virConfGetValueString(properties,
>>"lxc.cgroup.memory.limit_in_bytes",
>> -  &value) > 0) {

^^^ hard_limit instead of value, 4 spaces in for the "if"... This
repeats for each one. They're all top level.

>> -if (lxcConvertSize(value, &size) < 0)
>> -goto error;
>> +  &hard_limit) > 0) {
>> +if (lxcConvertSize(hard_limit, &size) < 0) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>> +   _("failed to parse integer: '%s'"),
>> +   hard_limit);
> 
> lxcConvertSize already reports an error: "failed to convert size: '%s'". And
> since the one you added provides essentially the same amount of information, 
> we
> might as well go with that one. Potentially, we could also change it so that 
> it
> matches the wording of all the other ones for consistency reasons, i.e. the 
> one
> you're proposing here.
> Moreover, I don't really like copying the same error message, having a goto
> error label is IMHO justified in case like these, although, we can drop it
> specifically for this function, it's different for the functions below 
> though...
> 

Generally speaking - I agree about the common error label; however, in
this case without looking at the lxcConvertSize because the previous
code was:

- error:
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("failed to parse integer: '%s'"), value);

and @value was being replaced by 3 new variables each unique, using a
common label was not viable.

So there's really two patches here. The first one should remove those
error: labels because the lxcConvertSize already generates one.  Then
the second patch would be to convert to VIR_AUTOFREE.


>> +return -1;
>> +}
>>  size = size / 1024;
>>  virDomainDefSetMemoryTotal(def, size);
>>  def->mem.hard_limit = virMemoryLimitTruncate(size);
>> @@ -771,76 +777,85 @@ lxcSetMemTune(virDomainDefPtr def, virConfPtr 
>> properties)
>>
>>  if (virConfGetValueString(properties,
>>"lxc.cgroup.memory.soft_limit_in_bytes",
>> -  &value) > 0) {
>> -if (lxcConvertSize(value, &size) < 0)
>> -goto error;
>> +  &soft_limit) > 0) {
>> +if (lxcConvertSize(soft_limit, &size) < 0) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>> +   _("failed to parse integer: '%s'"),
>> +   soft_limit);
>> +return -1;
>> +}
>>  def->mem.soft_limit = virMemoryLimitTruncate(size / 1024);
>>  }
>>
>>  if (virConfGetValueString(properties,
>>"lxc.cgroup.memory.memsw.limit_in_bytes",
>> -  &value) > 0) {
>> -if (lxcConvertSize(value, &size) < 0)
>> -goto error;
>> +  &swap_hard_limit) > 0) {
>> +if (lxcConvertSize(swap_hard_limit, &size) < 0) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>> +   _("failed to parse integer: '%s'"),
>> +   swap_hard_limit);
>> +return -1;
>> +}
>>  def->mem.swap_hard_limit = virMemoryLimitTruncate(size / 1024);
>>  }
>>  return 0;
>> -
>> - error:
>> -virReportError(VIR_ERR_INTERNAL_ERROR,
>> -   _("failed to parse integer: '%s'"), value);
>> -return -1;
> 
> we can definitely drop it ^here...Alternatively, we could drop the message in
> lxcConvertSize and leave the error label here as well, again, for consistency
> reasons, it's perhaps also the better way to let it behave like just like the
> virStrToX helpers.
> 
>> -
>>  }
>>
>>  static int
>>  lxcSetCpuTune(virDomainDefPtr def, virConfPtr properties)
>>  {
>> -char *value = NULL;
>> +VIR_AUTOFREE(char *) shares =

Re: [libvirt] [PATCH 0/2] tests: Add QEMU 3.0.0 capability tests for s390x

[John Ferlan]

On 09/21/2018 01:13 AM, Bjoern Walk wrote:
> John Ferlan  [2018-09-20, 04:30PM -0400]:
>>
>>
>> On 09/20/2018 06:20 AM, Boris Fiuczynski wrote:
>>> Add capability and domcaps tests for QEMU 3.0.0 on s390x.
>>>
>>> Boris Fiuczynski (2):
>>>   tests: Add capabilities data for QEMU 3.0.0 on s390x
>>>   tests: domaincaps: Add QEMU 3.0 for s390x
>>>
>>>  .../domaincapsschemadata/qemu_3.0.0.s390x.xml |   183 +
>>>  tests/domaincapstest.c| 4 +
>>>  .../caps_3.0.0.s390x.replies  | 20530 
>>>  .../qemucapabilitiesdata/caps_3.0.0.s390x.xml |  2691 ++
>>>  tests/qemucapabilitiestest.c  | 1 +
>>>  5 files changed, 23409 insertions(+)
>>>  create mode 100644 tests/domaincapsschemadata/qemu_3.0.0.s390x.xml
>>>  create mode 100644 tests/qemucapabilitiesdata/caps_3.0.0.s390x.replies
>>>  create mode 100644 tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml
>>>
>>
>> Reviewed-by: John Ferlan 
>> (series)
>>
>> and pushed,
> 
> Thanks, but just for interest, tracking Reviewed-by and other tags is a
> thing in libvirt or not?
> 

 - totally forgot to do that on this series - my apologies...
I still haven't succumbed to figuring out a way to make the R-B add
happen automagically And our hook only cares to see the S-O-B.

To answer the question - there's no "official" tracking. Some people
still use ACK while others use R-By. It was hard enough to jump the
hurdle for S-O-B.

John

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

Re: [libvirt] [PATCH libvirt-python 0/2] event-test.py fixes

[Michal Privoznik]

On 09/20/2018 08:10 AM, Philipp Hahn wrote:
> Hello,
> 
> event-test.py is bad at handling newer livecycle events.
> Attached are two patches to improve that.
> 
> Philipp Hahn (2):
>   event-test.py: Sync list of domain lifecycle events
>   event-test.py: Future proof lifecycle event handling
> 
>  examples/event-test.py | 18 --
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 

Philipp,

ACK to the first patch, no doubt about that. However, the second one -
I'm torn. One one hand it makes the code more future proof, on the other
- seeing a traceback (which is easy to spot) might inspire users to post
patches (because we are obviously not doing a good job in keeping
event-test.py in sync with libvirt). What do you think?

Michal

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

[libvirt] [PATCH 0/2] Couple of vhostuser fixes

[Michal Privoznik]

Almost trivial ones.

Michal Prívozník (2):
  qemu_hotplug: Fetch vhostuser ifname on hotplug
  virsh: Display vhostuser socket path in domiflist

 src/qemu/qemu_hotplug.c  | 5 +
 tools/virsh-domain-monitor.c | 3 ++-
 2 files changed, 7 insertions(+), 1 deletion(-)

-- 
2.16.4

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

[libvirt] [PATCH 1/2] qemu_hotplug: Fetch vhostuser ifname on hotplug

[Michal Privoznik]

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

Since 2a13a0a1033 we are querying the vhostuser's interface name
when building qemu command line. However, we forgot to do so on
hotplug.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_hotplug.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 633e7fc18f..b00f5ef300 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1330,6 +1330,11 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
 
 if (!(charDevAlias = qemuAliasChardevFromDevAlias(net->info.alias)))
 goto cleanup;
+
+if 
(virNetDevOpenvswitchGetVhostuserIfname(net->data.vhostuser->data.nix.path,
+   &net->ifname) < 0)
+goto cleanup;
+
 break;
 
 case VIR_DOMAIN_NET_TYPE_USER:
-- 
2.16.4

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

[libvirt] [PATCH 2/2] virsh: Display vhostuser socket path in domiflist

[Michal Privoznik]

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

The domiflist command is designed to show a brief information on
domain interfaces. One piece of information that is shows is
"Source" - source network, device, name, bridge. However, it's
ignoring vhostuser for which we can show the unix socket it's
associated with.

Signed-off-by: Michal Privoznik 
---
 tools/virsh-domain-monitor.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index 8962586d76..35c2216137 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -715,7 +715,8 @@ cmdDomiflist(vshControl *ctl, const vshCmd *cmd)
 source = virXPathString("string(./source/@bridge"
 "|./source/@dev"
 "|./source/@network"
-"|./source/@name)", ctxt);
+"|./source/@name"
+"|./source/@path)", ctxt);
 
 target = virXPathString("string(./target/@dev)", ctxt);
 model = virXPathString("string(./model/@type)", ctxt);
-- 
2.16.4

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

[libvirt] [PATCH 4/4] security: Always spawn process for transactions

[Michal Privoznik]

There is this latent bug which can result in virtlockd killing
libvirtd. The problem is when the following steps occur:

   Parent |  Child
--+
1) virSecurityManagerMetadataLock(path);  |
   This results in connection FD to virtlockd to be   |
   dup()-ed and saved for later use.  |
  |
2) Another thread calling fork(); |
   This results in the FD from step 1) to be cloned   |
  |
3) virSecurityManagerMetadataUnlock(path);|
   Here, the FD is closed, but the connection is  |
   still kept open because child process has  |
   duplicated FD  |
  |
4) virSecurityManagerMetadataLock(differentPath); |
   Again, new connection is opened, FD is dup()-ed|
  |
5)| exec() or exit()

Step 5) results in closing the connection from 1) to be closed,
finally. However, at this point virtlockd looks at its internal
records if PID from 1) does not still own any resources. Well it
does - in step 4) it locked differentPath. This results in
virtlockd killing libvirtd.

Note that this happens because we do some relabelling even before
fork() at which point vm->pid is still -1. When this value is
passed down to transaction code it simply runs the transaction
in parent process (=libvirtd), which means the owner of metadata
locks is the parent process.

Signed-off-by: Michal Privoznik 

Signed-off-by: Michal Privoznik 
---
 src/security/security_dac.c | 12 ++--
 src/security/security_selinux.c | 12 ++--
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 5aea386e7c..876cca0f2f 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -561,12 +561,12 @@ virSecurityDACTransactionCommit(virSecurityManagerPtr mgr 
ATTRIBUTE_UNUSED,
 goto cleanup;
 }
 
-if ((pid == -1 &&
- virSecurityDACTransactionRun(pid, list) < 0) ||
-(pid != -1 &&
- virProcessRunInMountNamespace(pid,
-   virSecurityDACTransactionRun,
-   list) < 0))
+if (pid == -1)
+pid = getpid();
+
+if (virProcessRunInMountNamespace(pid,
+  virSecurityDACTransactionRun,
+  list) < 0)
 goto cleanup;
 
 ret = 0;
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 31e42afee7..1e23d776ff 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1102,12 +1102,12 @@ 
virSecuritySELinuxTransactionCommit(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
 goto cleanup;
 }
 
-if ((pid == -1 &&
- virSecuritySELinuxTransactionRun(pid, list) < 0) ||
-(pid != -1 &&
- virProcessRunInMountNamespace(pid,
-   virSecuritySELinuxTransactionRun,
-   list) < 0))
+if (pid == -1)
+pid = getpid();
+
+if (virProcessRunInMountNamespace(pid,
+  virSecuritySELinuxTransactionRun,
+  list) < 0)
 goto cleanup;
 
 ret = 0;
-- 
2.16.4

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

[libvirt] [PATCH 3/4] virLockManagerLockDaemonAcquire: Duplicate client FD with CLOEXEC flag

[Michal Privoznik]

There is one caller (virSecurityManagerMetadataLock) which
duplicates the connection FD and wants to have the flag set.
However, trying to set the flag after dup() is not safe as
another thread might fork() meanwhile. Therefore, switch to
duplicating with the flag set and only let callers refine this
later.

Signed-off-by: Michal Privoznik 
---
 src/locking/domain_lock.c   | 18 ++
 src/locking/lock_driver_lockd.c |  2 +-
 src/rpc/virnetclient.c  |  9 +
 src/rpc/virnetclient.h  |  2 +-
 4 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/src/locking/domain_lock.c b/src/locking/domain_lock.c
index 705b132457..db20fa86a3 100644
--- a/src/locking/domain_lock.c
+++ b/src/locking/domain_lock.c
@@ -188,6 +188,24 @@ int virDomainLockProcessStart(virLockManagerPluginPtr 
plugin,
 ret = virLockManagerAcquire(lock, NULL, flags,
 dom->def->onLockFailure, fd);
 
+if (ret >= 0 && fd && *fd >= 0 && virSetInherit(*fd, true) < 0) {
+int saved_errno = errno;
+virErrorPtr origerr;
+
+virErrorPreserveLast(&origerr);
+
+if (virLockManagerRelease(lock, NULL, 0) < 0)
+VIR_WARN("Unable to release acquired resourced in cleanup path");
+
+virErrorRestore(&origerr);
+errno = saved_errno;
+
+virReportSystemError(errno, "%s",
+ _("Cannot disable close-on-exec flag"));
+
+ret = -1;
+}
+
 virLockManagerFree(lock);
 
 return ret;
diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c
index 0c672b05b1..9b1943daa6 100644
--- a/src/locking/lock_driver_lockd.c
+++ b/src/locking/lock_driver_lockd.c
@@ -796,7 +796,7 @@ static int 
virLockManagerLockDaemonAcquire(virLockManagerPtr lock,
 goto cleanup;
 
 if (fd &&
-(*fd = virNetClientDupFD(client, false)) < 0)
+(*fd = virNetClientDupFD(client)) < 0)
 goto cleanup;
 
 if (!(flags & VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY)) {
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index 40ed3fde6d..6b0ddbeaad 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -712,19 +712,12 @@ int virNetClientGetFD(virNetClientPtr client)
 }
 
 
-int virNetClientDupFD(virNetClientPtr client, bool cloexec)
+int virNetClientDupFD(virNetClientPtr client)
 {
 int fd;
 
 virObjectLock(client);
-
 fd = virNetSocketDupFD(client->sock);
-if (!cloexec && fd >= 0 && virSetInherit(fd, true)) {
-virReportSystemError(errno, "%s",
- _("Cannot disable close-on-exec flag"));
-VIR_FORCE_CLOSE(fd);
-}
-
 virObjectUnlock(client);
 return fd;
 }
diff --git a/src/rpc/virnetclient.h b/src/rpc/virnetclient.h
index 9cf32091f5..3702f7fe5a 100644
--- a/src/rpc/virnetclient.h
+++ b/src/rpc/virnetclient.h
@@ -95,7 +95,7 @@ void virNetClientSetCloseCallback(virNetClientPtr client,
   virFreeCallback ff);
 
 int virNetClientGetFD(virNetClientPtr client);
-int virNetClientDupFD(virNetClientPtr client, bool cloexec);
+int virNetClientDupFD(virNetClientPtr client);
 
 bool virNetClientHasPassFD(virNetClientPtr client);
 
-- 
2.16.4

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

[libvirt] [PATCH 0/4] Couple of metadata locking fixes

[Michal Privoznik]

Strictly speaking, only the last patch actually fixes a real problem.
The first three are more of a cleanup than anything. However, I am still
sending them because they make the code better.

Anyway, if my explanation in 4/4 is not clear enough, here it is in
simpler terms:

It's important to bear in mind that at any point when connection to
virtlockd is closed, virtlockd goes through its hash table and if it
finds any resources acquired for the process that has closed the
connection, it will kill the process. In other words, resources are per
PID not per connection.

For instance, if libvirtd would open two connections, acquired two
different resources through each of them and then closed one it would
get killed instantly because it still owns some resources.

So now that this virtlockd behaviour is clear (I must say intended and
desired behaviour), we can start looking into how libvirtd runs qemu.

Basically, problem is that instead of opening connection to virtlockd
once and keeping it open through whole run of libvirtd I used this
shortcut: open the connection in virSecurityManagerMetadataLock(), dup()
the connection FD, and close it in virSecurityManagerMetadataUnlock().

In theory, this works pretty well - when the duplicated FD is closed,
libvirtd doesn't own any locks anymore so virtlockd doesn't kill it.
What my design did not count with is fork(). On fork() the duplicated
FD is cloned into the child and thus connection is closed nobody knows
when. Meantime, libvirtd might continue its execution (e.g. starting
another domain from another thread) and call
virSecurityManagerMetadataLock(). However, before it gets to call
Unlock() the child process decides to close the FD (either via exec() or
exit()). Given virtlockd behaviour described couple of paragraphs above
I guess it is clear to see now that virtlockd kills libvirtd.

My solution to this is to always run secdriver transactions from a
separate child process because on fork() only the thread that calls it
is cloned. So only the thread that runs the transaction is cloned, not
the one that is starting a different domain. Therefore no other fork()
can occur here and therefore we are safe.

I know, I know, it's complicated. But it always is around fork() and
IPC.

If you want to see my fix in action, just enable metadata locking, and
try to start two or more domains in a loop:

  for ((i=0;i<1000;i++)); do \
  virsh start u1604-1 &  virsh start u1604-2 &\
  sleep 3;\
  virsh destroy u1604-1; virsh destroy u1604-2;\
  done

At some point you'll see virsh reporting I/O error (this is because
virtlockd killed libvirtd). With my patch, I run the test multiple times
without any hiccup.

@Bjoern: I'm still unable to reproduce the issue you reported. However,
whilst trying to do so I've came across this bug. However, my gut
feeling is that this might help you.


Michal Prívozník (4):
  security: Grab a reference to virSecurityManager for transactions
  virNetSocket: Be more safe with fork() around virNetSocketDupFD()
  virLockManagerLockDaemonAcquire: Duplicate client FD with CLOEXEC flag
  security: Always spawn process for transactions

 src/libxl/libxl_migration.c |  4 ++--
 src/locking/domain_lock.c   | 18 ++
 src/locking/lock_driver_lockd.c |  2 +-
 src/qemu/qemu_migration.c   |  2 +-
 src/rpc/virnetclient.c  |  5 +++--
 src/rpc/virnetclient.h  |  2 +-
 src/rpc/virnetsocket.c  |  7 ++-
 src/rpc/virnetsocket.h  |  2 +-
 src/security/security_dac.c | 17 +
 src/security/security_selinux.c | 17 +
 10 files changed, 47 insertions(+), 29 deletions(-)

-- 
2.16.4

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

[libvirt] [PATCH 1/4] security: Grab a reference to virSecurityManager for transactions

[Michal Privoznik]

This shouldn't be needed per-se. Security manager shouldn't
disappear during transactions - it's immutable. However, it
doesn't hurt to grab a reference either - transaction code uses
it after all.

Signed-off-by: Michal Privoznik 
---
 src/security/security_dac.c | 5 +++--
 src/security/security_selinux.c | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 2dbaf29ff5..5aea386e7c 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -141,6 +141,7 @@ virSecurityDACChownListFree(void *opaque)
 VIR_FREE(list->items[i]);
 }
 VIR_FREE(list->items);
+virObjectUnref(list->manager);
 VIR_FREE(list);
 }
 
@@ -511,12 +512,12 @@ virSecurityDACTransactionStart(virSecurityManagerPtr mgr)
 if (VIR_ALLOC(list) < 0)
 return -1;
 
-list->manager = mgr;
+list->manager = virObjectRef(mgr);
 
 if (virThreadLocalSet(&chownList, list) < 0) {
 virReportSystemError(errno, "%s",
  _("Unable to set thread local variable"));
-VIR_FREE(list);
+virSecurityDACChownListFree(list);
 return -1;
 }
 
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 056637e4cb..31e42afee7 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -156,6 +156,7 @@ virSecuritySELinuxContextListFree(void *opaque)
 for (i = 0; i < list->nItems; i++)
 virSecuritySELinuxContextItemFree(list->items[i]);
 
+virObjectUnref(list->manager);
 VIR_FREE(list);
 }
 
@@ -1054,12 +1055,12 @@ 
virSecuritySELinuxTransactionStart(virSecurityManagerPtr mgr)
 if (VIR_ALLOC(list) < 0)
 return -1;
 
-list->manager = mgr;
+list->manager = virObjectRef(mgr);
 
 if (virThreadLocalSet(&contextList, list) < 0) {
 virReportSystemError(errno, "%s",
  _("Unable to set thread local variable"));
-VIR_FREE(list);
+virSecuritySELinuxContextListFree(list);
 return -1;
 }
 
-- 
2.16.4

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

[libvirt] [PATCH 2/4] virNetSocket: Be more safe with fork() around virNetSocketDupFD()

[Michal Privoznik]

If there was a caller which would dup the client FD without
CLOEXEC flag and later decided to change the flag it wouldn't be
safe to do because fork() might have had occurred meantime.
Switch to the other pattern - always dup FD with the flag set and
let callers clear the flag if they need to do so.

Signed-off-by: Michal Privoznik 
---
 src/libxl/libxl_migration.c |  4 ++--
 src/qemu/qemu_migration.c   |  2 +-
 src/rpc/virnetclient.c  | 10 +-
 src/rpc/virnetsocket.c  |  7 ++-
 src/rpc/virnetsocket.h  |  2 +-
 5 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index fc7ccb53d0..5eb8eb1203 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -310,7 +310,7 @@ libxlMigrateDstReceive(virNetSocketPtr sock,
 }
 VIR_DEBUG("Accepted migration connection."
   "  Spawning thread to process migration data");
-recvfd = virNetSocketDupFD(client_sock, true);
+recvfd = virNetSocketDupFD(client_sock);
 virObjectUnref(client_sock);
 
 /*
@@ -1254,7 +1254,7 @@ libxlDomainMigrationSrcPerform(libxlDriverPrivatePtr 
driver,
 goto cleanup;
 }
 
-sockfd = virNetSocketDupFD(sock, true);
+sockfd = virNetSocketDupFD(sock);
 virObjectUnref(sock);
 
 if (virDomainLockProcessPause(driver->lockManager, vm, &priv->lockState) < 
0)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 825a9d399b..129be0a11a 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3303,7 +3303,7 @@ qemuMigrationSrcConnect(virQEMUDriverPtr driver,
 if (virNetSocketNewConnectTCP(host, port,
   AF_UNSPEC,
   &sock) == 0) {
-spec->dest.fd.qemu = virNetSocketDupFD(sock, true);
+spec->dest.fd.qemu = virNetSocketDupFD(sock);
 virObjectUnref(sock);
 }
 if (qemuSecurityClearSocketLabel(driver->securityManager, vm->def) < 0 ||
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index b4d8fb2187..40ed3fde6d 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -715,8 +715,16 @@ int virNetClientGetFD(virNetClientPtr client)
 int virNetClientDupFD(virNetClientPtr client, bool cloexec)
 {
 int fd;
+
 virObjectLock(client);
-fd = virNetSocketDupFD(client->sock, cloexec);
+
+fd = virNetSocketDupFD(client->sock);
+if (!cloexec && fd >= 0 && virSetInherit(fd, true)) {
+virReportSystemError(errno, "%s",
+ _("Cannot disable close-on-exec flag"));
+VIR_FORCE_CLOSE(fd);
+}
+
 virObjectUnlock(client);
 return fd;
 }
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
index 55de3b2aad..27ffa23bcd 100644
--- a/src/rpc/virnetsocket.c
+++ b/src/rpc/virnetsocket.c
@@ -1372,14 +1372,11 @@ int virNetSocketGetFD(virNetSocketPtr sock)
 }
 
 
-int virNetSocketDupFD(virNetSocketPtr sock, bool cloexec)
+int virNetSocketDupFD(virNetSocketPtr sock)
 {
 int fd;
 
-if (cloexec)
-fd = fcntl(sock->fd, F_DUPFD_CLOEXEC, 0);
-else
-fd = dup(sock->fd);
+fd = fcntl(sock->fd, F_DUPFD_CLOEXEC, 0);
 if (fd < 0) {
 virReportSystemError(errno, "%s",
  _("Unable to copy socket file handle"));
diff --git a/src/rpc/virnetsocket.h b/src/rpc/virnetsocket.h
index de795af917..e6bac27566 100644
--- a/src/rpc/virnetsocket.h
+++ b/src/rpc/virnetsocket.h
@@ -124,7 +124,7 @@ virNetSocketPtr 
virNetSocketNewPostExecRestart(virJSONValuePtr object);
 virJSONValuePtr virNetSocketPreExecRestart(virNetSocketPtr sock);
 
 int virNetSocketGetFD(virNetSocketPtr sock);
-int virNetSocketDupFD(virNetSocketPtr sock, bool cloexec);
+int virNetSocketDupFD(virNetSocketPtr sock);
 
 bool virNetSocketIsLocal(virNetSocketPtr sock);
 
-- 
2.16.4

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

Re: [libvirt] [PATCH 3/3] tests: Resolve possible overrun

[Erik Skultety]

On Thu, Sep 20, 2018 at 05:34:38PM -0400, John Ferlan wrote:
> Coverity noted that each of the fmemopen called used the strlen value
> in order to allocate space, but that neglected space for terminating
> null string. So just add 1 to the strlen.
>
> Signed-off-by: John Ferlan 
> ---
Reviewed-by: Erik Skultety 

Although, I'm wondering whether it's worth having an internal wrapper like the
STREQ stuff we have to mitigate the off-by-1 errors in cases like these, but I
guess there would be cases, where that might be undesirable.

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

Re: [libvirt] [PATCH 2/3] lxc: Resolve memory leak

[Erik Skultety]

On Thu, Sep 20, 2018 at 05:34:37PM -0400, John Ferlan wrote:
> Commit 40b5c99a modified the virConfGetValue callers to use
> virConfGetValueString. However, using the virConfGetValueString
> resulted in leaking the returned @value string in each case.
> So, let's modify each instance to use the VIR_AUTOFREE(char *)
> syntax. In some instances changing the variable name since
> @value was used more than once.
>
> Found by Coverity
>
> Signed-off-by: John Ferlan 
> ---
...


>  static int
>  lxcSetMemTune(virDomainDefPtr def, virConfPtr properties)
>  {
> -char *value = NULL;
> +VIR_AUTOFREE(char *) hard_limit = NULL;
> +VIR_AUTOFREE(char *) soft_limit = NULL;
> +VIR_AUTOFREE(char *) swap_hard_limit = NULL;
>  unsigned long long size = 0;

This is just a matter of taste, but I'd probably went with VIR_AUTOFREE(char *)
value within each of the 'if' blocks.

>
>  if (virConfGetValueString(properties,
>"lxc.cgroup.memory.limit_in_bytes",
> -  &value) > 0) {
> -if (lxcConvertSize(value, &size) < 0)
> -goto error;
> +  &hard_limit) > 0) {
> +if (lxcConvertSize(hard_limit, &size) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("failed to parse integer: '%s'"),
> +   hard_limit);

lxcConvertSize already reports an error: "failed to convert size: '%s'". And
since the one you added provides essentially the same amount of information, we
might as well go with that one. Potentially, we could also change it so that it
matches the wording of all the other ones for consistency reasons, i.e. the one
you're proposing here.
Moreover, I don't really like copying the same error message, having a goto
error label is IMHO justified in case like these, although, we can drop it
specifically for this function, it's different for the functions below though...

> +return -1;
> +}
>  size = size / 1024;
>  virDomainDefSetMemoryTotal(def, size);
>  def->mem.hard_limit = virMemoryLimitTruncate(size);
> @@ -771,76 +777,85 @@ lxcSetMemTune(virDomainDefPtr def, virConfPtr 
> properties)
>
>  if (virConfGetValueString(properties,
>"lxc.cgroup.memory.soft_limit_in_bytes",
> -  &value) > 0) {
> -if (lxcConvertSize(value, &size) < 0)
> -goto error;
> +  &soft_limit) > 0) {
> +if (lxcConvertSize(soft_limit, &size) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("failed to parse integer: '%s'"),
> +   soft_limit);
> +return -1;
> +}
>  def->mem.soft_limit = virMemoryLimitTruncate(size / 1024);
>  }
>
>  if (virConfGetValueString(properties,
>"lxc.cgroup.memory.memsw.limit_in_bytes",
> -  &value) > 0) {
> -if (lxcConvertSize(value, &size) < 0)
> -goto error;
> +  &swap_hard_limit) > 0) {
> +if (lxcConvertSize(swap_hard_limit, &size) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("failed to parse integer: '%s'"),
> +   swap_hard_limit);
> +return -1;
> +}
>  def->mem.swap_hard_limit = virMemoryLimitTruncate(size / 1024);
>  }
>  return 0;
> -
> - error:
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> -   _("failed to parse integer: '%s'"), value);
> -return -1;

we can definitely drop it ^here...Alternatively, we could drop the message in
lxcConvertSize and leave the error label here as well, again, for consistency
reasons, it's perhaps also the better way to let it behave like just like the
virStrToX helpers.

> -
>  }
>
>  static int
>  lxcSetCpuTune(virDomainDefPtr def, virConfPtr properties)
>  {
> -char *value = NULL;
> +VIR_AUTOFREE(char *) shares = NULL;
> +VIR_AUTOFREE(char *) quota = NULL;
> +VIR_AUTOFREE(char *) period = NULL;
>
>  if (virConfGetValueString(properties, "lxc.cgroup.cpu.shares",
> -  &value) > 0) {

again, matter of taste, but I think that having a helper VIR_AUTOFREE(char *)
value that gets destroyed after each block looks more clean than having three
different function local helper variables just to match the structure members
we're filling in.

> -if (virStrToLong_ull(value, NULL, 10, &def->cputune.shares) < 0)
> -goto error;
> +  &shares) > 0) {
> +if (virStrToLong_ull(shares, NULL, 10, &def->cputune.shares) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("failed to parse integer: '%s'"), shares);
> +return -1;
> +}
>  def->cputune.sharesS

Re: [libvirt] [Qemu-devel] [PATCH v2 4/4] hw/core/machine: Officially deprecate the enforce-config-section parameter

[Thomas Huth]

On 2018-09-20 20:13, Markus Armbruster wrote:
> Thomas Huth  writes:
> 
>> Commit 16f7244842b5135543ef068a1adafd94c6965953 added this parameter
>> to the documentation, including a note that it is deprecated. But it
>> has never been added to the "Deprecated features" appendix, which is
>> our official way to deprecate legacy parameters. So let's do this now.
>>
>> Signed-off-by: Thomas Huth 
>> ---
>>  hw/core/machine.c| 3 +++
>>  qemu-deprecated.texi | 5 +
>>  2 files changed, 8 insertions(+)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 6b68e12..882e7b4 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -324,6 +324,9 @@ static void machine_set_enforce_config_section(Object 
>> *obj, bool value,
>>  {
>>  MachineState *ms = MACHINE(obj);
>>  
>> +warn_report("enforce-config-section is deprecated. Use "
> 
> Comma, please.

Sure, I'll change it.

>> +"-global migration.send-configuration=on|off instead");
>> +
>>  ms->enforce_config_section = value;
>>  }
>>  
>> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
>> index 23fa78b..f1e807c 100644
>> --- a/qemu-deprecated.texi
>> +++ b/qemu-deprecated.texi
>> @@ -35,6 +35,11 @@ which is the default.
>>  
>>  @section System emulator command line arguments
>>  
>> +@subsection -machine enforce-config-section=on|off (since 3.1)
>> +
>> +The @option{enforce-config-section} parameter is replaced by the
>> +@option{-global migration.send-configuration=@var{on|off}} option.
>> +
>>  @subsection -no-kvm (since 1.3.0)
>>  
>>  The ``-no-kvm'' argument is now a synonym for setting
> 
> You keep the @item enforce-config-section=on|off in qemu-options.hx.
> Keeping it until we kill the option parameter is okay, but I'd drop now,
> since like to keep all the junk we don't want people to use anymore out
> of the first place they look for stuff they can use.

The text from 16f7244842b5135543ef06 also contains the hint what should
be used instead, so I think it's ok if we keep it until the option gets
removed completely - and that's also the way we did it so far with all
other deprecated options.

 Thomas

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

Re: [libvirt] [Qemu-devel] [QEMU PATCH] net: Deprecate the old way of using a legacy net via "name" instead of "id"

[Markus Armbruster]

Thomas Huth  writes:

> On 2018-09-20 08:07, Markus Armbruster wrote:
>> Thomas Huth  writes:
>> 
>>> In early times, network backends were specified by a "vlan" and "name"
>>> tuple. With the introduction of netdevs, the "name" was replaced by an
>>> "id" (which is supposed to be unique), but the "name" parameter stayed
>>> as an alias which could be used instead of "id". Unfortunately, we miss
>>> the duplication check for "name":
>>>
>>>  $ qemu-system-x86_64 -net user,name=n1 -net user,name=n1
>>>
>>> ... starts without an error, while "id" correctly complains:
>>>
>>>  $ qemu-system-x86_64 -net user,id=n1 -net user,id=n1
>>>  qemu-system-x86_64: -net user,id=n1: Duplicate ID 'n1' for net
>>>
>>> Instead of trying to fix the code for the legacy "name" parameter, let's
>>> rather get rid of this old interface and deprecate the "name" parameter
>>> now - this will also be less confusing for the users in the long run.
>>>
>>> While we're at it, also deprecate the old syntax for the hostfwd_add
>>> and hostfwd_remove commands that still work with this legacy "hub"
>>> plus "name" tuple. It is enough to specify the netdev id there instead.
>>>
>>> Also add a missing dependency to the Makefile to make sure that the
>>> docs get correctly regenerated when qemu-deprecated.texi is changed.
>> 
>> Sure sounds like three patches to me.
>
> Since the changes are really small and slightly related, one compact
> patch sounded fine for me, too. But ok, I just discovered yet another
> unrelated thing that should be added to the deprecation chapter
> ("enforce-config-section"), so I'll turn this into a small patch series
> instead.

Thanks.

>>> @@ -99,6 +104,14 @@ The ``query-cpus'' command is replaced by the 
>>> ``query-cpus-fast'' command.
>>>  The ``arch'' output member of the ``query-cpus-fast'' command is
>>>  replaced by the ``target'' output member.
>>>  
>>> +@section System emulator human monitor commands
>>> +
>>> +@subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove' 
>>> (since 3.1)
>>> +
>>> +The @option{hub_id} parameter of the 'hostfwd_add' and 'hostfwd_remove' HMP
>>> +commands is redundant. It is enough to specify the netdev ID of the backend
>>> +that should be changed.
>> 
>> Hmm.  What's redundant is the [hub_id name] alternative in
>> 
>> .params = "[hub_id name]|[netdev_id] ..."
>> 
>> The [netdev_id] alternative was added in January (commit 93653066445).
>> Suggest to word it more like other sections do:
>> 
>> Parameters @option{hub_id} and @option{name} have been replaced by
>> @option{netdev_id}.
>
> Ok. The funny thing is that IIUC "name" and "netdev_id" are the very
> same identifier nowadays (I was not aware of this in the past), so the
> descriptions could also have been written as "[[hub_id] netdev_id]"
> instead. ==> This "name" vs. "id" thing is really confusing, it's really
> a good idea to remove it soon.

Much appreciated.

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

Re: [libvirt] [PATCH 1/3] conf: Alter when ctxt->node is set

[Erik Skultety]

On Thu, Sep 20, 2018 at 05:34:36PM -0400, John Ferlan wrote:
> In virDomainMemoryDefParseXML and virDomainVideoDefParseXML if
> the VIR_ALLOC fails and NULL is returned, then the alteration
> to ctxt->node isn't reversed.
>
> Found by Coverity
>
> Signed-off-by: John Ferlan 
> ---
Reviewed-by: Erik Skultety 

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

Re: [libvirt] missing libvirt events related to snapshots and media-change for inactive VMs?

[Philipp Hahn]

Hello,

Am 20.09.18 um 08:10 schrieb Philipp Hahn:
> event-test.py is bad at handling newer livecycle events.
> Attached are two patches to improve that.
> 
> Philipp Hahn (2):
>   event-test.py: Sync list of domain lifecycle events
>   event-test.py: Future proof lifecycle event handling
> 
>  examples/event-test.py | 18 --
>  1 file changed, 12 insertions(+), 6 deletions(-)

While looking at the events reported by said script I noticed that there
are no events for creating / deleting snapshot, at leat when the VM is
inactive. For an active VM Qemu reports several events:
  VIR_DOMAIN_EVENT_DEFINED_FROM_SNAPSHOT
  VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT
  VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT
  VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT
  VIR_DOMAIN_EVENT_RESUMED_FROM_SNAPSHOT

Also there is no event generated when doing "virsh change-media" for an
inactive domain; I would have expected an
VIR_DOMAIN_EVENT_DEFINED_UPDATED event.
For an active domain you get the
libvirt.VIR_DOMAIN_EVENT_TRAY_CHANGE_OPEN / CLOSE events.

My current work-around is to do polling, but I want to get rid of that.
Did I miss something or is it okay to add events for tha?

Thanks in advance.
Philipp

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