[libvirt] [PATCH] tests: fix bhyve build

[Laine Stump]

This file was modified in an editor buffer but not saved prior to
commit e62cb4a9b78 (which removed virMacAddr::generated), so the bhyve
build would fail.

Signed-off-by: Laine Stump 
---

Pushed under the build breaker rule.

 tests/bhyvexml2argvmock.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tests/bhyvexml2argvmock.c b/tests/bhyvexml2argvmock.c
index bec7f902aa..7afa0e34c4 100644
--- a/tests/bhyvexml2argvmock.c
+++ b/tests/bhyvexml2argvmock.c
@@ -17,7 +17,6 @@ void virMacAddrGenerate(const unsigned char 
prefix[VIR_MAC_PREFIX_BUFLEN],
 addr->addr[3] = 0;
 addr->addr[4] = 0;
 addr->addr[5] = 0;
-addr->generated = true;
 }
 
 int virNetDevTapCreateInBridgePort(const char *brname ATTRIBUTE_UNUSED,
-- 
2.14.3

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

Re: [libvirt] [PATCH v2 16/42] util: add default: case to all switch statements

[John Ferlan]

On 02/19/2018 06:49 PM, John Ferlan wrote:
> 
> 
> On 02/15/2018 11:43 AM, Daniel P. Berrangé wrote:
>> Even if the compiler has validated that all enum constants have case
>> statements in a switch, it is not safe to omit a default: case
>> statement. When assigning a value to a variable / struct field that is
>> defined with an enum type, nothing prevents an invalid value being
>> assigned. So defensive code must assume existance of invalid values and
>> thus all switches should have a default: case.
>>
>> Signed-off-by: Daniel P. Berrangé 
>> ---
>>  src/libvirt.c|  3 +++
>>  src/util/vircgroup.c |  1 +
>>  src/util/vircrypto.c |  2 ++
>>  src/util/virerror.c  |  6 +-
>>  src/util/virfdstream.c   |  4 
>>  src/util/virfirewall.c   |  1 +
>>  src/util/virhook.c   | 12 +--
>>  src/util/virhostdev.c|  8 +++-
>>  src/util/virhostmem.c|  5 +
>>  src/util/virjson.c   |  5 +
>>  src/util/virlog.c|  7 +--
>>  src/util/virnetdev.c |  1 +
>>  src/util/virnetdevmacvlan.c  |  3 +++
>>  src/util/virnetdevvportprofile.c | 43 
>> ++--
>>  src/util/virnuma.c   |  5 -
>>  src/util/virprocess.c|  1 +
>>  src/util/virqemu.c   |  5 +
>>  src/util/virsexpr.c  |  2 ++
>>  src/util/virsocketaddr.c | 13 +++-
>>  src/util/virstoragefile.c| 15 --
>>  20 files changed, 118 insertions(+), 24 deletions(-)
>>
> 
> [...]
> 
>> diff --git a/src/util/virhook.c b/src/util/virhook.c
>> index facd74aeff..cf104d0234 100644
>> --- a/src/util/virhook.c
>> +++ b/src/util/virhook.c
>> @@ -277,12 +277,12 @@ virHookCall(int driver,
> 
> Above here at the top of the function there is a :
> 
> if ((driver < VIR_HOOK_DRIVER_DAEMON) ||
> (driver >= VIR_HOOK_DRIVER_LAST))
> return 1;
> 
> thus the "default:" case added achieves DEAD_ERROR from Coverity.
> 
>>  break;
>>  case VIR_HOOK_DRIVER_NETWORK:
>>  opstr = virHookNetworkOpTypeToString(op);
>> -}
>> -if (opstr == NULL) {
>> -virReportError(VIR_ERR_INTERNAL_ERROR,
>> -   _("Hook for %s, failed to find operation #%d"),
>> -   drvstr, op);
>> -return 1;
>> +break;
>> +default:
>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>> +   _("Hook for %s, failed to find operation #%d"),
>> +   drvstr, op);
> 
> Removing default: "works" because @driver is an int.
> 
> BTW: @drvstr would be NULL here, right?
> 
> If the initial condition is removed, then that makes Coverity happy.
> 
> 
>> +return 1;
>>  }
>>  subopstr = virHookSubopTypeToString(sub_op);
>>  if (subopstr == NULL)
> 
> [...]
> 
>> diff --git a/src/util/virhostmem.c b/src/util/virhostmem.c
>> index 11efe8c502..26576a73cc 100644
>> --- a/src/util/virhostmem.c
>> +++ b/src/util/virhostmem.c
>> @@ -608,6 +608,11 @@ virHostMemGetParameters(virTypedParameterPtr params 
>> ATTRIBUTE_UNUSED,
>>  return -1;
>>  
>>  break;
>> +
>> +default:
>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>> +   _("Unexpected parameter number %zu"), i);
>> +return -1;

Based on even more warnings like this in the subsequent patch, I'm just
going to disable DEADCODE checking in my Coverity check scripts. It'll
be easier that way. Maybe some day in the far future gcc8 will be used
in/for Coverity and they'll fix the problem ;-)

John

I'll pick up in the morning on the next set of patches...
> 
> Here we are in a for loop from 0 to NODE_MEMORY_PARAMETERS_NUM (or 8),
> so Coverity complains that default is DEAD... This one's a little
> trickier because removing default: leaves open the possibility of the
> constant changing and someone not adding the next number in the
> sequence.  The "easier" path is remove default:, the other option to
> avoid Coverity notification is adding a "/* coverity[dead_error_begin]
> */" right above default:.
> 
>>  }
>>  }
>>  
> 
> [...]
> 
>> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
>> index b250af9e2c..b185900bd6 100644
>> --- a/src/util/virnetdev.c
>> +++ b/src/util/virnetdev.c
>> @@ -2802,6 +2802,7 @@ static int virNetDevParseMcast(char *buf, 
>> virNetDevMcastEntryPtr mcast)
>>  
>>  /* coverity[dead_error_begin] */
>>  case VIR_MCAST_TYPE_LAST:
>> +default:
> 
> Similar to virhostmem, we're in a for loop where ifindex starts at
> VIR_MCAST_TYPE_INDEX_TOKEN and must be less than VIR_MCAST_TYPE_LAST, so
> we get a DEAD_ERROR for both of these.
> 
> Removing the (virMCastType) and leaving as an int removes the error, but
> leaves open the possibility of a 

Re: [libvirt] [PATCH v2 16/42] util: add default: case to all switch statements

[John Ferlan]

On 02/15/2018 11:43 AM, Daniel P. Berrangé wrote:
> Even if the compiler has validated that all enum constants have case
> statements in a switch, it is not safe to omit a default: case
> statement. When assigning a value to a variable / struct field that is
> defined with an enum type, nothing prevents an invalid value being
> assigned. So defensive code must assume existance of invalid values and
> thus all switches should have a default: case.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/libvirt.c|  3 +++
>  src/util/vircgroup.c |  1 +
>  src/util/vircrypto.c |  2 ++
>  src/util/virerror.c  |  6 +-
>  src/util/virfdstream.c   |  4 
>  src/util/virfirewall.c   |  1 +
>  src/util/virhook.c   | 12 +--
>  src/util/virhostdev.c|  8 +++-
>  src/util/virhostmem.c|  5 +
>  src/util/virjson.c   |  5 +
>  src/util/virlog.c|  7 +--
>  src/util/virnetdev.c |  1 +
>  src/util/virnetdevmacvlan.c  |  3 +++
>  src/util/virnetdevvportprofile.c | 43 
> ++--
>  src/util/virnuma.c   |  5 -
>  src/util/virprocess.c|  1 +
>  src/util/virqemu.c   |  5 +
>  src/util/virsexpr.c  |  2 ++
>  src/util/virsocketaddr.c | 13 +++-
>  src/util/virstoragefile.c| 15 --
>  20 files changed, 118 insertions(+), 24 deletions(-)
> 

[...]

> diff --git a/src/util/virhook.c b/src/util/virhook.c
> index facd74aeff..cf104d0234 100644
> --- a/src/util/virhook.c
> +++ b/src/util/virhook.c
> @@ -277,12 +277,12 @@ virHookCall(int driver,

Above here at the top of the function there is a :

if ((driver < VIR_HOOK_DRIVER_DAEMON) ||
(driver >= VIR_HOOK_DRIVER_LAST))
return 1;

thus the "default:" case added achieves DEAD_ERROR from Coverity.

>  break;
>  case VIR_HOOK_DRIVER_NETWORK:
>  opstr = virHookNetworkOpTypeToString(op);
> -}
> -if (opstr == NULL) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> -   _("Hook for %s, failed to find operation #%d"),
> -   drvstr, op);
> -return 1;
> +break;
> +default:
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Hook for %s, failed to find operation #%d"),
> +   drvstr, op);

Removing default: "works" because @driver is an int.

BTW: @drvstr would be NULL here, right?

If the initial condition is removed, then that makes Coverity happy.


> +return 1;
>  }
>  subopstr = virHookSubopTypeToString(sub_op);
>  if (subopstr == NULL)

[...]

> diff --git a/src/util/virhostmem.c b/src/util/virhostmem.c
> index 11efe8c502..26576a73cc 100644
> --- a/src/util/virhostmem.c
> +++ b/src/util/virhostmem.c
> @@ -608,6 +608,11 @@ virHostMemGetParameters(virTypedParameterPtr params 
> ATTRIBUTE_UNUSED,
>  return -1;
>  
>  break;
> +
> +default:
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Unexpected parameter number %zu"), i);
> +return -1;

Here we are in a for loop from 0 to NODE_MEMORY_PARAMETERS_NUM (or 8),
so Coverity complains that default is DEAD... This one's a little
trickier because removing default: leaves open the possibility of the
constant changing and someone not adding the next number in the
sequence.  The "easier" path is remove default:, the other option to
avoid Coverity notification is adding a "/* coverity[dead_error_begin]
*/" right above default:.

>  }
>  }
>  

[...]

> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index b250af9e2c..b185900bd6 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -2802,6 +2802,7 @@ static int virNetDevParseMcast(char *buf, 
> virNetDevMcastEntryPtr mcast)
>  
>  /* coverity[dead_error_begin] */
>  case VIR_MCAST_TYPE_LAST:
> +default:

Similar to virhostmem, we're in a for loop where ifindex starts at
VIR_MCAST_TYPE_INDEX_TOKEN and must be less than VIR_MCAST_TYPE_LAST, so
we get a DEAD_ERROR for both of these.

Removing the (virMCastType) and leaving as an int removes the error, but
leaves open the possibility of a new type being missed.

Modifying the code to be:

/* coverity[dead_error_condition] */
case VIR_MCAST_TYPE_LAST:
/* coverity[dead_error_begin] */
default:

Fixes things, but IMO is butt-ugly. So maybe we just leave this one as
is and I keep a local filter for it.

>  break;
>  }
>  }

[...]

I think the first one could be easily adjusted - the last two - IDC,
keep them as is and I'll filter them or adjust them later in my Coverity
branch.

Reviewed-by: John Ferlan 

John

--

[libvirt] [tck Patch] Require perl(IO::Pty)

[Laine Stump]

The Net::OpenSSH module requires IO::Pty if password authentication
will be used, but The Fedora Net::OpenSSH maintainers don't want to
put anything stronger that "Suggests: perl(IO::Pty)" in their
specfile, which unfortunately does absolutely nothing. (see
https://bugzilla.redhat.com/show_bug.cgi?id=1542666 ). The alternate
solution is for perl-Sys-Virt-TCK.spec to require perl(IO::Pty), and
that's what this patch does.

Signed-off-by: Laine Stump 
---
 perl-Sys-Virt-TCK.spec.PL | 1 +
 1 file changed, 1 insertion(+)

diff --git a/perl-Sys-Virt-TCK.spec.PL b/perl-Sys-Virt-TCK.spec.PL
index 1534854..7c83be9 100644
--- a/perl-Sys-Virt-TCK.spec.PL
+++ b/perl-Sys-Virt-TCK.spec.PL
@@ -74,6 +74,7 @@ Requires: perl(TAP::Formatter::HTML)
 Requires: perl(TAP::Formatter::JUnit)
 Requires: perl(TAP::Harness::Archive)
 Requires: perl(Net::OpenSSH)
+Requires: perl(IO::Pty)
 Requires: /usr/bin/mkisofs
 BuildArchitectures: noarch
 
-- 
2.14.3

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

Re: [libvirt] [PATCH v2 14/42] m4: enforce that all enum cases are listed in switch statements

[John Ferlan]

On 02/15/2018 11:43 AM, Daniel P. Berrangé wrote:
> As a general rule any time we switch() on something that is an enum, we
> want to have a case for every enum constant. The -Wswitch warning will
> report any switch where we've violated this rule, except if that switch
> has a default case.
> 
> Unfortunately it is reasonable to want to list all enum constants *and*
> also have a default case. To get a warning in that scenario requires
> that we turn on -Wswitch-enum.
> 
> In a few cases where we explicitly don't want to list all enum cases, we
> can discard the enum type checking by casting the value to a plain int.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  m4/virt-compile-warnings.m4 | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH v2 13/42] tools: handle missing switch enum cases

[John Ferlan]

On 02/15/2018 11:43 AM, Daniel P. Berrangé wrote:
> Cast away enum type in places where we don't wish to cover all cases.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  tools/virt-host-validate-qemu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH v2 11/42] security: handle missing switch enum cases

[John Ferlan]

On 02/15/2018 11:43 AM, Daniel P. Berrangé wrote:
> Ensure all enum cases are listed in switch statements.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/security/security_driver.c | 1 +
>  1 file changed, 1 insertion(+)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH v2 12/42] xen: handle missing switch enum cases

[John Ferlan]

On 02/15/2018 11:43 AM, Daniel P. Berrangé wrote:
> Ensure all enum cases are listed in switch statements.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/vmx/vmx.c  | 27 +--
>  src/xenconfig/xen_common.c | 17 +++--
>  src/xenconfig/xen_xl.c |  8 +++-
>  3 files changed, 47 insertions(+), 5 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH v2 10/42] rpc: handle missing switch enum cases

[John Ferlan]

On 02/15/2018 11:43 AM, Daniel P. Berrangé wrote:
> Ensure all enum cases are listed in switch statements.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/rpc/virnetclient.c| 2 ++
>  src/rpc/virnetclientprogram.c | 1 +
>  src/rpc/virnetserverprogram.c | 4 
>  3 files changed, 7 insertions(+)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH v2 09/42] qemu: handle missing switch enum cases

[John Ferlan]

On 02/15/2018 11:43 AM, Daniel P. Berrangé wrote:
> Ensure all enum cases are listed in switch statements, or cast away
> enum type in places where we don't wish to cover all cases.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/qemu/qemu_command.c   | 28 
>  src/qemu/qemu_domain.c| 21 +
>  src/qemu/qemu_driver.c| 27 +++
>  src/qemu/qemu_hotplug.c   | 36 +++-
>  src/qemu/qemu_migration.c | 11 ++-
>  src/qemu/qemu_process.c   |  2 ++
>  6 files changed, 103 insertions(+), 22 deletions(-)
> 

[...]

> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 3641b801f6..fe3342b38d 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -1437,11 +1437,20 @@ qemuMigrationJobName(virDomainObjPtr vm)
>  
>  switch (priv->job.asyncJob) {
>  case QEMU_ASYNC_JOB_MIGRATION_OUT:
> -return _("migration job");
> +return _("migration out job");
>  case QEMU_ASYNC_JOB_SAVE:
>  return _("domain save job");
>  case QEMU_ASYNC_JOB_DUMP:
>  return _("domain core dump job");
> +case QEMU_ASYNC_JOB_NONE:
> +return _("no job");

Can this be "undefined" instead of "no"?  Looking at the caller you
could get output such as "operation failed: no job: %s" where %s could
be "is not active", "unexpectedly failed", "canceled by client", or
"failed due to I/O error"

Could also jus  lump it in with LAST and default.

> +case QEMU_ASYNC_JOB_MIGRATION_IN:
> +return _("migration in job");
> +case QEMU_ASYNC_JOB_SNAPSHOT:
> +return _("snapshot job");
> +case QEMU_ASYNC_JOB_START:
> +return _("start job");
> +case QEMU_ASYNC_JOB_LAST:
>  default:
>  return _("job");
>  }

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH v2 08/42] nwfilter: handle missing switch enum cases

[John Ferlan]

On 02/15/2018 11:43 AM, Daniel P. Berrangé wrote:
> Ensure all enum cases are listed in switch statements, or cast away
> enum type in places where we don't wish to cover all cases.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/nwfilter/nwfilter_ebiptables_driver.c | 16 +++-
>  src/nwfilter/nwfilter_learnipaddr.c   |  6 +-
>  2 files changed, 16 insertions(+), 6 deletions(-)
> 

[...]

> diff --git a/src/nwfilter/nwfilter_learnipaddr.c 
> b/src/nwfilter/nwfilter_learnipaddr.c
> index 5b95f0e613..9ca0639576 100644
> --- a/src/nwfilter/nwfilter_learnipaddr.c
> +++ b/src/nwfilter/nwfilter_learnipaddr.c
> @@ -430,7 +430,7 @@ learnIPAddressThread(void *arg)
>  }
>  virBufferAddLit(, "src port 67 and dst port 68");
>  break;
> -default:
> +case DETECT_STATIC:
>  if (techdriver->applyBasicRules(req->ifname,
>  >macaddr) < 0) {
>  req->status = EINVAL;
> @@ -438,6 +438,10 @@ learnIPAddressThread(void *arg)
>  }
>  virBufferAsprintf(, "ether host %s or ether dst 
> ff:ff:ff:ff:ff:ff",
>macaddr);
> +break;
> +default:
> +req->status = EINVAL;
> +goto done;

Could add a VIR_DEBUG() in this just to "track" the reason it was EINVAL
since there's so many ways it could be EINVAL, not that important...

>  }
>  
>  if (virBufferError()) {
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH v2 07/42] lxc: handle missing switch enum cases

[John Ferlan]

On 02/15/2018 11:43 AM, Daniel P. Berrangé wrote:
> Ensure all enum cases are listed in switch statements, or cast away
> enum type in places where we don't wish to cover all cases.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/lxc/lxc_container.c  |  7 ---
>  src/lxc/lxc_controller.c | 10 +-
>  src/lxc/lxc_driver.c | 40 
>  3 files changed, 49 insertions(+), 8 deletions(-)
> 


[...]

> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> index 961baa344c..7d6568cdf8 100644
> --- a/src/lxc/lxc_driver.c
> +++ b/src/lxc/lxc_driver.c
> @@ -3968,10 +3968,22 @@ lxcDomainAttachDeviceNetLive(virConnectPtr conn,
>  if (!(veth = virLXCProcessSetupInterfaceDirect(conn, vm->def, net)))
>  goto cleanup;
>  }   break;
> -default:
> +case VIR_DOMAIN_NET_TYPE_USER:
> +case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
> +case VIR_DOMAIN_NET_TYPE_SERVER:
> +case VIR_DOMAIN_NET_TYPE_CLIENT:
> +case VIR_DOMAIN_NET_TYPE_MCAST:
> +case VIR_DOMAIN_NET_TYPE_INTERNAL:
> +case VIR_DOMAIN_NET_TYPE_HOSTDEV:
> +case VIR_DOMAIN_NET_TYPE_UDP:
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("Network device type is not supported"));
>  goto cleanup;
> +case VIR_DOMAIN_NET_TYPE_LAST:
> +default:
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Unexpected net type %d"), actualType);
> +goto cleanup;
>  }
>  /* Set bandwidth or warn if requested and not supported. */
>  actualBandwidth = virDomainNetGetActualBandwidth(net);
> @@ -4011,6 +4023,15 @@ lxcDomainAttachDeviceNetLive(virConnectPtr conn,
>  ignore_value(virNetDevMacVLanDelete(veth));
>  break;
>  
> +case VIR_DOMAIN_NET_TYPE_USER:
> +case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
> +case VIR_DOMAIN_NET_TYPE_SERVER:
> +case VIR_DOMAIN_NET_TYPE_CLIENT:
> +case VIR_DOMAIN_NET_TYPE_MCAST:
> +case VIR_DOMAIN_NET_TYPE_INTERNAL:
> +case VIR_DOMAIN_NET_TYPE_HOSTDEV:
> +case VIR_DOMAIN_NET_TYPE_UDP:
> +case VIR_DOMAIN_NET_TYPE_LAST:
>  default:
>  /* no-op */
>  break;

Something Coverity noted long ago, but I've just held onto the patch...
At this point (e.g. when ret != 0 in cleanup:), @veth can only defined
if DIRECT, BRIDGE, NETWORK, or ETHERNET, so it's not happy with the
switch and cases. The way I worked around this in Coverity was:

if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT)
ignore_value(virNetDevMacVLanDelete(veth));
else /* BRIDGE, NETWORK, ETHERNET */
ignore_value(virNetDevVethDelete(veth));

I don't think that's perfect either, but it sufficed.

IDC if you keep the code as is, but figured I'd note it.

> @@ -4446,13 +4467,24 @@ lxcDomainDetachDeviceNetLive(virDomainObjPtr vm,

Since they'd all be included here - if you adjust the comment to
indicate "It'd be nice to support DIRECT" instead of just "this" (or
somehow indicate that DIRECT is possible, but the others really aren't).

>   * the host side. Further the container can change
>   * the mac address of NIC name, so we can't easily
>   * find out which guest NIC it maps to
> + */
>  case VIR_DOMAIN_NET_TYPE_DIRECT:
> -*/
> -
> -default:
> +case VIR_DOMAIN_NET_TYPE_USER:
> +case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
> +case VIR_DOMAIN_NET_TYPE_SERVER:
> +case VIR_DOMAIN_NET_TYPE_CLIENT:
> +case VIR_DOMAIN_NET_TYPE_MCAST:
> +case VIR_DOMAIN_NET_TYPE_INTERNAL:
> +case VIR_DOMAIN_NET_TYPE_HOSTDEV:
> +case VIR_DOMAIN_NET_TYPE_UDP:
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("Only bridged veth devices can be detached"));
>  goto cleanup;

[...]

I'll leave it up to you for deciding on the comment above...

Reviewed-by: John Ferlan 

John

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

[libvirt] [PATCH] nwfilter: assure that virNWFilterSnoop(Eth|Dhcp)Hdr objects don't change size

[Laine Stump]

These two objects are used to access fields in actual ethernet packets
captures with libpcap, so it's essential that they don't change size
for any reason. This patch uses gnulib's verify() macro to make sure
their sizes don't change.

Signed-off-by: Laine Stump 
---

danpb suggested doing this in his response to my patch fixing virMacAddr:

  https://www.redhat.com/archives/libvir-list/2018-February/msg00850.html

I decided to make it a separate patch from the virMacAddr fix.

 src/nwfilter/nwfilter_dhcpsnoop.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c 
b/src/nwfilter/nwfilter_dhcpsnoop.c
index 743136277d..8e955150fa 100644
--- a/src/nwfilter/nwfilter_dhcpsnoop.c
+++ b/src/nwfilter/nwfilter_dhcpsnoop.c
@@ -195,6 +195,7 @@ struct _virNWFilterSnoopEthHdr {
 uint16_t eh_type;
 uint8_t eh_data[];
 } ATTRIBUTE_PACKED;
+verify(sizeof(struct _virNWFilterSnoopEthHdr) == 14);
 
 typedef struct _virNWFilterSnoopDHCPHdr virNWFilterSnoopDHCPHdr;
 typedef virNWFilterSnoopDHCPHdr *virNWFilterSnoopDHCPHdrPtr;
@@ -216,6 +217,7 @@ struct _virNWFilterSnoopDHCPHdr {
 char  d_file[128];
 uint8_t   d_opts[];
 } ATTRIBUTE_PACKED;
+verify(sizeof(struct _virNWFilterSnoopDHCPHdr) == 236);
 
 /* DHCP options */
 
-- 
2.14.3

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

Re: [libvirt] [PATCH v2 06/42] libxl: handle missing switch enum cases

[John Ferlan]

On 02/15/2018 11:43 AM, Daniel P. Berrangé wrote:
> Cast away enum type for libxl schedular constants since we don't want to
> cover all of them and don't want build to break when new ones are added.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/libxl/libxl_driver.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH v2 05/42] hyperv: handle missing switch enum cases

[John Ferlan]

On 02/15/2018 11:43 AM, Daniel P. Berrangé wrote:
> Ensure all enum cases are listed in switch statements. This improves
> debug logging integration with openwsman.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/hyperv/hyperv_driver.c | 18 --
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH v2 04/42] esx: handle missing switch enum cases

[John Ferlan]

On 02/15/2018 11:43 AM, Daniel P. Berrangé wrote:
> Ensure all enum cases are listed in switch statements, or explicitly
> cast away enum type where we don't want to list all cases.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/esx/esx_driver.c   |  1 +
>  src/esx/esx_vi.c   | 11 +++
>  src/esx/esx_vi_types.c |  9 +
>  3 files changed, 13 insertions(+), 8 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH v2 03/42] conf: handle missing switch enum cases

[John Ferlan]

On 02/15/2018 11:43 AM, Daniel P. Berrangé wrote:
> Ensure all enum cases are listed in switch statements.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/conf/domain_audit.c  |  1 +
>  src/conf/domain_conf.c   | 47 +--
>  src/conf/nwfilter_conf.c | 32 +++-
>  3 files changed, 73 insertions(+), 7 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH v2 02/42] util: handle missing switch enum cases

[John Ferlan]

On 02/15/2018 11:43 AM, Daniel P. Berrangé wrote:
> Ensure all enum cases are listed in switch statements.

and in some cases force an error if something unexpected is found.

[not necessary to add, especially since it's repeatable throughout the
series so far, but figured I'd note it at least B-)]


> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/util/virconf.c   | 13 -
>  src/util/virfirewall.c   |  7 +--
>  src/util/virlog.c| 10 +-
>  src/util/virnetdevvportprofile.c | 11 ++-
>  4 files changed, 36 insertions(+), 5 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH v2 01/42] conf: add enum constants for default controller models

[John Ferlan]

On 02/15/2018 11:43 AM, Daniel P. Berrangé wrote:
> The controller model is slightly unusual in that the default value is
> -1, not 0. As a result the default value is not covered by any of the
> existing enum cases. This in turn means that any switch() statements
> that think they have covered all cases, will in fact not match the
> default value at all. In the qemuDomainDeviceCalculatePCIConnectFlags()
> method this has caused a serious mistake where we fallthough from the
> SCSI controller case, to the VirtioSerial controller case, and from
> the USB controller case to the IDE controller case.
> 
> By adding explicit enum constant starting at -1, we can ensure switches
> remember to handle the default case.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/conf/domain_addr.c |  5 +
>  src/conf/domain_conf.c |  1 +
>  src/conf/domain_conf.h |  4 
>  src/qemu/qemu_command.c| 17 ++---
>  src/qemu/qemu_domain.c | 10 +-
>  src/qemu/qemu_domain_address.c | 22 ++
>  src/vbox/vbox_common.c | 13 +
>  7 files changed, 64 insertions(+), 8 deletions(-)
> 

There's a few places where in the code where model is compared against
-1 that could perhaps use the VIR_DOMAIN_CONTROLLER_MODEL_*_DEFAULT (I
used 'model.*=.*-1' in the Find this egrep pattern in cscope).

At least one of them (below) caused a Coverity complaint.

So should any of those be altered or should we just live with the fact
that using/knowing -1 is the 'default' model?

> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> index 6422682391..df19c6be1a 100644
> --- a/src/conf/domain_addr.c
> +++ b/src/conf/domain_addr.c

[...]

> @@ -1746,6 +1750,7 @@ 
> virDomainUSBAddressControllerModelToPorts(virDomainControllerDefPtr cont)
>  return cont->opts.usbopts.ports;
>  return 8;
>  
> +case VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT:

Coverity points out that because at the top of this function, we have:

if (model == -1)
model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI;

this particular condition cannot be met - IOW: DEADCODE

Since the code isn't changing cont->model - we could just remove the
@model local and move the DEFAULT into the return 2 leaving some sort of
comment (perhaps) that DEFAULT == PIIX3_UHCI?

>  case VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE:
>  case VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST:
>  break;

[...]

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 6c73cd7bfe..2a75a169c2 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c

[...]

> @@ -2771,9 +2777,14 @@ qemuBuildControllerDevStr(const virDomainDef 
> *domainDef,
>  virBufferAsprintf(, ",numa_node=%d", pciopts->numaNode);
>  break;
>  case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("Unsupported PCI-E root controller"));

PCI-Express

> +goto error;
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT:
>  case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -   _("wrong function called"));
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Unexpected PCI controller model %d"),
> +   def->model);
>  goto error;
>  }
>  break;

[...]

> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> index e28119e4b1..de565dbf74 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -513,6 +513,16 @@ 
> qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
>  
>  case VIR_DOMAIN_CONTROLLER_TYPE_USB:
>  switch ((virDomainControllerModelUSB) cont->model) {
> +case VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT:
> +/* XXX it isn't clear that this is the right
> + * thing to do here. We probably ought to
> + * return 0, but that breaks test suite right
> + * now because we call this method before we
> + * have turned the _DEFAULT case into a
> + * specific choice
> + */
> +return pciFlags;
> +
>  case VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI:
>  case VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI:
>  return pcieFlags;
> @@ -540,6 +550,16 @@ 
> qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
>  
>  case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
>  switch ((virDomainControllerModelSCSI) cont->model) {
> +case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT:
> +/* XXX it isn't clear that this is the right
> + * thing to do here. We probably 

[libvirt] [PATCH] daemon: trigger RPC re-generation when Makefile.am changes

[Daniel P . Berrangé]

The src/Makefile.am rules all re-generate the RPC dispatch code whenever
the Makefile.am changes, so for consistency do that for
daemon/Makefile.am too.

Signed-off-by: Daniel P. Berrangé 
---
 daemon/Makefile.am | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/daemon/Makefile.am b/daemon/Makefile.am
index 77dfd8943a..42d8f7b3fd 100644
--- a/daemon/Makefile.am
+++ b/daemon/Makefile.am
@@ -92,19 +92,19 @@ QEMU_PROTOCOL = $(top_srcdir)/src/remote/qemu_protocol.x
 ADMIN_PROTOCOL = $(top_srcdir)/src/admin/admin_protocol.x
 
 remote_dispatch.h: $(top_srcdir)/src/rpc/gendispatch.pl \
-   $(REMOTE_PROTOCOL)
+   $(REMOTE_PROTOCOL) Makefile.am
$(AM_V_GEN)$(PERL) -w $(top_srcdir)/src/rpc/gendispatch.pl \
  --mode=server remote REMOTE $(REMOTE_PROTOCOL) \
  > $(srcdir)/remote_dispatch.h
 
 lxc_dispatch.h: $(top_srcdir)/src/rpc/gendispatch.pl \
-   $(LXC_PROTOCOL)
+   $(LXC_PROTOCOL) Makefile.am
$(AM_V_GEN)$(PERL) -w $(top_srcdir)/src/rpc/gendispatch.pl \
  --mode=server lxc LXC $(LXC_PROTOCOL) \
  > $(srcdir)/lxc_dispatch.h
 
 qemu_dispatch.h: $(top_srcdir)/src/rpc/gendispatch.pl \
-   $(QEMU_PROTOCOL)
+   $(QEMU_PROTOCOL) Makefile.am
$(AM_V_GEN)$(PERL) -w $(top_srcdir)/src/rpc/gendispatch.pl \
  --mode=server qemu QEMU $(QEMU_PROTOCOL) \
  > $(srcdir)/qemu_dispatch.h
-- 
2.14.3

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

Re: [libvirt] [PATCH] docs: Document pcie-root requirement for q35 guests

[Daniel P . Berrangé]

On Mon, Feb 19, 2018 at 05:38:35PM +0100, Andrea Bolognani wrote:
> When you add a bunch of pcie-root-port controllers to a q35 guest
> in order to have hotplug capabilities, you also need to make sure
> you're adding the pcie-root controller at the same time or you
> will get an error. Document this fact.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  docs/pci-hotplug.html.in | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/pci-hotplug.html.in b/docs/pci-hotplug.html.in
> index 6e0648ee2..4ac87d644 100644
> --- a/docs/pci-hotplug.html.in
> +++ b/docs/pci-hotplug.html.in
> @@ -72,6 +72,7 @@
>  
>  
>  
> +controller type='pci' model='pcie-root'/
>  controller type='pci' model='pcie-root-port'/
>  controller type='pci' model='pcie-root-port'/
>  controller type='pci' model='pcie-root-port'/
> @@ -80,7 +81,10 @@
>if you expect to hotplug up to three PCI Express devices,
>either emulated or assigned from the host. That's all the
>information you need to provide: libvirt will fill in the
> -  remaining details automatically.
> +  remaining details automatically. Note that you need to add
> +  the pcie-root controller along with the
> +  pcie-root-port controllers or you will get an
> +  error.
>  
>  
>Note that if you're adding PCI controllers to a guest and at
> --

Reviewed-by: Daniel P. Berrangé 

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

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

[libvirt] [PATCH] docs: Document pcie-root requirement for q35 guests

[Andrea Bolognani]

When you add a bunch of pcie-root-port controllers to a q35 guest
in order to have hotplug capabilities, you also need to make sure
you're adding the pcie-root controller at the same time or you
will get an error. Document this fact.

Signed-off-by: Andrea Bolognani 
---
 docs/pci-hotplug.html.in | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/docs/pci-hotplug.html.in b/docs/pci-hotplug.html.in
index 6e0648ee2..4ac87d644 100644
--- a/docs/pci-hotplug.html.in
+++ b/docs/pci-hotplug.html.in
@@ -72,6 +72,7 @@
 
 
 
+controller type='pci' model='pcie-root'/
 controller type='pci' model='pcie-root-port'/
 controller type='pci' model='pcie-root-port'/
 controller type='pci' model='pcie-root-port'/
@@ -80,7 +81,10 @@
   if you expect to hotplug up to three PCI Express devices,
   either emulated or assigned from the host. That's all the
   information you need to provide: libvirt will fill in the
-  remaining details automatically.
+  remaining details automatically. Note that you need to add
+  the pcie-root controller along with the
+  pcie-root-port controllers or you will get an
+  error.
 
 
   Note that if you're adding PCI controllers to a guest and at
-- 
2.14.3

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

[libvirt] [PATCH] docs: Fix indentation of inlined JavaScript snippet

[Andrea Bolognani]

Signed-off-by: Andrea Bolognani 
---
Pushed as trivial.

 docs/page.xsl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/page.xsl b/docs/page.xsl
index 3a64a06c5..c1804eab3 100644
--- a/docs/page.xsl
+++ b/docs/page.xsl
@@ -103,7 +103,7 @@
   
   
-- 
2.14.3

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

Re: [libvirt] [PATCH v2 04/15] qemu: Restrict scope in qemuBuildControllerDevStr()

[Andrea Bolognani]

On Mon, 2018-02-19 at 16:00 +0100, Peter Krempa wrote:
> > @@ -2625,8 +2625,6 @@ qemuBuildControllerDevStr(const virDomainDef 
> > *domainDef,
> >  {
> >  virBuffer buf = VIR_BUFFER_INITIALIZER;
> >  int address_type = def->info.type;
> > -const virDomainPCIControllerOpts *pciopts;
> > -const char *modelName = NULL;
> >  
> >  *devstr = NULL;
> 
> Since the next patch changes both lines again I suggest you squash it
> into the next patch.

They're semantically independent changes so I thought it would be
better to perform them separately, but I can squash them together
if that's your preference.

-- 
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 05/15] qemu: Simplify modelName stringification

[Andrea Bolognani]

On Mon, 2018-02-19 at 16:06 +0100, Peter Krempa wrote:
> >  case VIR_DOMAIN_CONTROLLER_TYPE_PCI: {
> > -const virDomainPCIControllerOpts *pciopts;
> > -const char *modelName = NULL;
> > -
> > -pciopts = >opts.pciopts;
> > -if (def->model != VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT &&
> > -def->model != VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST)
> > -modelName = 
> > virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
> > +const virDomainPCIControllerOpts *pciopts = >opts.pciopts;
> > +const char *modelName = 
> > virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
> 
> This is not equivalent. virDomainControllerPCIModelName implementation
> actually defines a string for VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT so
> it will not return NULL and the commit message does not explain why/if
> it is actually okay.

The result doesn't change because the PCIE_ROOT case in the switch
statement immediately following the conversion does nothing but
error out, which makes whether or not modelName is NULL irrelevant.

I can add the explanation above to the commit message if you want.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v3] qemu: rename migration APIs to include Src or Dst in their name

[John Ferlan]

On 02/19/2018 06:46 AM, Daniel P. Berrangé wrote:
> It is very difficult while reading the migration code trying to
> understand whether a particular function is being called on the src side
> or the dst side, or either. Putting "Src" or "Dst" in the method names will
> make this much more obvious. "Any" is used in a few helpers which can be
> called from both sides.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
> 
> Changed in v3:
> 
>  - Made most naming suggestions from John.
> 
>  src/qemu/qemu_driver.c|  194 +++
>  src/qemu/qemu_migration.c | 1378 
> +++--
>  src/qemu/qemu_migration.h |  264 +
>  src/qemu/qemu_process.c   |   20 +-
>  tests/qemuxml2argvtest.c  |4 +-
>  5 files changed, 940 insertions(+), 920 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH v2 03/15] qemu: Move skip for implicit PHB of pSeries guests

[Andrea Bolognani]

On Mon, 2018-02-19 at 15:58 +0100, Peter Krempa wrote:
> On Fri, Feb 16, 2018 at 17:28:00 +0100, Andrea Bolognani wrote:
> > Performing the skip earlier will help us making the function
> > nicer later on. We also make the condition for the skip a bit
> > more precise, though that'a more for self-documenting purposes
> > and doesn't change anything in practice.
> > 
> > Signed-off-by: Andrea Bolognani 
> > ---
> >  src/qemu/qemu_command.c | 12 
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 1ab5b0818..5e4dfcf75 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -2732,6 +2732,14 @@ qemuBuildControllerDevStr(const virDomainDef 
> > *domainDef,
> >  def->model != VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST)
> >  modelName = 
> > virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
> >  
> > +/* Skip the implicit PHB for pSeries guests */
> > +if (def->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT &&
> > +pciopts->modelName == 
> > VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE &&
> > +pciopts->targetIndex == 0 &&
> > +qemuDomainIsPSeries(domainDef)) {
> 
> I'm not sure about the last line. Shouldn't that alredy be validated? At
> least in the case when the PHB model should not be present on
> non-pseries qemus?
> 
> ACK if you agree and drop the last line.

Strictly speaking, both the check on modelName and that on the
machine type are redundant, because we make sure targetIndex is only
set for PHBs and thus only for pSeries guests.

That said, checking again doesn't hurt and makes the reason for
skipping more obvious. It might also help catch bugs introduced in
the validate callback that would result in a controller that was
supposed to be skipped showing up in the output for some test case.

-- 
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 05/15] qemu: Simplify modelName stringification

[Peter Krempa]

On Fri, Feb 16, 2018 at 17:28:02 +0100, Andrea Bolognani wrote:
> There's no need to perform checks before conversion, we can
> just go try and check the results later on.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  src/qemu/qemu_command.c | 16 +---
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 2291bf5da..a44a1b2d2 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2725,13 +2725,8 @@ qemuBuildControllerDevStr(const virDomainDef 
> *domainDef,
>  break;
>  
>  case VIR_DOMAIN_CONTROLLER_TYPE_PCI: {
> -const virDomainPCIControllerOpts *pciopts;
> -const char *modelName = NULL;
> -
> -pciopts = >opts.pciopts;
> -if (def->model != VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT &&
> -def->model != VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST)
> -modelName = 
> virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
> +const virDomainPCIControllerOpts *pciopts = >opts.pciopts;
> +const char *modelName = 
> virDomainControllerPCIModelNameTypeToString(pciopts->modelName);

This is not equivalent. virDomainControllerPCIModelName implementation
actually defines a string for VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT so
it will not return NULL and the commit message does not explain why/if
it is actually okay.

>  
>  /* Skip the implicit PHB for pSeries guests */
>  if (def->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT &&
> @@ -2741,6 +2736,13 @@ qemuBuildControllerDevStr(const virDomainDef 
> *domainDef,
>  goto done;
>  }
>  
> +if (!modelName) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Unknown virDomainControllerPCIModelName value: 
> %d"),

I think this was under discussion somewhere in this series.

> +   pciopts->modelName);
> +return -1;
> +}
> +
>  switch ((virDomainControllerModelPCI) def->model) {
>  case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
>  virBufferAsprintf(, "%s,chassis_nr=%d,id=%s",
> -- 
> 2.14.3
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list


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

Re: [libvirt] [PATCH v2 04/15] qemu: Restrict scope in qemuBuildControllerDevStr()

[Peter Krempa]

On Fri, Feb 16, 2018 at 17:28:01 +0100, Andrea Bolognani wrote:
> Some variables are only used for PCI controllers, and we're going
> to add more soon. We can declare them in the 'case' scope rather
> than in the function scope to make it a bit nicer.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  src/qemu/qemu_command.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 5e4dfcf75..2291bf5da 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2625,8 +2625,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
>  {
>  virBuffer buf = VIR_BUFFER_INITIALIZER;
>  int address_type = def->info.type;
> -const virDomainPCIControllerOpts *pciopts;
> -const char *modelName = NULL;
>  
>  *devstr = NULL;

Since the next patch changes both lines again I suggest you squash it
into the next patch.



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

Re: [libvirt] [PATCH v2 03/15] qemu: Move skip for implicit PHB of pSeries guests

[Peter Krempa]

On Fri, Feb 16, 2018 at 17:28:00 +0100, Andrea Bolognani wrote:
> Performing the skip earlier will help us making the function
> nicer later on. We also make the condition for the skip a bit
> more precise, though that'a more for self-documenting purposes
> and doesn't change anything in practice.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  src/qemu/qemu_command.c | 12 
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 1ab5b0818..5e4dfcf75 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2732,6 +2732,14 @@ qemuBuildControllerDevStr(const virDomainDef 
> *domainDef,
>  def->model != VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST)
>  modelName = 
> virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
>  
> +/* Skip the implicit PHB for pSeries guests */
> +if (def->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT &&
> +pciopts->modelName == 
> VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE &&
> +pciopts->targetIndex == 0 &&
> +qemuDomainIsPSeries(domainDef)) {

I'm not sure about the last line. Shouldn't that alredy be validated? At
least in the case when the PHB model should not be present on
non-pseries qemus?

ACK if you agree and drop the last line.


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

Re: [libvirt] [PATCH v2 02/15] qemu: Move 'done' label in qemuBuildControllerDevStr()

[Peter Krempa]

On Fri, Feb 16, 2018 at 17:27:59 +0100, Andrea Bolognani wrote:
> Even when we skip part of the processing, we still want error
> checking on the buffer.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  src/qemu/qemu_command.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

ACK


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

Re: [libvirt] [PATCH v2 06/15] qemu: Defer capability check to command line generation time

[Andrea Bolognani]

On Mon, 2018-02-19 at 14:30 +0100, Peter Krempa wrote:
> On Mon, Feb 19, 2018 at 13:21:35 +, Daniel Berrange wrote:
> > On Mon, Feb 19, 2018 at 02:13:14PM +0100, Peter Krempa wrote:
> > > The only situation when we should not fail if QEMU is not installed and
> > > you restart libvirtd. Making defined domains disappear is bad.

I think we can all agree on that :)

> > A long time back we did have a patch series somewhere that tried to
> > keep domain's loaded, but mark them as "broken" if the QEMU we needed
> > was missing. Can't remember why we never went further with it now...
> 
> Well in the meanwhile I've made qemuCaps in the PostParse callbacks
> optional and it's re-run in such case on startup of the VM. The
> validation callbacks are not run when parsing status/config XMLs so it
> should mostly work right now properly. (at least it fixed my case when
> my binaries failed to exec due to broken deps)

Okay, I'll leave capabilities checks in the validate callback.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [python PATCH 3/4] Add missing virPyDictToTypedParams hint for migration params

[Daniel P . Berrangé]

On Mon, Feb 19, 2018 at 03:22:00PM +0100, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina 
> ---
>  libvirt-override.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Daniel P. Berrangé 
 

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

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

Re: [libvirt] [python PATCH 4/4] Fix virPyDictToTypedParams type hint for block copy params

[Daniel P . Berrangé]

On Mon, Feb 19, 2018 at 03:22:01PM +0100, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina 
> ---
>  libvirt-override.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé 


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

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

Re: [libvirt] [python PATCH 2/4] Fix order of virPyDictToTypedParams hints

[Daniel P . Berrangé]

On Mon, Feb 19, 2018 at 03:21:59PM +0100, Pavel Hrdina wrote:
> This corresponds to the order in libvirt-domain.h header file.
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  libvirt-override.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Daniel P. Berrangé 


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

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

Re: [libvirt] [python PATCH 1/4] Use static variables to store virPyDictToTypedParams hints

[Daniel P . Berrangé]

On Mon, Feb 19, 2018 at 03:21:58PM +0100, Pavel Hrdina wrote:
> There is no need to have dynamic allocation every time the API
> is called.  Rewrites commit <314b2346df>.
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  libvirt-override.c | 149 
> -
>  libvirt-utils.h|   2 +
>  2 files changed, 34 insertions(+), 117 deletions(-)

Reviewed-by: Daniel P. Berrangé 

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

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

[libvirt] [python PATCH 2/4] Fix order of virPyDictToTypedParams hints

[Pavel Hrdina]

This corresponds to the order in libvirt-domain.h header file.

Signed-off-by: Pavel Hrdina 
---
 libvirt-override.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/libvirt-override.c b/libvirt-override.c
index ab4232f..4f546b4 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -7743,16 +7743,16 @@ static virPyTypedParamsHint virPyDomainMigrate3Params[] 
= {
 { VIR_MIGRATE_PARAM_URI, VIR_TYPED_PARAM_STRING },
 { VIR_MIGRATE_PARAM_DEST_NAME, VIR_TYPED_PARAM_STRING },
 { VIR_MIGRATE_PARAM_DEST_XML, VIR_TYPED_PARAM_STRING },
-{ VIR_MIGRATE_PARAM_GRAPHICS_URI, VIR_TYPED_PARAM_STRING },
+{ VIR_MIGRATE_PARAM_PERSIST_XML, VIR_TYPED_PARAM_STRING },
 { VIR_MIGRATE_PARAM_BANDWIDTH, VIR_TYPED_PARAM_ULLONG },
+{ VIR_MIGRATE_PARAM_GRAPHICS_URI, VIR_TYPED_PARAM_STRING },
 { VIR_MIGRATE_PARAM_LISTEN_ADDRESS, VIR_TYPED_PARAM_STRING },
 { VIR_MIGRATE_PARAM_DISKS_PORT, VIR_TYPED_PARAM_INT },
 { VIR_MIGRATE_PARAM_COMPRESSION, VIR_TYPED_PARAM_STRING },
-{ VIR_MIGRATE_PARAM_COMPRESSION_MT_DTHREADS, VIR_TYPED_PARAM_INT },
 { VIR_MIGRATE_PARAM_COMPRESSION_MT_LEVEL, VIR_TYPED_PARAM_INT },
 { VIR_MIGRATE_PARAM_COMPRESSION_MT_THREADS, VIR_TYPED_PARAM_INT },
+{ VIR_MIGRATE_PARAM_COMPRESSION_MT_DTHREADS, VIR_TYPED_PARAM_INT },
 { VIR_MIGRATE_PARAM_COMPRESSION_XBZRLE_CACHE, VIR_TYPED_PARAM_ULLONG },
-{ VIR_MIGRATE_PARAM_PERSIST_XML, VIR_TYPED_PARAM_STRING },
 { VIR_MIGRATE_PARAM_AUTO_CONVERGE_INITIAL, VIR_TYPED_PARAM_INT },
 { VIR_MIGRATE_PARAM_AUTO_CONVERGE_INCREMENT, VIR_TYPED_PARAM_INT },
 };
-- 
2.14.3

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

[libvirt] [python PATCH 4/4] Fix virPyDictToTypedParams type hint for block copy params

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina 
---
 libvirt-override.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libvirt-override.c b/libvirt-override.c
index 59c1e0c..580c259 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -8668,7 +8668,7 @@ libvirt_virDomainListGetStats(PyObject *self 
ATTRIBUTE_UNUSED,
 static virPyTypedParamsHint virPyDomainBlockCopyParams[] = {
 { VIR_DOMAIN_BLOCK_COPY_BANDWIDTH, VIR_TYPED_PARAM_ULLONG },
 { VIR_DOMAIN_BLOCK_COPY_GRANULARITY, VIR_TYPED_PARAM_UINT },
-{ VIR_DOMAIN_BLOCK_COPY_BUF_SIZE, VIR_TYPED_PARAM_UINT },
+{ VIR_DOMAIN_BLOCK_COPY_BUF_SIZE, VIR_TYPED_PARAM_ULLONG },
 };
 
 
-- 
2.14.3

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

[libvirt] [python PATCH 3/4] Add missing virPyDictToTypedParams hint for migration params

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina 
---
 libvirt-override.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libvirt-override.c b/libvirt-override.c
index 4f546b4..59c1e0c 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -7747,6 +7747,7 @@ static virPyTypedParamsHint virPyDomainMigrate3Params[] = 
{
 { VIR_MIGRATE_PARAM_BANDWIDTH, VIR_TYPED_PARAM_ULLONG },
 { VIR_MIGRATE_PARAM_GRAPHICS_URI, VIR_TYPED_PARAM_STRING },
 { VIR_MIGRATE_PARAM_LISTEN_ADDRESS, VIR_TYPED_PARAM_STRING },
+{ VIR_MIGRATE_PARAM_MIGRATE_DISKS, VIR_TYPED_PARAM_STRING },
 { VIR_MIGRATE_PARAM_DISKS_PORT, VIR_TYPED_PARAM_INT },
 { VIR_MIGRATE_PARAM_COMPRESSION, VIR_TYPED_PARAM_STRING },
 { VIR_MIGRATE_PARAM_COMPRESSION_MT_LEVEL, VIR_TYPED_PARAM_INT },
-- 
2.14.3

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

[libvirt] [python PATCH 0/4] followup fixes for virPyDictToTypedParams

[Pavel Hrdina]

Pavel Hrdina (4):
  Use static variables to store virPyDictToTypedParams hints
  Fix order of virPyDictToTypedParams hints
  Add missing virPyDictToTypedParams hint for migration params
  Fix virPyDictToTypedParams type hint for block copy params

 libvirt-override.c | 150 -
 libvirt-utils.h|   2 +
 2 files changed, 35 insertions(+), 117 deletions(-)

-- 
2.14.3

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

[libvirt] [python PATCH 1/4] Use static variables to store virPyDictToTypedParams hints

[Pavel Hrdina]

There is no need to have dynamic allocation every time the API
is called.  Rewrites commit <314b2346df>.

Signed-off-by: Pavel Hrdina 
---
 libvirt-override.c | 149 -
 libvirt-utils.h|   2 +
 2 files changed, 34 insertions(+), 117 deletions(-)

diff --git a/libvirt-override.c b/libvirt-override.c
index dba42d4..ab4232f 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -7739,6 +7739,25 @@ libvirt_virDomainMigrateGetMaxDowntime(PyObject *self 
ATTRIBUTE_UNUSED,
 #endif /* LIBVIR_CHECK_VERSION(3, 7, 0) */
 
 #if LIBVIR_CHECK_VERSION(1, 1, 0)
+static virPyTypedParamsHint virPyDomainMigrate3Params[] = {
+{ VIR_MIGRATE_PARAM_URI, VIR_TYPED_PARAM_STRING },
+{ VIR_MIGRATE_PARAM_DEST_NAME, VIR_TYPED_PARAM_STRING },
+{ VIR_MIGRATE_PARAM_DEST_XML, VIR_TYPED_PARAM_STRING },
+{ VIR_MIGRATE_PARAM_GRAPHICS_URI, VIR_TYPED_PARAM_STRING },
+{ VIR_MIGRATE_PARAM_BANDWIDTH, VIR_TYPED_PARAM_ULLONG },
+{ VIR_MIGRATE_PARAM_LISTEN_ADDRESS, VIR_TYPED_PARAM_STRING },
+{ VIR_MIGRATE_PARAM_DISKS_PORT, VIR_TYPED_PARAM_INT },
+{ VIR_MIGRATE_PARAM_COMPRESSION, VIR_TYPED_PARAM_STRING },
+{ VIR_MIGRATE_PARAM_COMPRESSION_MT_DTHREADS, VIR_TYPED_PARAM_INT },
+{ VIR_MIGRATE_PARAM_COMPRESSION_MT_LEVEL, VIR_TYPED_PARAM_INT },
+{ VIR_MIGRATE_PARAM_COMPRESSION_MT_THREADS, VIR_TYPED_PARAM_INT },
+{ VIR_MIGRATE_PARAM_COMPRESSION_XBZRLE_CACHE, VIR_TYPED_PARAM_ULLONG },
+{ VIR_MIGRATE_PARAM_PERSIST_XML, VIR_TYPED_PARAM_STRING },
+{ VIR_MIGRATE_PARAM_AUTO_CONVERGE_INITIAL, VIR_TYPED_PARAM_INT },
+{ VIR_MIGRATE_PARAM_AUTO_CONVERGE_INCREMENT, VIR_TYPED_PARAM_INT },
+};
+
+
 static PyObject *
 libvirt_virDomainMigrate3(PyObject *self ATTRIBUTE_UNUSED,
   PyObject *args)
@@ -7750,9 +7769,7 @@ libvirt_virDomainMigrate3(PyObject *self ATTRIBUTE_UNUSED,
 PyObject *dict;
 unsigned int flags;
 virTypedParameterPtr params;
-virPyTypedParamsHintPtr hparams;
 int nparams = 0;
-int nhparams = 15;
 virDomainPtr ddom = NULL;
 
 if (!PyArg_ParseTuple(args, (char *) "OOOI:virDomainMigrate3",
@@ -7762,55 +7779,9 @@ libvirt_virDomainMigrate3(PyObject *self 
ATTRIBUTE_UNUSED,
 domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
 dconn = (virConnectPtr) PyvirConnect_Get(pyobj_dconn);
 
-hparams = malloc(sizeof(virPyTypedParamsHint) * nhparams);
-hparams[0].name = VIR_MIGRATE_PARAM_URI;
-hparams[0].type = VIR_TYPED_PARAM_STRING;
-
-hparams[1].name = VIR_MIGRATE_PARAM_DEST_NAME;
-hparams[1].type = VIR_TYPED_PARAM_STRING;
-
-hparams[2].name = VIR_MIGRATE_PARAM_DEST_XML;
-hparams[2].type = VIR_TYPED_PARAM_STRING;
-
-hparams[3].name = VIR_MIGRATE_PARAM_GRAPHICS_URI;
-hparams[3].type = VIR_TYPED_PARAM_STRING;
-
-hparams[4].name = VIR_MIGRATE_PARAM_BANDWIDTH;
-hparams[4].type = VIR_TYPED_PARAM_ULLONG;
-
-hparams[5].name = VIR_MIGRATE_PARAM_LISTEN_ADDRESS;
-hparams[5].type = VIR_TYPED_PARAM_STRING;
-
-hparams[6].name = VIR_MIGRATE_PARAM_DISKS_PORT;
-hparams[6].type = VIR_TYPED_PARAM_INT;
-
-hparams[7].name = VIR_MIGRATE_PARAM_COMPRESSION;
-hparams[7].type = VIR_TYPED_PARAM_STRING;
-
-hparams[8].name = VIR_MIGRATE_PARAM_COMPRESSION_MT_DTHREADS;
-hparams[8].type = VIR_TYPED_PARAM_INT;
-
-hparams[9].name = VIR_MIGRATE_PARAM_COMPRESSION_MT_LEVEL;
-hparams[9].type = VIR_TYPED_PARAM_INT;
-
-hparams[10].name = VIR_MIGRATE_PARAM_COMPRESSION_MT_THREADS;
-hparams[10].type = VIR_TYPED_PARAM_INT;
-
-hparams[11].name = VIR_MIGRATE_PARAM_COMPRESSION_XBZRLE_CACHE;
-hparams[11].type = VIR_TYPED_PARAM_ULLONG;
-
-hparams[12].name = VIR_MIGRATE_PARAM_PERSIST_XML;
-hparams[12].type = VIR_TYPED_PARAM_STRING;
-
-hparams[13].name = VIR_MIGRATE_PARAM_AUTO_CONVERGE_INITIAL;
-hparams[13].type = VIR_TYPED_PARAM_INT;
-
-hparams[14].name = VIR_MIGRATE_PARAM_AUTO_CONVERGE_INCREMENT;
-hparams[14].type = VIR_TYPED_PARAM_INT;
-
 if (virPyDictToTypedParams(dict, , ,
-   hparams, nhparams) < 0) {
-free(hparams);
+   virPyDomainMigrate3Params,
+   VIR_N_ELEMENTS(virPyDomainMigrate3Params)) < 0) 
{
 return NULL;
 }
 
@@ -7819,7 +7790,6 @@ libvirt_virDomainMigrate3(PyObject *self ATTRIBUTE_UNUSED,
 LIBVIRT_END_ALLOW_THREADS;
 
 virTypedParamsFree(params, nparams);
-free(hparams);
 return libvirt_virDomainPtrWrap(ddom);
 }
 
@@ -7833,9 +7803,7 @@ libvirt_virDomainMigrateToURI3(PyObject *self 
ATTRIBUTE_UNUSED,
 PyObject *dict;
 unsigned int flags;
 virTypedParameterPtr params;
-virPyTypedParamsHintPtr hparams;
 int nparams;
-int nhparams = 15;
 int ret = -1;
 
 if (!PyArg_ParseTuple(args, (char *) "OzOI:virDomainMigrate3",
@@ -7844,55 +7812,9 @@ libvirt_virDomainMigrateToURI3(PyObject *self 
ATTRIBUTE_UNUSED,
 
 domain = 

Re: [libvirt] [RFC] libvirt hexagon sticker design

[Daniel P . Berrangé]

On Mon, Feb 19, 2018 at 02:49:34PM +0100, Andrea Bolognani wrote:
> On Mon, 2018-02-19 at 13:25 +0100, Martin Kletzander wrote:
> > > Agreed. The lighter green background also mostly works, but only
> > > with the grey text variation of the logo.
> > 
> > I'd vote from second one from the top row.  Did you try making a circular 
> > (or
> > hexagonal) gradien tthat would start as white in the center and continued 
> > to the
> > outside where it would end up as a border of the same colour as the one I 
> > voted
> > for?  Might be very ugly, but also pretty nice, hard to say without any 
> > trying =)
> 
> I don't think this would work. On its own, possibly, but the whole
> point of these stickers is that you'll have a bunch of them side
> by side, and I've never seen another one with gradients. We don't
> want our to look out of place.

There's a small number with gradients here

http://hexb.in/

but that's basically cases where the rest of the logo is boring that they
need a gradient to make it look nicer. In libvirt case, I think that using
a gradiant would just distract from the logo. Just stick with white background
and a border so it has good contrast

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

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

Re: [libvirt] [RFC] libvirt hexagon sticker design

[Martin Kletzander]

On Mon, Feb 19, 2018 at 02:49:34PM +0100, Andrea Bolognani wrote:

On Mon, 2018-02-19 at 13:25 +0100, Martin Kletzander wrote:

> Agreed. The lighter green background also mostly works, but only
> with the grey text variation of the logo.

I'd vote from second one from the top row.  Did you try making a circular (or
hexagonal) gradien tthat would start as white in the center and continued to the
outside where it would end up as a border of the same colour as the one I voted
for?  Might be very ugly, but also pretty nice, hard to say without any trying 
=)


I don't think this would work. On its own, possibly, but the whole
point of these stickers is that you'll have a bunch of them side
by side, and I've never seen another one with gradients. We don't
want our to look out of place.



I have ;)  If that's enough.  I think Erik has some on his hexagon-sticker wall.


--
Andrea Bolognani / Red Hat / Virtualization


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

Re: [libvirt] [RFC] libvirt hexagon sticker design

[Andrea Bolognani]

On Mon, 2018-02-19 at 13:25 +0100, Martin Kletzander wrote:
> > Agreed. The lighter green background also mostly works, but only
> > with the grey text variation of the logo.
> 
> I'd vote from second one from the top row.  Did you try making a circular (or
> hexagonal) gradien tthat would start as white in the center and continued to 
> the
> outside where it would end up as a border of the same colour as the one I 
> voted
> for?  Might be very ugly, but also pretty nice, hard to say without any 
> trying =)

I don't think this would work. On its own, possibly, but the whole
point of these stickers is that you'll have a bunch of them side
by side, and I've never seen another one with gradients. We don't
want our to look out of place.

-- 
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 06/15] qemu: Defer capability check to command line generation time

[Peter Krempa]

On Mon, Feb 19, 2018 at 13:21:35 +, Daniel Berrange wrote:
> On Mon, Feb 19, 2018 at 02:13:14PM +0100, Peter Krempa wrote:
> > On Mon, Feb 19, 2018 at 13:04:08 +, Daniel Berrange wrote:
> > > On Mon, Feb 19, 2018 at 10:29:18AM +0100, Andrea Bolognani wrote:
> > > > On Mon, 2018-02-19 at 07:24 +0100, Peter Krempa wrote:
> > > > > On Fri, Feb 16, 2018 at 17:28:03 +0100, Andrea Bolognani wrote:
> > > > > > Validate time is a bit too early to check whether the required
> > > > > > capabilities are available, since the QEMU binary might have
> > > > > > been updated or replaced by the time we are asked to run the
> > > > > > guest.
> > > > > 
> > > > > So are you having problem with the fact that the definition will be
> > > > > rejected right away and not just when you try to start it?
> > > > > 
> > > > > Validate is re-run when starting the VM so a downgrade is handled
> > > > > properly.
> > > > 
> > > > Right, but isn't checking for QEMU capabilities at validate time
> > > > unreasonably strict? A guest which uses eg. an invalid combination
> > > > of machine type and architecture will never become valid at a later
> > > > point, but a guest should not be considered invalid just because
> > > > the QEMU binary you happened to have installed at the time you
> > > > defined it lacked some features - the guest itself is perfectly
> > > > valid, it just can't be run :)
> > > 
> > > Given that we increasingly fill in alot of information in the XML at 
> > > define
> > > time, we already have a general expectation that the QEMU binary will  be
> > > present at define time. I think this is not unreasonable - we suggest apps
> > > call virConnectGetCapabilities and/or virDomainGetCapabilities to 
> > > understand
> > > what is installed/available before creating an XML document to define. 
> > > Those
> > > APIs of course require binaries to be installed too.   So I don't think we
> > > should really put effort into coping with defining XML for a time when the
> > > QEMU binaries aren't installed.  Such a scenario is so unlikely to be hit
> > > that any code trying to cope with that is going to be largely untested and
> > > fragile, so it would be a disservice to pretend it'll be something worth
> > > relying on.
> > 
> > The only situation when we should not fail if QEMU is not installed and
> > you restart libvirtd. Making defined domains disappear is bad.
> 
> A long time back we did have a patch series somewhere that tried to
> keep domain's loaded, but mark them as "broken" if the QEMU we needed
> was missing. Can't remember why we never went further with it now...

Well in the meanwhile I've made qemuCaps in the PostParse callbacks
optional and it's re-run in such case on startup of the VM. The
validation callbacks are not run when parsing status/config XMLs so it
should mostly work right now properly. (at least it fixed my case when
my binaries failed to exec due to broken deps)


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

Re: [libvirt] [PATCH python 1/1] Set hints for virPyDictToTypedParams

[Daniel P . Berrangé]

On Mon, Feb 19, 2018 at 02:20:30PM +0100, Pavel Hrdina wrote:
> On Mon, Feb 19, 2018 at 01:09:27PM +, Daniel P. Berrangé wrote:
> > On Wed, Feb 07, 2018 at 05:49:30PM +0300, Edgar Kaziakhmedov wrote:
> > > Predefine hints for all parameters possible to avoid wrong type
> > > convert.
> > > 
> > > Signed-off-by: Edgar Kaziakhmedov 
> > > ---
> > >  libvirt-override.c | 128 
> > > +++--
> > >  1 file changed, 124 insertions(+), 4 deletions(-)
> > 
> > Thanks, looks good, so I've pushed this now.
> 
> Actually I wanted to suggest to use static variables to store the hints:

If you want to send a followup todo that its fine by me...



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

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

[libvirt] [PATCH] bhyve: Fix build

[Andrea Bolognani]

Commit 2d43f0a2dcfd dropped virDomainDiskTranslateSourcePool()'s
first argument but failed to update callers in the bhyve driver.

Signed-off-by: Andrea Bolognani 
---
Pushed under the build breaker rule.

 src/bhyve/bhyve_command.c | 21 ++---
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
index c1241b811..fd738b42c 100644
--- a/src/bhyve/bhyve_command.c
+++ b/src/bhyve/bhyve_command.c
@@ -198,7 +198,7 @@ bhyveBuildAHCIControllerArgStr(const virDomainDef *def,
 goto error;
 }
 
-if (virDomainDiskTranslateSourcePool(conn, disk) < 0)
+if (virDomainDiskTranslateSourcePool(disk) < 0)
 goto error;
 
 disk_source = virDomainDiskGetSource(disk);
@@ -289,12 +289,11 @@ bhyveBuildUSBControllerArgStr(const virDomainDef *def,
 static int
 bhyveBuildVirtIODiskArgStr(const virDomainDef *def ATTRIBUTE_UNUSED,
  virDomainDiskDefPtr disk,
- virConnectPtr conn,
  virCommandPtr cmd)
 {
 const char *disk_source;
 
-if (virDomainDiskTranslateSourcePool(conn, disk) < 0)
+if (virDomainDiskTranslateSourcePool(disk) < 0)
 return -1;
 
 if (disk->device != VIR_DOMAIN_DISK_DEVICE_DISK) {
@@ -562,7 +561,7 @@ virBhyveProcessBuildBhyveCmd(virConnectPtr conn,
 /* Handled by bhyveBuildAHCIControllerArgStr() */
 break;
 case VIR_DOMAIN_DISK_BUS_VIRTIO:
-if (bhyveBuildVirtIODiskArgStr(def, disk, conn, cmd) < 0)
+if (bhyveBuildVirtIODiskArgStr(def, disk, cmd) < 0)
 goto error;
 break;
 default:
@@ -672,10 +671,10 @@ virBhyveProcessBuildCustomLoaderCmd(virDomainDefPtr def)
 }
 
 static bool
-virBhyveUsableDisk(virConnectPtr conn, virDomainDiskDefPtr disk)
+virBhyveUsableDisk(virDomainDiskDefPtr disk)
 {
 
-if (virDomainDiskTranslateSourcePool(conn, disk) < 0)
+if (virDomainDiskTranslateSourcePool(disk) < 0)
 return false;
 
 if ((disk->device != VIR_DOMAIN_DISK_DEVICE_DISK) &&
@@ -729,7 +728,7 @@ virBhyveProcessBuildGrubbhyveCmd(virDomainDefPtr def,
  * across. */
 cd = hdd = userdef = NULL;
 for (i = 0; i < def->ndisks; i++) {
-if (!virBhyveUsableDisk(conn, def->disks[i]))
+if (!virBhyveUsableDisk(def->disks[i]))
 continue;
 
 diskdef = def->disks[i];
@@ -815,7 +814,7 @@ virBhyveProcessBuildGrubbhyveCmd(virDomainDefPtr def,
 }
 
 static virDomainDiskDefPtr
-virBhyveGetBootDisk(virConnectPtr conn, virDomainDefPtr def)
+virBhyveGetBootDisk(virDomainDefPtr def)
 {
 size_t i;
 virDomainDiskDefPtr match = NULL;
@@ -851,7 +850,7 @@ virBhyveGetBootDisk(virConnectPtr conn, virDomainDefPtr def)
 /* If boot_dev is set, we return the first device of
  * the request type */
 for (i = 0; i < def->ndisks; i++) {
-if (!virBhyveUsableDisk(conn, def->disks[i]))
+if (!virBhyveUsableDisk(def->disks[i]))
 continue;
 
 if (def->disks[i]->device == boot_dev) {
@@ -875,7 +874,7 @@ virBhyveGetBootDisk(virConnectPtr conn, virDomainDefPtr def)
 int first_usable_disk_index = -1;
 
 for (i = 0; i < def->ndisks; i++) {
-if (!virBhyveUsableDisk(conn, def->disks[i]))
+if (!virBhyveUsableDisk(def->disks[i]))
 continue;
 else
 first_usable_disk_index = i;
@@ -907,7 +906,7 @@ virBhyveProcessBuildLoadCmd(virConnectPtr conn, 
virDomainDefPtr def,
 virDomainDiskDefPtr disk = NULL;
 
 if (def->os.bootloader == NULL) {
-disk = virBhyveGetBootDisk(conn, def);
+disk = virBhyveGetBootDisk(def);
 
 if (disk == NULL)
 return NULL;
-- 
2.14.3

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

Re: [libvirt] [PATCH v2 06/15] qemu: Defer capability check to command line generation time

[Daniel P . Berrangé]

On Mon, Feb 19, 2018 at 02:13:14PM +0100, Peter Krempa wrote:
> On Mon, Feb 19, 2018 at 13:04:08 +, Daniel Berrange wrote:
> > On Mon, Feb 19, 2018 at 10:29:18AM +0100, Andrea Bolognani wrote:
> > > On Mon, 2018-02-19 at 07:24 +0100, Peter Krempa wrote:
> > > > On Fri, Feb 16, 2018 at 17:28:03 +0100, Andrea Bolognani wrote:
> > > > > Validate time is a bit too early to check whether the required
> > > > > capabilities are available, since the QEMU binary might have
> > > > > been updated or replaced by the time we are asked to run the
> > > > > guest.
> > > > 
> > > > So are you having problem with the fact that the definition will be
> > > > rejected right away and not just when you try to start it?
> > > > 
> > > > Validate is re-run when starting the VM so a downgrade is handled
> > > > properly.
> > > 
> > > Right, but isn't checking for QEMU capabilities at validate time
> > > unreasonably strict? A guest which uses eg. an invalid combination
> > > of machine type and architecture will never become valid at a later
> > > point, but a guest should not be considered invalid just because
> > > the QEMU binary you happened to have installed at the time you
> > > defined it lacked some features - the guest itself is perfectly
> > > valid, it just can't be run :)
> > 
> > Given that we increasingly fill in alot of information in the XML at define
> > time, we already have a general expectation that the QEMU binary will  be
> > present at define time. I think this is not unreasonable - we suggest apps
> > call virConnectGetCapabilities and/or virDomainGetCapabilities to understand
> > what is installed/available before creating an XML document to define. Those
> > APIs of course require binaries to be installed too.   So I don't think we
> > should really put effort into coping with defining XML for a time when the
> > QEMU binaries aren't installed.  Such a scenario is so unlikely to be hit
> > that any code trying to cope with that is going to be largely untested and
> > fragile, so it would be a disservice to pretend it'll be something worth
> > relying on.
> 
> The only situation when we should not fail if QEMU is not installed and
> you restart libvirtd. Making defined domains disappear is bad.

A long time back we did have a patch series somewhere that tried to
keep domain's loaded, but mark them as "broken" if the QEMU we needed
was missing. Can't remember why we never went further with it now...

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

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

Re: [libvirt] [PATCH python 1/1] Set hints for virPyDictToTypedParams

[Pavel Hrdina]

On Mon, Feb 19, 2018 at 01:09:27PM +, Daniel P. Berrangé wrote:
> On Wed, Feb 07, 2018 at 05:49:30PM +0300, Edgar Kaziakhmedov wrote:
> > Predefine hints for all parameters possible to avoid wrong type
> > convert.
> > 
> > Signed-off-by: Edgar Kaziakhmedov 
> > ---
> >  libvirt-override.c | 128 
> > +++--
> >  1 file changed, 124 insertions(+), 4 deletions(-)
> 
> Thanks, looks good, so I've pushed this now.

Actually I wanted to suggest to use static variables to store the hints:


diff --git a/libvirt-override.c b/libvirt-override.c
index 78a7f08..c41c03c 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -7739,6 +7739,25 @@ libvirt_virDomainMigrateGetMaxDowntime(PyObject *self 
ATTRIBUT
E_UNUSED,
 #endif /* LIBVIR_CHECK_VERSION(3, 7, 0) */

 #if LIBVIR_CHECK_VERSION(1, 1, 0)
+
+static virPyTypedParamsHint virDomainMigrate3Params[] = {
+{ VIR_MIGRATE_PARAM_URI, VIR_TYPED_PARAM_STRING },
+{ VIR_MIGRATE_PARAM_DEST_NAME, VIR_TYPED_PARAM_STRING },
+{ VIR_MIGRATE_PARAM_DEST_XML, VIR_TYPED_PARAM_STRING },
+{ VIR_MIGRATE_PARAM_GRAPHICS_URI, VIR_TYPED_PARAM_STRING },
+{ VIR_MIGRATE_PARAM_BANDWIDTH, VIR_TYPED_PARAM_ULLONG },
+{ VIR_MIGRATE_PARAM_LISTEN_ADDRESS, VIR_TYPED_PARAM_STRING },
+{ VIR_MIGRATE_PARAM_DISKS_PORT, VIR_TYPED_PARAM_INT },
+{ VIR_MIGRATE_PARAM_COMPRESSION, VIR_TYPED_PARAM_STRING },
+{ VIR_MIGRATE_PARAM_COMPRESSION_MT_DTHREADS, VIR_TYPED_PARAM_INT },
+{ VIR_MIGRATE_PARAM_COMPRESSION_MT_LEVEL, VIR_TYPED_PARAM_INT },
+{ VIR_MIGRATE_PARAM_COMPRESSION_MT_THREADS, VIR_TYPED_PARAM_INT },
+{ VIR_MIGRATE_PARAM_COMPRESSION_XBZRLE_CACHE, VIR_TYPED_PARAM_ULLONG },
+{ VIR_MIGRATE_PARAM_PERSIST_XML, VIR_TYPED_PARAM_STRING },
+{ VIR_MIGRATE_PARAM_AUTO_CONVERGE_INITIAL, VIR_TYPED_PARAM_INT },
+{ VIR_MIGRATE_PARAM_AUTO_CONVERGE_INCREMENT, VIR_TYPED_PARAM_INT },
+};
+
 static PyObject *
 libvirt_virDomainMigrate3(PyObject *self ATTRIBUTE_UNUSED,
   PyObject *args)
@@ -7760,7 +7779,9 @@ libvirt_virDomainMigrate3(PyObject *self ATTRIBUTE_UNUSED,
 domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
 dconn = (virConnectPtr) PyvirConnect_Get(pyobj_dconn);

-if (virPyDictToTypedParams(dict, , , NULL, 0) < 0)
+if (virPyDictToTypedParams(dict, , ,
+   virDomainMigrate3Params,
+   VIR_N_ELEMENTS(virDomainMigrate3Params)) < 0)
 return NULL;

 LIBVIRT_BEGIN_ALLOW_THREADS;
diff --git a/libvirt-utils.h b/libvirt-utils.h
index 779fd56..0af1e62 100644
--- a/libvirt-utils.h
+++ b/libvirt-utils.h
@@ -146,6 +146,8 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1);
 # endif


+#define VIR_N_ELEMENTS(array) (sizeof(array) / sizeof(*(array)))
+
 /* The two-statement sequence "Py_INCREF(Py_None); return Py_None;"
is so common that we encapsulate it here.  Now, each use is simply
return VIR_PY_NONE;  */


Which IMHO looks better.

Pavel


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

Re: [libvirt] [PATCH v2 06/15] qemu: Defer capability check to command line generation time

[Peter Krempa]

On Mon, Feb 19, 2018 at 13:04:08 +, Daniel Berrange wrote:
> On Mon, Feb 19, 2018 at 10:29:18AM +0100, Andrea Bolognani wrote:
> > On Mon, 2018-02-19 at 07:24 +0100, Peter Krempa wrote:
> > > On Fri, Feb 16, 2018 at 17:28:03 +0100, Andrea Bolognani wrote:
> > > > Validate time is a bit too early to check whether the required
> > > > capabilities are available, since the QEMU binary might have
> > > > been updated or replaced by the time we are asked to run the
> > > > guest.
> > > 
> > > So are you having problem with the fact that the definition will be
> > > rejected right away and not just when you try to start it?
> > > 
> > > Validate is re-run when starting the VM so a downgrade is handled
> > > properly.
> > 
> > Right, but isn't checking for QEMU capabilities at validate time
> > unreasonably strict? A guest which uses eg. an invalid combination
> > of machine type and architecture will never become valid at a later
> > point, but a guest should not be considered invalid just because
> > the QEMU binary you happened to have installed at the time you
> > defined it lacked some features - the guest itself is perfectly
> > valid, it just can't be run :)
> 
> Given that we increasingly fill in alot of information in the XML at define
> time, we already have a general expectation that the QEMU binary will  be
> present at define time. I think this is not unreasonable - we suggest apps
> call virConnectGetCapabilities and/or virDomainGetCapabilities to understand
> what is installed/available before creating an XML document to define. Those
> APIs of course require binaries to be installed too.   So I don't think we
> should really put effort into coping with defining XML for a time when the
> QEMU binaries aren't installed.  Such a scenario is so unlikely to be hit
> that any code trying to cope with that is going to be largely untested and
> fragile, so it would be a disservice to pretend it'll be something worth
> relying on.

The only situation when we should not fail if QEMU is not installed and
you restart libvirtd. Making defined domains disappear is bad.

The good thing is that at that point all defaults should be already
filled in so it should not matter much whether capabilities are present.

Other than that, I agree. Checking stuff uprfront is usually better than
get to the situation when it fails to start.


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

Re: [libvirt] [RFC] libvirt hexagon sticker design

[Kashyap Chamarthy]

On Mon, Feb 19, 2018 at 10:48:26AM +, Daniel P. Berrangé wrote:
> On Mon, Feb 19, 2018 at 11:36:03AM +0100, Kashyap Chamarthy wrote:
> > On Mon, Feb 19, 2018 at 09:44:36AM +0100, Erik Skultety wrote:

[...]

> > Speaking of logos...at the risk of opening a huge bucket of paint: 
> > 
> > The current libvirt logo is bit non-intuitive.  Unless you squint at it
> > to see what it might be about, you won't immediately get an idea what is
> > trying to tell you.  Not sure if there's appetite to rework the logo
> > itself.
> 
> I don't really want to go there - I really like our logo.
> 
> In general I don't think logos really need to explain what the project
> is, largely because doing so is impossible/impractical for most technical
> projects.

I agree that's a rabbit hole we don't want to go into, not least because
of the practicality point you mention.

> > Let me see if I can describe the image in words: "you're peeling off
> > some layer and you see little penguins (VMs) get churned out".
> 
> You know it is basically a "sardine tin", but with penguins right ?

Ah, I didn't know that; thanks for the education.  I heard of the term
Sardine (a kind of a small edible fish that is commonly shipped in a
tin) for the first time; just learnt from wikipedia.

> The tin is the computer, and the sardines (penguins) are the OS
> that are crammed inside it. That's a pretty good analogy for VMs
> IMHO.

In that context of "tinned sardines", and your explanation, it does make
for a decent analogy.  Interesting; I never connected the dots this way
(/me chalks it up to him being a vegetarian ;-)) and always wondered
about the meaning of the logo.  Now I see.  

Maybe a quick libvirt wiki page, "about logo" would be nice, explaining
what it is.  I think it is amusing to know.

> > (But that's from my biased POV of already knowing what libvirt is.)
> > 
> > For some inspiration, take a look at how the `curl` project went about
> > redesigning its logo[1].
> >
> > [1] https://daniel.haxx.se/blog/2016/05/27/a-new-curl-logo/
> 
> I don't think the new curl logo is any more "intuitive" in explaining
> what the project is than their old logo. Unless you already know that
> CURL is a library for downloading web content, the implications of the
> "://" will pass right over your head. The new curl logo is certainly a
> very nice improvement, but that's because the old one was really pretty
> crude so anything would be better. I wouldn't say it is any more
> intuitive though - just a nicer graphic design.

Yeah, I was indeed thinking: "Okay, the new logo _looks_ pretty, and
makes sense to those who know what `curl` is; but isn't all that
terribly intuitive for a newcomer."

Alright, your argument persuades me to not go down that route of a new
logo.

[...]


-- 
/kashyap

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

Re: [libvirt] [PATCH python 1/1] Set hints for virPyDictToTypedParams

[Daniel P . Berrangé]

On Wed, Feb 07, 2018 at 05:49:30PM +0300, Edgar Kaziakhmedov wrote:
> Predefine hints for all parameters possible to avoid wrong type
> convert.
> 
> Signed-off-by: Edgar Kaziakhmedov 
> ---
>  libvirt-override.c | 128 
> +++--
>  1 file changed, 124 insertions(+), 4 deletions(-)

Thanks, looks good, so I've pushed this now.


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

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

Re: [libvirt] [PATCH v2 06/15] qemu: Defer capability check to command line generation time

[Daniel P . Berrangé]

On Mon, Feb 19, 2018 at 10:29:18AM +0100, Andrea Bolognani wrote:
> On Mon, 2018-02-19 at 07:24 +0100, Peter Krempa wrote:
> > On Fri, Feb 16, 2018 at 17:28:03 +0100, Andrea Bolognani wrote:
> > > Validate time is a bit too early to check whether the required
> > > capabilities are available, since the QEMU binary might have
> > > been updated or replaced by the time we are asked to run the
> > > guest.
> > 
> > So are you having problem with the fact that the definition will be
> > rejected right away and not just when you try to start it?
> > 
> > Validate is re-run when starting the VM so a downgrade is handled
> > properly.
> 
> Right, but isn't checking for QEMU capabilities at validate time
> unreasonably strict? A guest which uses eg. an invalid combination
> of machine type and architecture will never become valid at a later
> point, but a guest should not be considered invalid just because
> the QEMU binary you happened to have installed at the time you
> defined it lacked some features - the guest itself is perfectly
> valid, it just can't be run :)

Given that we increasingly fill in alot of information in the XML at define
time, we already have a general expectation that the QEMU binary will  be
present at define time. I think this is not unreasonable - we suggest apps
call virConnectGetCapabilities and/or virDomainGetCapabilities to understand
what is installed/available before creating an XML document to define. Those
APIs of course require binaries to be installed too.   So I don't think we
should really put effort into coping with defining XML for a time when the
QEMU binaries aren't installed.  Such a scenario is so unlikely to be hit
that any code trying to cope with that is going to be largely untested and
fragile, so it would be a disservice to pretend it'll be something worth
relying on.

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

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

Re: [libvirt] [PATCH v2 06/15] qemu: Defer capability check to command line generation time

[John Ferlan]

On 02/19/2018 04:29 AM, Andrea Bolognani wrote:
> On Mon, 2018-02-19 at 07:24 +0100, Peter Krempa wrote:
>> On Fri, Feb 16, 2018 at 17:28:03 +0100, Andrea Bolognani wrote:
>>> Validate time is a bit too early to check whether the required
>>> capabilities are available, since the QEMU binary might have
>>> been updated or replaced by the time we are asked to run the
>>> guest.
>>
>> So are you having problem with the fact that the definition will be
>> rejected right away and not just when you try to start it?
>>
>> Validate is re-run when starting the VM so a downgrade is handled
>> properly.
> 
> Right, but isn't checking for QEMU capabilities at validate time
> unreasonably strict? A guest which uses eg. an invalid combination
> of machine type and architecture will never become valid at a later
> point, but a guest should not be considered invalid just because
> the QEMU binary you happened to have installed at the time you
> defined it lacked some features - the guest itself is perfectly
> valid, it just can't be run :)
> 

IIRC - the decision processing I used for moving the capability checking
into Validation for controller options (PCI, SCSI, USB, SATA) was
because at least qemuDomainDefValidateMemoryHotplug already had failure
paths when capabilities weren't set. Undoing PCI would perhaps mean
quite a few more places that need adjustment to get that bald yak.

As for the question of "unreasonably strict" at Validation time - isn't
a purpose of Validation to ensure certain combinations that were defined
would work together properly? It just so happens that using certain
combinations of options with a specific version of a binary could be
that type of check. Although I do see your point. Personally I'd rather
get something "right" when defining it rather than find out later - it's
one of those - well why didn't you tell me that before type gripes.

John

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

Re: [libvirt] [PATCH python 1/1] Set hints for virPyDictToTypedParams

[Denis V. Lunev]

On 02/19/2018 01:08 PM, Edgar Kaziakhmedov wrote:
> ping ^ 2
>
>
> On 02/13/2018 11:20 AM, Edgar Kaziakhmedov wrote:
>> ping
>>
>>
>> On 02/07/2018 05:49 PM, Edgar Kaziakhmedov wrote:
>>> Predefine hints for all parameters possible to avoid wrong type
>>> convert.
>>>
>>> Signed-off-by: Edgar Kaziakhmedov 
>>> ---
>>>   libvirt-override.c | 128
>>> +++--
>>>   1 file changed, 124 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/libvirt-override.c b/libvirt-override.c
>>> index 78a7f08..dba42d4 100644
>>> --- a/libvirt-override.c
>>> +++ b/libvirt-override.c
>>> @@ -7750,7 +7750,9 @@ libvirt_virDomainMigrate3(PyObject *self
>>> ATTRIBUTE_UNUSED,
>>>   PyObject *dict;
>>>   unsigned int flags;
>>>   virTypedParameterPtr params;
>>> -    int nparams;
>>> +    virPyTypedParamsHintPtr hparams;
>>> +    int nparams = 0;
>>> +    int nhparams = 15;
>>>   virDomainPtr ddom = NULL;
>>>
>>>   if (!PyArg_ParseTuple(args, (char *) "OOOI:virDomainMigrate3",
>>> @@ -7760,14 +7762,64 @@ libvirt_virDomainMigrate3(PyObject *self
>>> ATTRIBUTE_UNUSED,
>>>   domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
>>>   dconn = (virConnectPtr) PyvirConnect_Get(pyobj_dconn);
>>>
>>> -    if (virPyDictToTypedParams(dict, , , NULL, 0) < 0)
>>> +    hparams = malloc(sizeof(virPyTypedParamsHint) * nhparams);
>>> +    hparams[0].name = VIR_MIGRATE_PARAM_URI;
>>> +    hparams[0].type = VIR_TYPED_PARAM_STRING;
>>> +
>>> +    hparams[1].name = VIR_MIGRATE_PARAM_DEST_NAME;
>>> +    hparams[1].type = VIR_TYPED_PARAM_STRING;
>>> +
>>> +    hparams[2].name = VIR_MIGRATE_PARAM_DEST_XML;
>>> +    hparams[2].type = VIR_TYPED_PARAM_STRING;
>>> +
>>> +    hparams[3].name = VIR_MIGRATE_PARAM_GRAPHICS_URI;
>>> +    hparams[3].type = VIR_TYPED_PARAM_STRING;
>>> +
>>> +    hparams[4].name = VIR_MIGRATE_PARAM_BANDWIDTH;
>>> +    hparams[4].type = VIR_TYPED_PARAM_ULLONG;
>>> +
>>> +    hparams[5].name = VIR_MIGRATE_PARAM_LISTEN_ADDRESS;
>>> +    hparams[5].type = VIR_TYPED_PARAM_STRING;
>>> +
>>> +    hparams[6].name = VIR_MIGRATE_PARAM_DISKS_PORT;
>>> +    hparams[6].type = VIR_TYPED_PARAM_INT;
>>> +
>>> +    hparams[7].name = VIR_MIGRATE_PARAM_COMPRESSION;
>>> +    hparams[7].type = VIR_TYPED_PARAM_STRING;
>>> +
>>> +    hparams[8].name = VIR_MIGRATE_PARAM_COMPRESSION_MT_DTHREADS;
>>> +    hparams[8].type = VIR_TYPED_PARAM_INT;
>>> +
>>> +    hparams[9].name = VIR_MIGRATE_PARAM_COMPRESSION_MT_LEVEL;
>>> +    hparams[9].type = VIR_TYPED_PARAM_INT;
>>> +
>>> +    hparams[10].name = VIR_MIGRATE_PARAM_COMPRESSION_MT_THREADS;
>>> +    hparams[10].type = VIR_TYPED_PARAM_INT;
>>> +
>>> +    hparams[11].name = VIR_MIGRATE_PARAM_COMPRESSION_XBZRLE_CACHE;
>>> +    hparams[11].type = VIR_TYPED_PARAM_ULLONG;
>>> +
>>> +    hparams[12].name = VIR_MIGRATE_PARAM_PERSIST_XML;
>>> +    hparams[12].type = VIR_TYPED_PARAM_STRING;
>>> +
>>> +    hparams[13].name = VIR_MIGRATE_PARAM_AUTO_CONVERGE_INITIAL;
>>> +    hparams[13].type = VIR_TYPED_PARAM_INT;
>>> +
>>> +    hparams[14].name = VIR_MIGRATE_PARAM_AUTO_CONVERGE_INCREMENT;
>>> +    hparams[14].type = VIR_TYPED_PARAM_INT;
>>> +
>>> +    if (virPyDictToTypedParams(dict, , ,
>>> +   hparams, nhparams) < 0) {
>>> +    free(hparams);
>>>   return NULL;
>>> +    }
>>>
>>>   LIBVIRT_BEGIN_ALLOW_THREADS;
>>>   ddom = virDomainMigrate3(domain, dconn, params, nparams, flags);
>>>   LIBVIRT_END_ALLOW_THREADS;
>>>
>>>   virTypedParamsFree(params, nparams);
>>> +    free(hparams);
>>>   return libvirt_virDomainPtrWrap(ddom);
>>>   }
>>>
>>> @@ -7781,7 +7833,9 @@ libvirt_virDomainMigrateToURI3(PyObject *self
>>> ATTRIBUTE_UNUSED,
>>>   PyObject *dict;
>>>   unsigned int flags;
>>>   virTypedParameterPtr params;
>>> +    virPyTypedParamsHintPtr hparams;
>>>   int nparams;
>>> +    int nhparams = 15;
>>>   int ret = -1;
>>>
>>>   if (!PyArg_ParseTuple(args, (char *) "OzOI:virDomainMigrate3",
>>> @@ -7790,14 +7844,64 @@ libvirt_virDomainMigrateToURI3(PyObject
>>> *self ATTRIBUTE_UNUSED,
>>>
>>>   domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
>>>
>>> -    if (virPyDictToTypedParams(dict, , , NULL, 0) < 0)
>>> +    hparams = malloc(sizeof(virPyTypedParamsHint) * nhparams);
>>> +    hparams[0].name = VIR_MIGRATE_PARAM_URI;
>>> +    hparams[0].type = VIR_TYPED_PARAM_STRING;
>>> +
>>> +    hparams[1].name = VIR_MIGRATE_PARAM_DEST_NAME;
>>> +    hparams[1].type = VIR_TYPED_PARAM_STRING;
>>> +
>>> +    hparams[2].name = VIR_MIGRATE_PARAM_DEST_XML;
>>> +    hparams[2].type = VIR_TYPED_PARAM_STRING;
>>> +
>>> +    hparams[3].name = VIR_MIGRATE_PARAM_GRAPHICS_URI;
>>> +    hparams[3].type = VIR_TYPED_PARAM_STRING;
>>> +
>>> +    hparams[4].name = VIR_MIGRATE_PARAM_BANDWIDTH;
>>> +    hparams[4].type = VIR_TYPED_PARAM_ULLONG;
>>> +
>>> +    hparams[5].name = VIR_MIGRATE_PARAM_LISTEN_ADDRESS;
>>> +    hparams[5].type = 

Re: [libvirt] [RFC] libvirt hexagon sticker design

[Martin Kletzander]

On Mon, Feb 19, 2018 at 12:41:32PM +0100, Andrea Bolognani wrote:

On Mon, 2018-02-19 at 10:31 +, Daniel P. Berrangé wrote:

> [1] https://drive.google.com/open?id=1xhn0GLvCKMcEOUnqn6boghaUOgjA8sOG
> [2] https://drive.google.com/open?id=1b2MUyv0eo723F2rorWnmv1sTeCycUCN4

I think the second style works better, as having the 'VIRTUALIZATION API'
line there pushes the main logo off to the left hand side, making the
image unbalanced.


Agreed on the second style.



Definitely.


In the original stickers we had, which were round, we have the website
URL and "VIRTUALIZATION API" done above & below the image

https://www.berrange.com/~dan/libvirt-sticker.jpg

Perhaps we could try and do the same layout but with the hexagon, as
that would get rid of the big white space above & below the main
graphic.


I think trying to cram too much information inside a small
exhagon is not going to look great. If you look at most other
stickers, they don't include things such as the URL either.



Exactly.  And libvirt is not a word of many meanings.  Google "libvirt" and
there is no confusion what the sticker is supposed to represent.


In terms of background & border, IMHO, it really should be the white
background - the logo doesn't stand out well when you have it on the
green background. Green background is ok for the wbsite banner as we
don't want the logo to be too distrating, but for a promotional sticker
having a prominent bold style is desirable.


Agreed. The lighter green background also mostly works, but only
with the grey text variation of the logo.



I'd vote from second one from the top row.  Did you try making a circular (or
hexagonal) gradien tthat would start as white in the center and continued to the
outside where it would end up as a border of the same colour as the one I voted
for?  Might be very ugly, but also pretty nice, hard to say without any trying 
=)


--
Andrea Bolognani / Red Hat / Virtualization

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


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

[libvirt] [PATCH] tests: drop linkage to libvirt_driver_network_impl.la

[Daniel P . Berrangé]

The qemuxml2argvtest does not need to link to the network driver
after this commit:

  commit 0c63c117a2d17f66b05dd83e50aa36ac0b0c9843
  Author: Daniel P. Berrangé 
  Date:   Fri Feb 9 15:08:53 2018 +

conf: reimplement virDomainNetResolveActualType in terms of public API

Signed-off-by: Daniel P. Berrangé 
---

Pushed as a CI build fix for OS-X

 tests/Makefile.am | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tests/Makefile.am b/tests/Makefile.am
index d9b3a99477..09647a959d 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -583,7 +583,6 @@ qemuxml2argvtest_SOURCES = \
qemuxml2argvtest.c testutilsqemu.c testutilsqemu.h \
testutils.c testutils.h
 qemuxml2argvtest_LDADD = libqemutestdriver.la \
-   ../src/libvirt_driver_network_impl.la \
$(LDADDS) $(LIBXML_LIBS)
 
 qemuxml2argvmock_la_SOURCES = \
-- 
2.14.3

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

Re: [libvirt] [PATCH v2 08/15] qemu: Validate PCI controllers (index)

[Andrea Bolognani]

On Mon, 2018-02-19 at 11:52 +, Daniel P. Berrangé wrote:
> > +qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef 
> > *controller,
> > + const virDomainDef *def)
> >  {
> > +const virDomainPCIControllerOpts *pciopts = >opts.pciopts;
> > +const char *model = 
> > virDomainControllerModelPCITypeToString(controller->model);
> > +const char *modelName = 
> > virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
> > +
> > +if (!model) {
> > +virReportError(VIR_ERR_INTERNAL_ERROR,
> > +   _("Unknown virDomainControllerModelPCI value: %d"),
> 
> Including type names in error messages is not too nice as a user facing
> error - better to use plain english  "Unexpected PCI controller model")

But these are marked as internal errors, eg. They Should Never
Happen™, and if they ever do it's useful for developers to have
more detailed information. Users can't really do anything but
report the issue, after all.

> > +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> > +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
> > +/* pci-root controllers for pSeries guests can have any index, but
> > + * all other pci-root and pcie-root controller must have index 0 */
> > +if (controller->idx != 0 &&
> > +!(qemuDomainIsPSeries(def) &&
> > +  controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT)) {
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +   _("Index for '%s' controller must be 0"),
> > +   model);
> > +return -1;
> > +}
> > +break;
> > +
> > +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
> 
> Any time there is a _LAST element we should have an error report too
> 
>virReportError(VIR_ERR_INTERNAL_ERROR,
>   _("Unexpected controller model %d"), controller->model);
>return -1;
> 
> It should also have an 'default:' next to the LAST. Both are things that
> should never occur unless we've screwed up code elsewherein libvirt, but
> we want to be robust about catching such screw ups. This is particularly
> important for controller models, as we have many different controller
> type specific enums all stored in the same struct field

Okay, I'll address that, but the point above about what error
message to show in such scenarios still applies IMHO.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [RFC] libvirt hexagon sticker design

[Daniel P . Berrangé]

On Mon, Feb 19, 2018 at 12:54:24PM +0100, Erik Skultety wrote:
> On Mon, Feb 19, 2018 at 11:09:40AM +, Daniel P. Berrangé wrote:
> > On Mon, Feb 19, 2018 at 12:02:01PM +0100, Erik Skultety wrote:
> > > [...]
> > >
> > > > > In terms of background & border, IMHO, it really should be the white
> > > > > background - the logo doesn't stand out well when you have it on the
> > > > > green background. Green background is ok for the wbsite banner as we
> > > > > don't want the logo to be too distrating, but for a promotional 
> > > > > sticker
> > > > > having a prominent bold style is desirable.
> > > > >
> > > > > I'm undecided whether having a green border or not is neccessary. If
> > > > > we have the border, the logo has to be slightly smaller, to allow
> > > > > it space it breathe within the border. Do you have any reference of
> > > > > what other proojects have done wrt borders ?
> > > >
> > > > To answer my own question:
> > > >
> > > >   http://hexb.in/
> > > >
> > > > There's no real majority either with or without borders.
> >
> > Actually, out of those which have a white background, the majority do have
> > a saturated colour border.
> >
> > > In general, most probably no, but vast majority of the ones I have put 
> > > around
> > > my working space follow a simple pattern - the border is just a few shades
> > > darker than the artwork area.
> >
> > That works well for stickers with a coloured background. For those with a
> > white background, it feels like a saturated colour gives a good contrast.
> >
> > I'd be inclined to prototype, the two top-left logos from this image:
> >
> >   https://drive.google.com/file/d/1b2MUyv0eo723F2rorWnmv1sTeCycUCN4/view
> >
> > perhaps, variants with & without the  website URL (though the majority
> > on hexb.in omit the website URL and keep text to a minimum)
> >
> > Do a clone of the hexb.in repo, and add our logo and see how nicely it
> > works when surrounded on all sides by other stickers.
> 
> I'm not sure I understand your message, i.e. whether you'd like to prototype
> on your own and you're missing some sources in which case I think you should 
> be
> able to download the SVG and extract everything necessary from it with 
> whatever
> editor you prefer or you'd like me to make another variants with the URL
> website and test it with the hexb.in repo.

I don't mind either way - I can prototype myself, or let you carry on with it
since you started ..

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

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

Re: [libvirt] [RFC] libvirt hexagon sticker design

[Erik Skultety]

On Mon, Feb 19, 2018 at 11:09:40AM +, Daniel P. Berrangé wrote:
> On Mon, Feb 19, 2018 at 12:02:01PM +0100, Erik Skultety wrote:
> > [...]
> >
> > > > In terms of background & border, IMHO, it really should be the white
> > > > background - the logo doesn't stand out well when you have it on the
> > > > green background. Green background is ok for the wbsite banner as we
> > > > don't want the logo to be too distrating, but for a promotional sticker
> > > > having a prominent bold style is desirable.
> > > >
> > > > I'm undecided whether having a green border or not is neccessary. If
> > > > we have the border, the logo has to be slightly smaller, to allow
> > > > it space it breathe within the border. Do you have any reference of
> > > > what other proojects have done wrt borders ?
> > >
> > > To answer my own question:
> > >
> > >   http://hexb.in/
> > >
> > > There's no real majority either with or without borders.
>
> Actually, out of those which have a white background, the majority do have
> a saturated colour border.
>
> > In general, most probably no, but vast majority of the ones I have put 
> > around
> > my working space follow a simple pattern - the border is just a few shades
> > darker than the artwork area.
>
> That works well for stickers with a coloured background. For those with a
> white background, it feels like a saturated colour gives a good contrast.
>
> I'd be inclined to prototype, the two top-left logos from this image:
>
>   https://drive.google.com/file/d/1b2MUyv0eo723F2rorWnmv1sTeCycUCN4/view
>
> perhaps, variants with & without the  website URL (though the majority
> on hexb.in omit the website URL and keep text to a minimum)
>
> Do a clone of the hexb.in repo, and add our logo and see how nicely it
> works when surrounded on all sides by other stickers.

I'm not sure I understand your message, i.e. whether you'd like to prototype
on your own and you're missing some sources in which case I think you should be
able to download the SVG and extract everything necessary from it with whatever
editor you prefer or you'd like me to make another variants with the URL
website and test it with the hexb.in repo.

I'm sorry I just got a bit confused.

Erik

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

Re: [libvirt] [PATCH v2 08/15] qemu: Validate PCI controllers (index)

[Daniel P . Berrangé]

On Fri, Feb 16, 2018 at 05:28:05PM +0100, Andrea Bolognani wrote:
> Signed-off-by: Andrea Bolognani 
> ---
>  src/qemu/qemu_domain.c | 56 
> --
>  1 file changed, 54 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 9dc3d5597..e4e67c585 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4243,9 +4243,61 @@ qemuDomainDeviceDefValidateControllerSCSI(const 
> virDomainControllerDef *controll
>  
>  
>  static int
> -qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef 
> *controller ATTRIBUTE_UNUSED,
> - const virDomainDef *def 
> ATTRIBUTE_UNUSED)
> +qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef 
> *controller,
> + const virDomainDef *def)
>  {
> +const virDomainPCIControllerOpts *pciopts = >opts.pciopts;
> +const char *model = 
> virDomainControllerModelPCITypeToString(controller->model);
> +const char *modelName = 
> virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
> +
> +if (!model) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Unknown virDomainControllerModelPCI value: %d"),

Including type names in error messages is not too nice as a user facing
error - better to use plain english  "Unexpected PCI controller model")

> +   controller->model);
> +return -1;
> +}
> +if (!modelName) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Unknown virDomainControllerPCIModelName value: 
> %d"),
> +   pciopts->modelName);

Likewise here.

> +return -1;
> +}
> +
> +/* index */
> +switch ((virDomainControllerModelPCI) controller->model) {
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
> +case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
> +if (controller->idx == 0) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("Index for '%s' controller must be > 0"),
> +   model);
> +return -1;
> +}
> +break;
> +
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
> +/* pci-root controllers for pSeries guests can have any index, but
> + * all other pci-root and pcie-root controller must have index 0 */
> +if (controller->idx != 0 &&
> +!(qemuDomainIsPSeries(def) &&
> +  controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("Index for '%s' controller must be 0"),
> +   model);
> +return -1;
> +}
> +break;
> +
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:

Any time there is a _LAST element we should have an error report too

   virReportError(VIR_ERR_INTERNAL_ERROR,
  _("Unexpected controller model %d"), controller->model);
   return -1; 

It should also have an 'default:' next to the LAST. Both are things that
should never occur unless we've screwed up code elsewherein libvirt, but
we want to be robust about catching such screw ups. This is particularly
important for controller models, as we have many different controller
type specific enums all stored in the same struct field

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

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

Re: [libvirt] [PATCH v2 07/15] qemu: Clear out qemuDomainDeviceDefValidateControllerPCI()

[Andrea Bolognani]

On Mon, 2018-02-19 at 10:40 +0100, Peter Krempa wrote:
> > Renaming the function won't work because then the compiler will
> > complain about it being unused. Unless you meant something like
> > 
> >   /* Delete once done */
> >   ValidateControllerPCIOld() {
> > /* Existing checks here */
> >   }
> > 
> >   ValidateControllerPCI() {
> > /* New checks here */
> > 
> > /* Delete once done */
> > ValidateControllerPCIOld();
> >   }
> > 
> > which could actually do the trick.
> 
> I meant this flow obviously, so that the checks are kept until
> fixed/reimplemented.

It was not obvious to me when reading the first message :)

But I agree it's a better way to handle the situation, so I will
do it this way in v3.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [RFC] libvirt hexagon sticker design

[Andrea Bolognani]

On Mon, 2018-02-19 at 10:31 +, Daniel P. Berrangé wrote:
> > [1] https://drive.google.com/open?id=1xhn0GLvCKMcEOUnqn6boghaUOgjA8sOG
> > [2] https://drive.google.com/open?id=1b2MUyv0eo723F2rorWnmv1sTeCycUCN4
> 
> I think the second style works better, as having the 'VIRTUALIZATION API'
> line there pushes the main logo off to the left hand side, making the
> image unbalanced.

Agreed on the second style.

> In the original stickers we had, which were round, we have the website
> URL and "VIRTUALIZATION API" done above & below the image
> 
> https://www.berrange.com/~dan/libvirt-sticker.jpg
> 
> Perhaps we could try and do the same layout but with the hexagon, as
> that would get rid of the big white space above & below the main
> graphic.

I think trying to cram too much information inside a small
exhagon is not going to look great. If you look at most other
stickers, they don't include things such as the URL either.

> In terms of background & border, IMHO, it really should be the white
> background - the logo doesn't stand out well when you have it on the
> green background. Green background is ok for the wbsite banner as we
> don't want the logo to be too distrating, but for a promotional sticker
> having a prominent bold style is desirable.

Agreed. The lighter green background also mostly works, but only
with the grey text variation of the logo.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [RFC] libvirt hexagon sticker design

[Daniel P . Berrangé]

On Mon, Feb 19, 2018 at 12:02:01PM +0100, Erik Skultety wrote:
> [...]
> 
> > > In terms of background & border, IMHO, it really should be the white
> > > background - the logo doesn't stand out well when you have it on the
> > > green background. Green background is ok for the wbsite banner as we
> > > don't want the logo to be too distrating, but for a promotional sticker
> > > having a prominent bold style is desirable.
> > >
> > > I'm undecided whether having a green border or not is neccessary. If
> > > we have the border, the logo has to be slightly smaller, to allow
> > > it space it breathe within the border. Do you have any reference of
> > > what other proojects have done wrt borders ?
> >
> > To answer my own question:
> >
> >   http://hexb.in/
> >
> > There's no real majority either with or without borders.

Actually, out of those which have a white background, the majority do have
a saturated colour border.

> In general, most probably no, but vast majority of the ones I have put around
> my working space follow a simple pattern - the border is just a few shades
> darker than the artwork area.

That works well for stickers with a coloured background. For those with a
white background, it feels like a saturated colour gives a good contrast.

I'd be inclined to prototype, the two top-left logos from this image:

  https://drive.google.com/file/d/1b2MUyv0eo723F2rorWnmv1sTeCycUCN4/view

perhaps, variants with & without the  website URL (though the majority
on hexb.in omit the website URL and keep text to a minimum)

Do a clone of the hexb.in repo, and add our logo and see how nicely it
works when surrounded on all sides by other stickers.

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

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

Re: [libvirt] [RFC] libvirt hexagon sticker design

[Erik Skultety]

[...]

> > In terms of background & border, IMHO, it really should be the white
> > background - the logo doesn't stand out well when you have it on the
> > green background. Green background is ok for the wbsite banner as we
> > don't want the logo to be too distrating, but for a promotional sticker
> > having a prominent bold style is desirable.
> >
> > I'm undecided whether having a green border or not is neccessary. If
> > we have the border, the logo has to be slightly smaller, to allow
> > it space it breathe within the border. Do you have any reference of
> > what other proojects have done wrt borders ?
>
> To answer my own question:
>
>   http://hexb.in/
>
> There's no real majority either with or without borders.

In general, most probably no, but vast majority of the ones I have put around
my working space follow a simple pattern - the border is just a few shades
darker than the artwork area.

Erik

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

Re: [libvirt] [RFC] libvirt hexagon sticker design

[Daniel P . Berrangé]

On Mon, Feb 19, 2018 at 10:31:34AM +, Daniel P. Berrangé wrote:
> On Mon, Feb 19, 2018 at 09:44:36AM +0100, Erik Skultety wrote:
> > Hi folks,
> > those who attended at least one conference for the past year have probably
> > noticed the rising trend (more like "sticker hype") of FOSS projects giving 
> > away
> > these hexagon stickers, it's very inexpensive way of making some promo for
> > their project and since we don't do many promos (AFAIK none to be precise) I
> > guess as a project that's been going strong for 12 years already we should
> > probably start somewhere, even baby steps count (as it turns out in this 
> > case -
> 
> I've got a pile of a 100+ stickers for libvirt still that I've given away
> at many confs :-)
> 
> > - literally...). So, I've taken our libvirt-publican repo and came up with a
> > few various color combinations for libvirt hexagon sticker design. Below you
> > can find links to my personal google drive (these are hexagon meshes, I can,
> > or anyone can for that matter, isolate individual designs and send them as
> > separate patches on demand), since each of the SVGs is over 1.5MB and I'd 
> > easily
> > run into message size limits for the mailing list, had I gone with sending 
> > these
> > as patches against libvirt-publican.
> 
> BTW I think the graphics in the libvirt-publican repo are the original
> logo image. I created brand new SVG based art when i redid the website
> last time. Aside from creating SVG format images, I fixed the totally
> messed up depth perspective that the original had in the inner penguins.
> The master files are all in docs/logos/  in the main GIT repo.
> 
> 
> > [1] https://drive.google.com/open?id=1xhn0GLvCKMcEOUnqn6boghaUOgjA8sOG
> > [2] https://drive.google.com/open?id=1b2MUyv0eo723F2rorWnmv1sTeCycUCN4
> 
> I think the second style works better, as having the 'VIRTUALIZATION API'
> line there pushes the main logo off to the left hand side, making the
> image unbalanced.
> 
> In the original stickers we had, which were round, we have the website
> URL and "VIRTUALIZATION API" done above & below the image
> 
> https://www.berrange.com/~dan/libvirt-sticker.jpg
> 
> Perhaps we could try and do the same layout but with the hexagon, as
> that would get rid of the big white space above & below the main
> graphic.
> 
> In terms of background & border, IMHO, it really should be the white
> background - the logo doesn't stand out well when you have it on the
> green background. Green background is ok for the wbsite banner as we
> don't want the logo to be too distrating, but for a promotional sticker
> having a prominent bold style is desirable.
> 
> I'm undecided whether having a green border or not is neccessary. If
> we have the border, the logo has to be slightly smaller, to allow
> it space it breathe within the border. Do you have any reference of
> what other proojects have done wrt borders ?

To answer my own question:

  http://hexb.in/

There's no real majority either with or without borders.

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

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

Re: [libvirt] [RFC] libvirt hexagon sticker design

[Daniel P . Berrangé]

On Mon, Feb 19, 2018 at 11:36:03AM +0100, Kashyap Chamarthy wrote:
> On Mon, Feb 19, 2018 at 09:44:36AM +0100, Erik Skultety wrote:
> > Hi folks,
> > those who attended at least one conference for the past year have probably
> > noticed the rising trend (more like "sticker hype") of FOSS projects giving 
> > away
> > these hexagon stickers, it's very inexpensive way of making some promo for
> > their project and since we don't do many promos (AFAIK none to be precise) I
> > guess as a project that's been going strong for 12 years already we should
> > probably start somewhere, even baby steps count (as it turns out in this 
> > case -
> > - literally...). So, I've taken our libvirt-publican repo and came up with a
> > few various color combinations for libvirt hexagon sticker design. Below you
> > can find links to my personal google drive (these are hexagon meshes, I can,
> > or anyone can for that matter, isolate individual designs and send them as
> > separate patches on demand), since each of the SVGs is over 1.5MB and I'd 
> > easily
> > run into message size limits for the mailing list, had I gone with sending 
> > these
> > as patches against libvirt-publican.
> 
> Speaking of logos...at the risk of opening a huge bucket of paint: 
> 
> The current libvirt logo is bit non-intuitive.  Unless you squint at it
> to see what it might be about, you won't immediately get an idea what is
> trying to tell you.  Not sure if there's appetite to rework the logo
> itself.

I don't really want to go there - I really like our logo.

In general I don't think logos really need to explain what the project
is, largely because doing so is impossible/impractical for most technical
projects.

> Let me see if I can describe the image in words: "you're peeling off
> some layer and you see little penguins (VMs) get churned out".

You know it is basically a "sardine tin", but with penguins right ?

The tin is the computer, and the sardines (penguins) are the OS
that are crammed inside it. That's a pretty good analogy for VMs
IMHO.

> (But that's from my biased POV of already knowing what libvirt is.)
> 
> For some inspiration, take a look at how the `curl` project went about
> redesigning its logo[1].
>
> [1] https://daniel.haxx.se/blog/2016/05/27/a-new-curl-logo/

I don't think the new curl logo is any more "intuitive" in explaining
what the project is than their old logo. Unless you already know that
CURL is a library for downloading web content, the implications of the
"://" will pass right over your head. The new curl logo is certainly a
very nice improvement, but that's because the old one was really pretty
crude so anything would be better. I wouldn't say it is any more
intuitive though - just a nicer graphic design.

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

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

Re: [libvirt] [RFC] libvirt hexagon sticker design

[Kashyap Chamarthy]

On Mon, Feb 19, 2018 at 09:44:36AM +0100, Erik Skultety wrote:
> Hi folks,
> those who attended at least one conference for the past year have probably
> noticed the rising trend (more like "sticker hype") of FOSS projects giving 
> away
> these hexagon stickers, it's very inexpensive way of making some promo for
> their project and since we don't do many promos (AFAIK none to be precise) I
> guess as a project that's been going strong for 12 years already we should
> probably start somewhere, even baby steps count (as it turns out in this case 
> -
> - literally...). So, I've taken our libvirt-publican repo and came up with a
> few various color combinations for libvirt hexagon sticker design. Below you
> can find links to my personal google drive (these are hexagon meshes, I can,
> or anyone can for that matter, isolate individual designs and send them as
> separate patches on demand), since each of the SVGs is over 1.5MB and I'd 
> easily
> run into message size limits for the mailing list, had I gone with sending 
> these
> as patches against libvirt-publican.

Speaking of logos...at the risk of opening a huge bucket of paint: 

The current libvirt logo is bit non-intuitive.  Unless you squint at it
to see what it might be about, you won't immediately get an idea what is
trying to tell you.  Not sure if there's appetite to rework the logo
itself.

Let me see if I can describe the image in words: "you're peeling off
some layer and you see little penguins (VMs) get churned out".

(But that's from my biased POV of already knowing what libvirt is.)

---

For some inspiration, take a look at how the `curl` project went about
redesigning its logo[1].


[1] https://daniel.haxx.se/blog/2016/05/27/a-new-curl-logo/

[...]

-- 
/kashyap

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

Re: [libvirt] [RFC] libvirt hexagon sticker design

[Daniel P . Berrangé]

On Mon, Feb 19, 2018 at 09:44:36AM +0100, Erik Skultety wrote:
> Hi folks,
> those who attended at least one conference for the past year have probably
> noticed the rising trend (more like "sticker hype") of FOSS projects giving 
> away
> these hexagon stickers, it's very inexpensive way of making some promo for
> their project and since we don't do many promos (AFAIK none to be precise) I
> guess as a project that's been going strong for 12 years already we should
> probably start somewhere, even baby steps count (as it turns out in this case 
> -

I've got a pile of a 100+ stickers for libvirt still that I've given away
at many confs :-)

> - literally...). So, I've taken our libvirt-publican repo and came up with a
> few various color combinations for libvirt hexagon sticker design. Below you
> can find links to my personal google drive (these are hexagon meshes, I can,
> or anyone can for that matter, isolate individual designs and send them as
> separate patches on demand), since each of the SVGs is over 1.5MB and I'd 
> easily
> run into message size limits for the mailing list, had I gone with sending 
> these
> as patches against libvirt-publican.

BTW I think the graphics in the libvirt-publican repo are the original
logo image. I created brand new SVG based art when i redid the website
last time. Aside from creating SVG format images, I fixed the totally
messed up depth perspective that the original had in the inner penguins.
The master files are all in docs/logos/  in the main GIT repo.


> [1] https://drive.google.com/open?id=1xhn0GLvCKMcEOUnqn6boghaUOgjA8sOG
> [2] https://drive.google.com/open?id=1b2MUyv0eo723F2rorWnmv1sTeCycUCN4

I think the second style works better, as having the 'VIRTUALIZATION API'
line there pushes the main logo off to the left hand side, making the
image unbalanced.

In the original stickers we had, which were round, we have the website
URL and "VIRTUALIZATION API" done above & below the image

https://www.berrange.com/~dan/libvirt-sticker.jpg

Perhaps we could try and do the same layout but with the hexagon, as
that would get rid of the big white space above & below the main
graphic.

In terms of background & border, IMHO, it really should be the white
background - the logo doesn't stand out well when you have it on the
green background. Green background is ok for the wbsite banner as we
don't want the logo to be too distrating, but for a promotional sticker
having a prominent bold style is desirable.

I'm undecided whether having a green border or not is neccessary. If
we have the border, the logo has to be slightly smaller, to allow
it space it breathe within the border. Do you have any reference of
what other proojects have done wrt borders ?

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

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

Re: [libvirt] [PATCH] conf: move 'generated' member from virMacAddr to virDomainNetDef

[Daniel P . Berrangé]

On Mon, Feb 19, 2018 at 07:23:23AM +0100, Michal Privoznik wrote:
> On 02/16/2018 09:04 PM, Laine Stump wrote:
> > Commit 7e62c4cd26d (first appearing in libvirt-3.9.0 as a resolution
> > to rhbz #1343919) added a "generated" attribute to virMacAddr that was
> > set whenever a mac address was auto-generated by libvirt. This
> > knowledge was used in a single place - when trying to match a NetDef
> > from the domain to delete with user-provided XML. Since the XML parser
> > always auto-generates a MAC address for NetDefs when none is provided,
> > it was previously impossible to make a search where the MAC address
> > wasn't significant, but the addition of the "generated" attribute made
> > it possible for the search function to ignore auto-generated MACs.
> > 
> > This implementation had a problem though - it was adding a field to a
> > "low level" struct - virMacAddr - which is used in other places with
> > the assumption that it contains exactly a 6 byte MAC address and
> > nothing else. In particular, virNWFilterSnoopEthHdr uses virMacAddr as
> > part of the definition of an ethernet packet header, whose layout must
> > of course match an actual ethernet packet. Adding the extra bools into
> > virNWFilterSnoopEthHdr caused the nwfilter driver's "IP discovery via
> > DHCP packet snooping" functionality to mysteriously stop working.
> > 
> > In order to fix that behavior, and prevent potential future similar
> > odd behavior, this patch moves the "generated" member out of
> > virMacAddr (so that it is again really just a MAC address) and into
> > virDomainNetDef, and sets it only when virDomainNetGenerateMAC() is
> > called from virDomainNetDefParseXML() (which is the only time we care
> > about it).
> > 
> > Resolves: https://bugzilla.redhat.com/1529338
> > 
> > (It should also be applied to any maintenance branch that applies
> > commit 7e62c4cd26 and friends to resolve
> > https://bugzilla.redhat.com/1343919)
> > 
> > Signed-off-by: Laine Stump 
> > ---
> >  src/conf/domain_conf.c| 3 ++-
> >  src/conf/domain_conf.h| 1 +
> >  src/util/virmacaddr.c | 5 -
> >  src/util/virmacaddr.h | 2 --
> >  tests/bhyveargv2xmlmock.c | 1 -
> >  5 files changed, 3 insertions(+), 9 deletions(-)
> 
> What I am missing here is a comment to _virMacAddr struct saying that
> the structure cannot change because of NWFilter code.

I might be nice to put a compile time assert in nwfilter code

   assert(sizef(struct foobar) == 42);

to validate it matches the ethernet packet size.

> ACK with that changed.

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

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

Re: [libvirt] [PATCH v3 08/10] qemu: remove virConnectPtr in some migration methods

[Daniel P . Berrangé]

On Fri, Feb 16, 2018 at 05:02:34PM -0500, John Ferlan wrote:
> 
> 
> On 02/16/2018 06:22 AM, Daniel P. Berrangé wrote:
> > The qemuMigrationPrecreateStorage method needs a connection
> > to access the storage driver. Instead of passing it around,
> > open it at time of use.
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >  src/qemu/qemu_driver.c| 16 +++
> >  src/qemu/qemu_migration.c | 51 
> > ++-
> >  src/qemu/qemu_migration.h |  4 +---
> >  3 files changed, 33 insertions(+), 38 deletions(-)
> > 
> 
> [...]
> 
> >  
> > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > index 312d717617..6edadac719 100644
> > --- a/src/qemu/qemu_migration.c
> > +++ b/src/qemu/qemu_migration.c
> 
> [...]
> 
> >  static int doPeer2PeerMigrate2(virQEMUDriverPtr driver,
> > -   virConnectPtr sconn ATTRIBUTE_UNUSED,
> > +   virConnectPtr sconn,
> 
> I know this has an impact in later patches too, but @sconn is only used
> for (duplicated) debug prints...
> 
> > virConnectPtr dconn,
> > virDomainObjPtr vm,
> > const char *dconnuri,
> > @@ -4654,7 +4651,7 @@ doPeer2PeerMigrate3(virQEMUDriverPtr driver,
> 
> Same here it seems.  Removing affects the calling stack a bit, but I
> think would be the right thing to do.

I did look at removing them, however, I feel they are useful debug
statements - understanding migration is already very difficult so
I think it is worth it.


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

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

Re: [libvirt] [PATCH python 1/1] Set hints for virPyDictToTypedParams

[Edgar Kaziakhmedov]

ping ^ 2


On 02/13/2018 11:20 AM, Edgar Kaziakhmedov wrote:

ping


On 02/07/2018 05:49 PM, Edgar Kaziakhmedov wrote:

Predefine hints for all parameters possible to avoid wrong type
convert.

Signed-off-by: Edgar Kaziakhmedov 
---
  libvirt-override.c | 128 
+++--

  1 file changed, 124 insertions(+), 4 deletions(-)

diff --git a/libvirt-override.c b/libvirt-override.c
index 78a7f08..dba42d4 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -7750,7 +7750,9 @@ libvirt_virDomainMigrate3(PyObject *self 
ATTRIBUTE_UNUSED,

  PyObject *dict;
  unsigned int flags;
  virTypedParameterPtr params;
-    int nparams;
+    virPyTypedParamsHintPtr hparams;
+    int nparams = 0;
+    int nhparams = 15;
  virDomainPtr ddom = NULL;

  if (!PyArg_ParseTuple(args, (char *) "OOOI:virDomainMigrate3",
@@ -7760,14 +7762,64 @@ libvirt_virDomainMigrate3(PyObject *self 
ATTRIBUTE_UNUSED,

  domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
  dconn = (virConnectPtr) PyvirConnect_Get(pyobj_dconn);

-    if (virPyDictToTypedParams(dict, , , NULL, 0) < 0)
+    hparams = malloc(sizeof(virPyTypedParamsHint) * nhparams);
+    hparams[0].name = VIR_MIGRATE_PARAM_URI;
+    hparams[0].type = VIR_TYPED_PARAM_STRING;
+
+    hparams[1].name = VIR_MIGRATE_PARAM_DEST_NAME;
+    hparams[1].type = VIR_TYPED_PARAM_STRING;
+
+    hparams[2].name = VIR_MIGRATE_PARAM_DEST_XML;
+    hparams[2].type = VIR_TYPED_PARAM_STRING;
+
+    hparams[3].name = VIR_MIGRATE_PARAM_GRAPHICS_URI;
+    hparams[3].type = VIR_TYPED_PARAM_STRING;
+
+    hparams[4].name = VIR_MIGRATE_PARAM_BANDWIDTH;
+    hparams[4].type = VIR_TYPED_PARAM_ULLONG;
+
+    hparams[5].name = VIR_MIGRATE_PARAM_LISTEN_ADDRESS;
+    hparams[5].type = VIR_TYPED_PARAM_STRING;
+
+    hparams[6].name = VIR_MIGRATE_PARAM_DISKS_PORT;
+    hparams[6].type = VIR_TYPED_PARAM_INT;
+
+    hparams[7].name = VIR_MIGRATE_PARAM_COMPRESSION;
+    hparams[7].type = VIR_TYPED_PARAM_STRING;
+
+    hparams[8].name = VIR_MIGRATE_PARAM_COMPRESSION_MT_DTHREADS;
+    hparams[8].type = VIR_TYPED_PARAM_INT;
+
+    hparams[9].name = VIR_MIGRATE_PARAM_COMPRESSION_MT_LEVEL;
+    hparams[9].type = VIR_TYPED_PARAM_INT;
+
+    hparams[10].name = VIR_MIGRATE_PARAM_COMPRESSION_MT_THREADS;
+    hparams[10].type = VIR_TYPED_PARAM_INT;
+
+    hparams[11].name = VIR_MIGRATE_PARAM_COMPRESSION_XBZRLE_CACHE;
+    hparams[11].type = VIR_TYPED_PARAM_ULLONG;
+
+    hparams[12].name = VIR_MIGRATE_PARAM_PERSIST_XML;
+    hparams[12].type = VIR_TYPED_PARAM_STRING;
+
+    hparams[13].name = VIR_MIGRATE_PARAM_AUTO_CONVERGE_INITIAL;
+    hparams[13].type = VIR_TYPED_PARAM_INT;
+
+    hparams[14].name = VIR_MIGRATE_PARAM_AUTO_CONVERGE_INCREMENT;
+    hparams[14].type = VIR_TYPED_PARAM_INT;
+
+    if (virPyDictToTypedParams(dict, , ,
+   hparams, nhparams) < 0) {
+    free(hparams);
  return NULL;
+    }

  LIBVIRT_BEGIN_ALLOW_THREADS;
  ddom = virDomainMigrate3(domain, dconn, params, nparams, flags);
  LIBVIRT_END_ALLOW_THREADS;

  virTypedParamsFree(params, nparams);
+    free(hparams);
  return libvirt_virDomainPtrWrap(ddom);
  }

@@ -7781,7 +7833,9 @@ libvirt_virDomainMigrateToURI3(PyObject *self 
ATTRIBUTE_UNUSED,

  PyObject *dict;
  unsigned int flags;
  virTypedParameterPtr params;
+    virPyTypedParamsHintPtr hparams;
  int nparams;
+    int nhparams = 15;
  int ret = -1;

  if (!PyArg_ParseTuple(args, (char *) "OzOI:virDomainMigrate3",
@@ -7790,14 +7844,64 @@ libvirt_virDomainMigrateToURI3(PyObject *self 
ATTRIBUTE_UNUSED,


  domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);

-    if (virPyDictToTypedParams(dict, , , NULL, 0) < 0)
+    hparams = malloc(sizeof(virPyTypedParamsHint) * nhparams);
+    hparams[0].name = VIR_MIGRATE_PARAM_URI;
+    hparams[0].type = VIR_TYPED_PARAM_STRING;
+
+    hparams[1].name = VIR_MIGRATE_PARAM_DEST_NAME;
+    hparams[1].type = VIR_TYPED_PARAM_STRING;
+
+    hparams[2].name = VIR_MIGRATE_PARAM_DEST_XML;
+    hparams[2].type = VIR_TYPED_PARAM_STRING;
+
+    hparams[3].name = VIR_MIGRATE_PARAM_GRAPHICS_URI;
+    hparams[3].type = VIR_TYPED_PARAM_STRING;
+
+    hparams[4].name = VIR_MIGRATE_PARAM_BANDWIDTH;
+    hparams[4].type = VIR_TYPED_PARAM_ULLONG;
+
+    hparams[5].name = VIR_MIGRATE_PARAM_LISTEN_ADDRESS;
+    hparams[5].type = VIR_TYPED_PARAM_STRING;
+
+    hparams[6].name = VIR_MIGRATE_PARAM_DISKS_PORT;
+    hparams[6].type = VIR_TYPED_PARAM_INT;
+
+    hparams[7].name = VIR_MIGRATE_PARAM_COMPRESSION;
+    hparams[7].type = VIR_TYPED_PARAM_STRING;
+
+    hparams[8].name = VIR_MIGRATE_PARAM_COMPRESSION_MT_DTHREADS;
+    hparams[8].type = VIR_TYPED_PARAM_INT;
+
+    hparams[9].name = VIR_MIGRATE_PARAM_COMPRESSION_MT_LEVEL;
+    hparams[9].type = VIR_TYPED_PARAM_INT;
+
+    hparams[10].name = VIR_MIGRATE_PARAM_COMPRESSION_MT_THREADS;
+    hparams[10].type = VIR_TYPED_PARAM_INT;
+
+    

Re: [libvirt] Regarding libvirt patchset "Keep original security label"

[Michal Privoznik]

[Adding libvirt-list as others might chime in or when somebody is
solving similar issue they can find answers in the archive.]

On 02/19/2018 10:09 AM, Toni Peltonen wrote:
> Hi,
> 
> Sorry to bother you with ages old stuff like this 
> (https://www.redhat.com/archives/libvir-list/2014-September/msg00551.html) 
> patch set.
> 
> I did some proof of concept work on my laptop during the weekend with a 
> couple of CentOS 7 virtual machines running shared GFS2 mount (with working 
> dlm locking) and libvirtd + QEMU guests on top of that. I am running the 
> latest libvirtd packages from CentOS upstream.
> 
> While I managed to get dlm and global POSIX locking working as expected when 
> I added virtlockd to the picture to actually hold locks for the VM images I 
> ended up getting some gray hair realizing that this ages old security label 
> issue is still present. The situation is basically still the same:
> 
> - On a shared filesystem (like GFS2), despite virtlockd locks working as 
> expected, libvirtd still (as expected with current code) tries to change the 
> ownership and security labels of the target file (QCOW2 image)
> 
> - This sudden change causes the virtual machine running on the other host to 
> drop as read-only since SELinux starts preventing all write operations for it
> 
> - With SELinux in Permissive mode on the host that runs the virtual machine 
> everything work as expected

So you have a disk that is shared between two domains? One way to avoid
the problem is to have  in disk definition so that libvirt
doesn't restore disk labels. However, that still might not work for you
because it means that libvirt still changes labels on domain start.

So far the only way to prevent this from happening is using custom
labels https://libvirt.org/formatdomain.html#elementsDisks .

> 
> 
> I saw that the only code available (that I could easily find and understand) 
> to really tackle this issue was your patch set from as early as 2014. It 
> seems it never hit upstream though, at least I can't find any of the relevant 
> parts in the upstream code.
> 
> I am just reaching out to ask whether this patch set was left out 
> intentionally 
> (https://www.redhat.com/archives/libvir-list/2014-September/msg00551.html), 
> just lost in time or if you are aware of any other work that might be in 
> progress to tackle this issue in the upstream?

Unfortunately, the patches were abandoned and the issue is even worse. I
remember having a discussion on this topic lately (although can't recall
where). Turns out, we firstly relabel the disks and only after that we
try to obtain disk locks. So if the latter fails (e.g. because another
domain holds the locks) the lables are changed anyway. The suggested
solution was to have two locks: one for disk contents the other for disk
metadata (like security labels).

Anyway, I don't think there's anybody working on this actively. Sorry.

Michal

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

Re: [libvirt] [PATCH 0/2] Two blockjob fixes

[Ján Tomko]

On Mon, Feb 19, 2018 at 09:40:39AM +0100, Peter Krempa wrote:

Peter Krempa (2):
 virsh: Fix internal naming of some blockjob commands
 qemu: blockcopy: Add check for bandwidth

src/qemu/qemu_driver.c |  8 +++
tools/virsh-domain.c   | 60 +-
2 files changed, 38 insertions(+), 30 deletions(-)



ACK series

Jan


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

Re: [libvirt] [PATCH v2 07/15] qemu: Clear out qemuDomainDeviceDefValidateControllerPCI()

[Peter Krempa]

On Mon, Feb 19, 2018 at 10:22:29 +0100, Andrea Bolognani wrote:
> On Mon, 2018-02-19 at 07:28 +0100, Peter Krempa wrote:
> > On Fri, Feb 16, 2018 at 17:28:04 +0100, Andrea Bolognani wrote:
> > > We will rewrite pretty much every single line of this function
> > > over the course of the next several commits, and starting from
> > > a clean slate rather than replacing it bit by bit makes the
> > > resulting diffs unmeasurably easier to read and understand,
> > > and you need fewer of them to boot. Trust me, I tried the other
> > > approach first :)
> > 
> > Will this remove any checks during the series? If yes, then you probably
> > should at first rename this function and add a almost-empty wrapper then
> > add new code to the wrapper and delete the renamed function at the end.
> 
> No, if anything it *adds* a bunch of checks :)

Well, I meant that after applying this patch a bunch of checks will
vanish until you add them in the next patches, which I don't think we
should do.

> 
> Renaming the function won't work because then the compiler will
> complain about it being unused. Unless you meant something like
> 
>   /* Delete once done */
>   ValidateControllerPCIOld() {
> /* Existing checks here */
>   }
> 
>   ValidateControllerPCI() {
> /* New checks here */
> 
> /* Delete once done */
> ValidateControllerPCIOld();
>   }
> 
> which could actually do the trick.

I meant this flow obviously, so that the checks are kept until
fixed/reimplemented.


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

Re: [libvirt] [PATCH v2 06/15] qemu: Defer capability check to command line generation time

[Peter Krempa]

On Mon, Feb 19, 2018 at 10:29:18 +0100, Andrea Bolognani wrote:
> On Mon, 2018-02-19 at 07:24 +0100, Peter Krempa wrote:
> > On Fri, Feb 16, 2018 at 17:28:03 +0100, Andrea Bolognani wrote:
> > > Validate time is a bit too early to check whether the required
> > > capabilities are available, since the QEMU binary might have
> > > been updated or replaced by the time we are asked to run the
> > > guest.
> > 
> > So are you having problem with the fact that the definition will be
> > rejected right away and not just when you try to start it?
> > 
> > Validate is re-run when starting the VM so a downgrade is handled
> > properly.
> 
> Right, but isn't checking for QEMU capabilities at validate time
> unreasonably strict?

Well, the only difference is that you are unable to 'define' such XML. I
don't think it's strict since you would not be able to run it anyways.

> A guest which uses eg. an invalid combination
> of machine type and architecture will never become valid at a later
> point, but a guest should not be considered invalid just because
> the QEMU binary you happened to have installed at the time you
> defined it lacked some features - the guest itself is perfectly
> valid, it just can't be run :)

I think the term 'guest' here is misleading. The configuration is
invalid and thus you can't really create a guest out of it.

This is actually a great example, as you actually are not able to define
a XML which would fail to be run anyways.

The specific reason for the validation callback is to not make the
definition vanish in such case.

In general, I think that the command line generator is the wrong place
for such checks since they are scattered around and you can't really
figure out whether you can start such config until you've done a lot of
preparation steps.


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

Re: [libvirt] [PATCH v2 06/15] qemu: Defer capability check to command line generation time

[Andrea Bolognani]

On Mon, 2018-02-19 at 07:24 +0100, Peter Krempa wrote:
> On Fri, Feb 16, 2018 at 17:28:03 +0100, Andrea Bolognani wrote:
> > Validate time is a bit too early to check whether the required
> > capabilities are available, since the QEMU binary might have
> > been updated or replaced by the time we are asked to run the
> > guest.
> 
> So are you having problem with the fact that the definition will be
> rejected right away and not just when you try to start it?
> 
> Validate is re-run when starting the VM so a downgrade is handled
> properly.

Right, but isn't checking for QEMU capabilities at validate time
unreasonably strict? A guest which uses eg. an invalid combination
of machine type and architecture will never become valid at a later
point, but a guest should not be considered invalid just because
the QEMU binary you happened to have installed at the time you
defined it lacked some features - the guest itself is perfectly
valid, it just can't be run :)

-- 
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 07/15] qemu: Clear out qemuDomainDeviceDefValidateControllerPCI()

[Andrea Bolognani]

On Mon, 2018-02-19 at 07:28 +0100, Peter Krempa wrote:
> On Fri, Feb 16, 2018 at 17:28:04 +0100, Andrea Bolognani wrote:
> > We will rewrite pretty much every single line of this function
> > over the course of the next several commits, and starting from
> > a clean slate rather than replacing it bit by bit makes the
> > resulting diffs unmeasurably easier to read and understand,
> > and you need fewer of them to boot. Trust me, I tried the other
> > approach first :)
> 
> Will this remove any checks during the series? If yes, then you probably
> should at first rename this function and add a almost-empty wrapper then
> add new code to the wrapper and delete the renamed function at the end.

No, if anything it *adds* a bunch of checks :)

Renaming the function won't work because then the compiler will
complain about it being unused. Unless you meant something like

  /* Delete once done */
  ValidateControllerPCIOld() {
/* Existing checks here */
  }

  ValidateControllerPCI() {
/* New checks here */

/* Delete once done */
ValidateControllerPCIOld();
  }

which could actually do the trick.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [RFC] libvirt hexagon sticker design

[Marcin Juszkiewicz]

W dniu 19.02.2018 o 09:44, Erik Skultety pisze:

> So, I've taken our libvirt-publican repo and came up with a few
> various color combinations for libvirt hexagon sticker design. 

> [1] https://drive.google.com/open?id=1xhn0GLvCKMcEOUnqn6boghaUOgjA8sOG 
> [2] https://drive.google.com/open?id=1b2MUyv0eo723F2rorWnmv1sTeCycUCN4 

After taking a look at my laptop I prefer second version. Just no white
border please (too big contrast).

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

[libvirt] [RFC] libvirt hexagon sticker design

[Erik Skultety]

Hi folks,
those who attended at least one conference for the past year have probably
noticed the rising trend (more like "sticker hype") of FOSS projects giving away
these hexagon stickers, it's very inexpensive way of making some promo for
their project and since we don't do many promos (AFAIK none to be precise) I
guess as a project that's been going strong for 12 years already we should
probably start somewhere, even baby steps count (as it turns out in this case -
- literally...). So, I've taken our libvirt-publican repo and came up with a
few various color combinations for libvirt hexagon sticker design. Below you
can find links to my personal google drive (these are hexagon meshes, I can,
or anyone can for that matter, isolate individual designs and send them as
separate patches on demand), since each of the SVGs is over 1.5MB and I'd easily
run into message size limits for the mailing list, had I gone with sending these
as patches against libvirt-publican.

Lastly, I just want to point out, if it's not obvious, that the last design was
meant as a joke (apparently I had a bit too much fun spending time on this)
reflecting what's becoming "the new black" in "sticker design world" - emphasis
on simplification while retaining its expressiveness and unambiguity (yeah,
right...) - but I'm not going to point the finger, but there's a heptagon
involved ;).

Cheers,
Erik

[1] https://drive.google.com/open?id=1xhn0GLvCKMcEOUnqn6boghaUOgjA8sOG
[2] https://drive.google.com/open?id=1b2MUyv0eo723F2rorWnmv1sTeCycUCN4
[joke] https://drive.google.com/open?id=1SBjJn2eptXFQVPuEMxzGY8CX6TFX1IzP
[3] https://github.com/terinjokes/StickerConstructorSpec

PS: the last link is the specification I used for the design
PS-2: if someone would like to point out that the joke proposal is missing a
few color combination, it was on purpose...

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

[libvirt] [PATCH 1/2] virsh: Fix internal naming of some blockjob commands

[Peter Krempa]

The variable names for the options and information about a command
should have an underscore in places where the virsh command has a
hyphen. The function callback name should capitalize the letter after
the hyphen. This was not used in 'blockcommit', 'blockcopy', 'blockjob',
'blockpull', and 'blockresize' commands.

Signed-off-by: Peter Krempa 
---
 tools/virsh-domain.c | 60 ++--
 1 file changed, 30 insertions(+), 30 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index d158327bd7..29bc8e6db1 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -1874,7 +1874,7 @@ virshBlockJobWait(virshBlockJobWaitDataPtr data)
 /*
  * "blockcommit" command
  */
-static const vshCmdInfo info_block_commit[] = {
+static const vshCmdInfo info_blockcommit[] = {
 {.name = "help",
  .data = N_("Start a block commit operation.")
 },
@@ -1884,7 +1884,7 @@ static const vshCmdInfo info_block_commit[] = {
 {.name = NULL}
 };

-static const vshCmdOptDef opts_block_commit[] = {
+static const vshCmdOptDef opts_blockcommit[] = {
 VIRSH_COMMON_OPT_DOMAIN_FULL(0),
 {.name = "path",
  .type = VSH_OT_DATA,
@@ -1952,7 +1952,7 @@ static const vshCmdOptDef opts_block_commit[] = {
 };

 static bool
-cmdBlockCommit(vshControl *ctl, const vshCmd *cmd)
+cmdBlockcommit(vshControl *ctl, const vshCmd *cmd)
 {
 virDomainPtr dom = NULL;
 bool ret = false;
@@ -2099,7 +2099,7 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd)
 /*
  * "blockcopy" command
  */
-static const vshCmdInfo info_block_copy[] = {
+static const vshCmdInfo info_blockcopy[] = {
 {.name = "help",
  .data = N_("Start a block copy operation.")
 },
@@ -2109,7 +2109,7 @@ static const vshCmdInfo info_block_copy[] = {
 {.name = NULL}
 };

-static const vshCmdOptDef opts_block_copy[] = {
+static const vshCmdOptDef opts_blockcopy[] = {
 VIRSH_COMMON_OPT_DOMAIN_FULL(0),
 {.name = "path",
  .type = VSH_OT_DATA,
@@ -2192,7 +2192,7 @@ static const vshCmdOptDef opts_block_copy[] = {
 };

 static bool
-cmdBlockCopy(vshControl *ctl, const vshCmd *cmd)
+cmdBlockcopy(vshControl *ctl, const vshCmd *cmd)
 {
 virDomainPtr dom = NULL;
 const char *dest = NULL;
@@ -2415,7 +2415,7 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd)
 /*
  * "blockjob" command
  */
-static const vshCmdInfo info_block_job[] = {
+static const vshCmdInfo info_blockjob[] = {
 {.name = "help",
  .data = N_("Manage active block operations")
 },
@@ -2425,7 +2425,7 @@ static const vshCmdInfo info_block_job[] = {
 {.name = NULL}
 };

-static const vshCmdOptDef opts_block_job[] = {
+static const vshCmdOptDef opts_blockjob[] = {
 VIRSH_COMMON_OPT_DOMAIN_FULL(0),
 {.name = "path",
  .type = VSH_OT_DATA,
@@ -2609,7 +2609,7 @@ virshBlockJobAbort(virDomainPtr dom,


 static bool
-cmdBlockJob(vshControl *ctl, const vshCmd *cmd)
+cmdBlockjob(vshControl *ctl, const vshCmd *cmd)
 {
 bool ret = false;
 bool raw = vshCommandOptBool(cmd, "raw");
@@ -2658,7 +2658,7 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd)
 /*
  * "blockpull" command
  */
-static const vshCmdInfo info_block_pull[] = {
+static const vshCmdInfo info_blockpull[] = {
 {.name = "help",
  .data = N_("Populate a disk from its backing image.")
 },
@@ -2668,7 +2668,7 @@ static const vshCmdInfo info_block_pull[] = {
 {.name = NULL}
 };

-static const vshCmdOptDef opts_block_pull[] = {
+static const vshCmdOptDef opts_blockpull[] = {
 VIRSH_COMMON_OPT_DOMAIN_FULL(0),
 {.name = "path",
  .type = VSH_OT_DATA,
@@ -2711,7 +2711,7 @@ static const vshCmdOptDef opts_block_pull[] = {
 };

 static bool
-cmdBlockPull(vshControl *ctl, const vshCmd *cmd)
+cmdBlockpull(vshControl *ctl, const vshCmd *cmd)
 {
 virDomainPtr dom = NULL;
 bool ret = false;
@@ -2804,7 +2804,7 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd)
 /*
  * "blockresize" command
  */
-static const vshCmdInfo info_block_resize[] = {
+static const vshCmdInfo info_blockresize[] = {
 {.name = "help",
  .data = N_("Resize block device of domain.")
 },
@@ -2814,7 +2814,7 @@ static const vshCmdInfo info_block_resize[] = {
 {.name = NULL}
 };

-static const vshCmdOptDef opts_block_resize[] = {
+static const vshCmdOptDef opts_blockresize[] = {
 VIRSH_COMMON_OPT_DOMAIN_FULL(0),
 {.name = "path",
  .type = VSH_OT_DATA,
@@ -2830,7 +2830,7 @@ static const vshCmdOptDef opts_block_resize[] = {
 };

 static bool
-cmdBlockResize(vshControl *ctl, const vshCmd *cmd)
+cmdBlockresize(vshControl *ctl, const vshCmd *cmd)
 {
 virDomainPtr dom;
 const char *path = NULL;
@@ -13910,33 +13910,33 @@ const vshCmdDef domManagementCmds[] = {
  .flags = 0
 },
 {.name = "blockcommit",
- .handler = cmdBlockCommit,
- .opts = opts_block_commit,
- .info = info_block_commit,
+ .handler = cmdBlockcommit,
+ .opts = opts_blockcommit,
+ .info = 

[libvirt] [PATCH 2/2] qemu: blockcopy: Add check for bandwidth

[Peter Krempa]

QEMU code does not work well with too big numbers on the JSON monitor so
our monitor code supports sending only numbers up to LLONG_MAX. Avoid a
weird error message by limiting the size of the 'bandwidth' parameter
for block copy.

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

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_driver.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index bbce5bd81b..9e7ac1cfa0 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17218,6 +17218,14 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
 goto cleanup;
 }

+if (bandwidth > LLONG_MAX) {
+virReportError(VIR_ERR_INVALID_ARG,
+   _("bandwidth must be less than "
+ "'%llu' bytes/s (%llu MiB/s)"),
+   LLONG_MAX, LLONG_MAX >> 20);
+goto cleanup;
+}
+
 if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
 goto cleanup;

-- 
2.15.0

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

[libvirt] [PATCH 0/2] Two blockjob fixes

[Peter Krempa]

Peter Krempa (2):
  virsh: Fix internal naming of some blockjob commands
  qemu: blockcopy: Add check for bandwidth

 src/qemu/qemu_driver.c |  8 +++
 tools/virsh-domain.c   | 60 +-
 2 files changed, 38 insertions(+), 30 deletions(-)

-- 
2.15.0

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