Re: [PATCH 1/1] bridge_driver: use ovs-vsctl to setup and clean Qos when

[Laine Stump]

On 11/8/21 5:18 AM, Michal Prívozník wrote:

On 11/3/21 4:11 AM, jx8zjs wrote:

From: zhangjl02 

Fix bug 1826168: bridge type network with ovs bridge can start with Qos
setting which do not take any effect

Resolves:https://bugzilla.redhat.com/show_bug.cgi?id=1826168
Signed-off-by: zhangjl02 
---
  src/network/bridge_driver.c | 65 +++--
  1 file changed, 55 insertions(+), 10 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 498c45d0a7..d0627848cd 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -2305,6 +2305,15 @@ networkAddRouteToBridge(virNetworkObj *obj,
  }
  
  
+static int

+networkDefIsOvsBridge(virNetworkDef *def)


This @def should have been const too. And function returns a bool,
effectively.


+{
+const virNetDevVPortProfile *vport = def->virtPortProfile;
+return vport &&
+vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH;
+}
+
+
  static int
  networkStartNetworkVirtual(virNetworkDriverState *driver,
 virNetworkObj *obj)
@@ -2320,6 +2329,7 @@ networkStartNetworkVirtual(virNetworkDriverState *driver,
  bool dnsmasqStarted = false;
  bool devOnline = false;
  bool firewalRulesAdded = false;
+bool ovsType = networkDefIsOvsBridge(def);
  
  /* Check to see if any network IP collides with an existing route */

  if (networkCheckRouteCollision(def) < 0)
@@ -2439,15 +2449,29 @@ networkStartNetworkVirtual(virNetworkDriverState 
*driver,
  if (v6present && networkStartRadvd(driver, obj) < 0)
  goto error;
  
-if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0)

-goto error;
+if (ovsType) {
+if (virNetDevOpenvswitchInterfaceSetQos(def->bridge, def->bandwidth,
+def->uuid,
+true) < 0)
+goto error;
+} else {
+if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0)
+goto error;
+}


I don't think this should be here. At this point, the bridge was created
using netlink (definitely NOT ovs-vsctl add-br). Therefore,
virNetDevOpenvswitchInterfaceSetQos() must fail because it won't be able
to find any ports/queues/...


Beyond that, networks of the type that would end up calling 
networkStartNetworkVirtual (forward modes 'route' and "nat') don't 
support using OVS bridges anyway, so this code in 
networkStartNetworkVirtual and networkShutdownNetworkVirtual will never 
be executed anyway.




  
  return 0;
  
   error:

  virErrorPreserveLast(_err);
-if (def->bandwidth)
-   virNetDevBandwidthClear(def->bridge);
+if (ovsType) {
+if (virNetDevOpenvswitchInterfaceClearQos(def->bridge, def->uuid) < 0)
+VIR_WARN("cannot clear bandwidth setting for ovs bridge : %s",
+ def->bridge);
+} else {
+if (def->bandwidth) {
+virNetDevBandwidthClear(def->bridge);
+}
+}
  


Similarly, this shouldn't be here either.


  if (dnsmasqStarted) {
  pid_t dnsmasqPid = virNetworkObjGetDnsmasqPid(obj);
@@ -2536,13 +2560,21 @@ static int
  networkStartNetworkBridge(virNetworkObj *obj)
  {
  virNetworkDef *def = virNetworkObjGetDef(obj);
+bool ovsType = networkDefIsOvsBridge(def);
  
  /* put anything here that needs to be done each time a network of

   * type BRIDGE, is started. On failure, undo anything you've done,
   * and return -1. On success return 0.
   */
-if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0)
-goto error;
+if (ovsType) {
+if (virNetDevOpenvswitchInterfaceSetQos(def->bridge, def->bandwidth,
+def->uuid,
+true) < 0)
+goto error;
+} else {
+if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0)
+goto error;
+}
  
  if (networkStartHandleMACTableManagerMode(obj) < 0)

  goto error;
@@ -2550,8 +2582,15 @@ networkStartNetworkBridge(virNetworkObj *obj)
  return 0;
  
   error:

-if (def->bandwidth)
-   virNetDevBandwidthClear(def->bridge);
+if (ovsType) {
+if (virNetDevOpenvswitchInterfaceClearQos(def->bridge, def->uuid) < 0)
+VIR_WARN("cannot clear bandwidth setting for ovs bridge : %s",
+ def->bridge);
+} else {
+if (def->bandwidth) {
+virNetDevBandwidthClear(def->bridge);
+}
+}
  return -1;
  }
  
@@ -2565,9 +2604,15 @@ networkShutdownNetworkBridge(virNetworkObj *obj G_GNUC_UNUSED)

   * type BRIDGE is shutdown. On failure, undo anything you've done,
   * and return -1. On success return 0.
   */
-if (def->bandwidth)
-   virNetDevBandwidthClear(def->bridge);
  
+if 

Re: [PATCH] qemu: tpm: Initialize variable with NULL

[Michal Prívozník]

On 11/8/21 3:50 PM, Stefan Berger wrote:
> 
> On 11/8/21 09:40, Ján Tomko wrote:
>> On a Monday in 2021, Marc-André Lureau wrote:
>>> On Mon, Nov 8, 2021 at 5:53 PM Stefan Berger 
>>> wrote:

 Initialize an autofree'd variable with NULL that causes crashes
 if a TPM 1.2 is used and the variable doesn't get a value assigned.

 Signed-off-by: Stefan Berger 
>>>
>>> certainly, I wished there would be a gcc error there..
>>>
>>
>> There was
>> https://gitlab.com/libvirt/libvirt/-/pipelines/402739013/failures
>>
>> And this was already patched by Peter:
>> commit c43f22d5c1ea08c5516d791010ed3c230c47cb53
>>     qemuTPMEmulatorReconfigure: Fix two build issues
> 
> Sorry for the noise. F34 gcc hadn't complained.
> 

I've noticed that some of these warnings fire only when compiling with
-O2. Since I'm debugging a lot I'm in habit of setting -O0 -ggdb. And
that's when gcc did not emit the warning.

Michal

Re: [PATCH] qemu: tpm: Initialize variable with NULL

[Stefan Berger]

On 11/8/21 09:40, Ján Tomko wrote:

On a Monday in 2021, Marc-André Lureau wrote:
On Mon, Nov 8, 2021 at 5:53 PM Stefan Berger  
wrote:


Initialize an autofree'd variable with NULL that causes crashes
if a TPM 1.2 is used and the variable doesn't get a value assigned.

Signed-off-by: Stefan Berger 


certainly, I wished there would be a gcc error there..



There was
https://gitlab.com/libvirt/libvirt/-/pipelines/402739013/failures

And this was already patched by Peter:
commit c43f22d5c1ea08c5516d791010ed3c230c47cb53
    qemuTPMEmulatorReconfigure: Fix two build issues


Sorry for the noise. F34 gcc hadn't complained.


   Stefan

Re: [PATCH] qemu: tpm: Initialize variable with NULL

[Ján Tomko]

On a Monday in 2021, Marc-André Lureau wrote:

On Mon, Nov 8, 2021 at 5:53 PM Stefan Berger  wrote:


Initialize an autofree'd variable with NULL that causes crashes
if a TPM 1.2 is used and the variable doesn't get a value assigned.

Signed-off-by: Stefan Berger 


certainly, I wished there would be a gcc error there..



There was
https://gitlab.com/libvirt/libvirt/-/pipelines/402739013/failures

And this was already patched by Peter:
commit c43f22d5c1ea08c5516d791010ed3c230c47cb53
qemuTPMEmulatorReconfigure: Fix two build issues

Jano


Reviewed-by: Marc-André Lureau 


---
 src/qemu/qemu_tpm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


signature.asc
Description: PGP signature

Re: [PATCH] qemu: tpm: Initialize variable with NULL

[Daniel P . Berrangé]

On Mon, Nov 08, 2021 at 06:06:50PM +0400, Marc-André Lureau wrote:
> On Mon, Nov 8, 2021 at 5:53 PM Stefan Berger  wrote:
> >
> > Initialize an autofree'd variable with NULL that causes crashes
> > if a TPM 1.2 is used and the variable doesn't get a value assigned.
> >
> > Signed-off-by: Stefan Berger 
> 
> certainly, I wished there would be a gcc error there..

I'd support a coding rule for libvirt that *every* single stack variable
must *always* be initialized at time of declaration, even if there's an
initialization happening later in the method, even if it is on the very
next line.

We've had way too many bugs from leaving variables uninitialized over
the years, that mandatory explicit init is worthwhile on balance IMHO.

It isn't that easy to detect this in a coding style rule though, as
parsing C with regexes is flakey at best. I wonder if libclang could
be used to write a better style check.

Meanwhile, I'm looking forward to GCC 12 which introduces a flag

   $CC -ftrivial-auto-var-init=zero

to tell the compiler to initialize everything to zero. Annoyingly
that LLVM maintainers have had this in clang for a while, but don't
want to officially support this nice enhancement to the security
and reliability of C code, so hide this feature behind a crazy
flag [1]

> 
> Reviewed-by: Marc-André Lureau 
> 
> > ---
> >  src/qemu/qemu_tpm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
> > index b305313ad2..1992b596cb 100644
> > --- a/src/qemu/qemu_tpm.c
> > +++ b/src/qemu/qemu_tpm.c
> > @@ -610,7 +610,7 @@ qemuTPMEmulatorReconfigure(const char *storagepath,
> >  {
> >  g_autoptr(virCommand) cmd = NULL;
> >  int exitstatus;
> > -g_autofree char *activePcrBanksStr;
> > +g_autofree char *activePcrBanksStr = NULL;
> >  g_autofree char *swtpm_setup = virTPMGetSwtpmSetup();
> >  VIR_AUTOCLOSE pwdfile_fd = -1;
> >
> > --
> > 2.31.1
> >
> 

Regards,
Daniel

[1]  $  clang -ftrivial-auto-var-init=zero demo.c
 clang-12: error: -ftrivial-auto-var-init=zero hasn't been enabled. Enable 
it at your own peril for benchmarking purpose only with 
-enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang


-- 
|: 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 :|

Re: [PATCH] qemu: tpm: Initialize variable with NULL

[Marc-André Lureau]

On Mon, Nov 8, 2021 at 5:53 PM Stefan Berger  wrote:
>
> Initialize an autofree'd variable with NULL that causes crashes
> if a TPM 1.2 is used and the variable doesn't get a value assigned.
>
> Signed-off-by: Stefan Berger 

certainly, I wished there would be a gcc error there..

Reviewed-by: Marc-André Lureau 

> ---
>  src/qemu/qemu_tpm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
> index b305313ad2..1992b596cb 100644
> --- a/src/qemu/qemu_tpm.c
> +++ b/src/qemu/qemu_tpm.c
> @@ -610,7 +610,7 @@ qemuTPMEmulatorReconfigure(const char *storagepath,
>  {
>  g_autoptr(virCommand) cmd = NULL;
>  int exitstatus;
> -g_autofree char *activePcrBanksStr;
> +g_autofree char *activePcrBanksStr = NULL;
>  g_autofree char *swtpm_setup = virTPMGetSwtpmSetup();
>  VIR_AUTOCLOSE pwdfile_fd = -1;
>
> --
> 2.31.1
>

[PATCH] qemu: tpm: Initialize variable with NULL

[Stefan Berger]

Initialize an autofree'd variable with NULL that causes crashes
if a TPM 1.2 is used and the variable doesn't get a value assigned.

Signed-off-by: Stefan Berger 
---
 src/qemu/qemu_tpm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index b305313ad2..1992b596cb 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -610,7 +610,7 @@ qemuTPMEmulatorReconfigure(const char *storagepath,
 {
 g_autoptr(virCommand) cmd = NULL;
 int exitstatus;
-g_autofree char *activePcrBanksStr;
+g_autofree char *activePcrBanksStr = NULL;
 g_autofree char *swtpm_setup = virTPMGetSwtpmSetup();
 VIR_AUTOCLOSE pwdfile_fd = -1;
 
-- 
2.31.1

Re: [libvirt PATCH 04/10] virNetworkEventDispatchDefaultFunc: Cleanup

[Michal Prívozník]

On 11/8/21 1:17 PM, Tim Wiederhake wrote:
> Remove unnecessary label and goto.
> 
> Signed-off-by: Tim Wiederhake 
> ---
>  src/conf/network_event.c | 9 +++--
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/src/conf/network_event.c b/src/conf/network_event.c
> index a47bf4dd3e..6335cbf711 100644
> --- a/src/conf/network_event.c
> +++ b/src/conf/network_event.c
> @@ -86,8 +86,8 @@ virNetworkEventDispatchDefaultFunc(virConnectPtr conn,
> virConnectObjectEventGenericCallback cb,
> void *cbopaque)
>  {
> -virNetworkPtr net = virGetNetwork(conn, event->meta.name, 
> event->meta.uuid);
> -if (!net)
> +g_autoptr(virNetwork) net = NULL;
> +if (!(net = virGetNetwork(conn, event->meta.name, event->meta.uuid)))

I think we can use this opportunity to put an empty line in between
declaration and code blocks.

>  return;
>  
>  switch ((virNetworkEventID)event->eventID) {
> @@ -100,16 +100,13 @@ virNetworkEventDispatchDefaultFunc(virConnectPtr conn,
>
> networkLifecycleEvent->type,
>
> networkLifecycleEvent->detail,
>cbopaque);
> -goto cleanup;
> +return;
>  }
>  
>  case VIR_NETWORK_EVENT_ID_LAST:
>  break;
>  }
>  VIR_WARN("Unexpected event ID %d", event->eventID);
> -
> - cleanup:
> -virObjectUnref(net);
>  }
>  
>  
> 

Michal

Re: [libvirt PATCH 00/10] Cleanup some cleanup / error paths

[Michal Prívozník]

On 11/8/21 1:17 PM, Tim Wiederhake wrote:
> 
> 
> Tim Wiederhake (10):
>   adminConnectListServers: Cleanup
>   virCHDomainObjBeginJob: Cleanup
>   virDomainCapsCPUModelsCopy: Cleanup
>   virNetworkEventDispatchDefaultFunc: Cleanup
>   virSaveCookieParse: Cleanup
>   virBufferAddBuffer: Cleanup
>   virSCSIVHostOpenVhostSCSI: Cleanup
>   fillXenCaps: Cleanup
>   testLXCCapsInit: Cleanup
>   testVshTableHeader: Cleanup
> 
>  src/admin/admin_server.c   |  4 ++--
>  src/ch/ch_domain.c | 34 --
>  src/conf/domain_capabilities.c | 10 +++---
>  src/conf/network_event.c   |  9 +++--
>  src/conf/virsavecookie.c   |  9 ++---
>  src/util/virbuffer.c   | 10 --
>  src/util/virscsivhost.c|  7 +--
>  tests/domaincapstest.c | 18 +-
>  tests/testutilslxc.c   | 17 +
>  tests/vshtabletest.c   | 12 +---
>  10 files changed, 46 insertions(+), 84 deletions(-)
> 

Reviewed-by: Michal Privoznik 

Michal

Re: [libvirt PATCHv2 5/7] tests: convert name-escape to use real caps

[Tim Wiederhake]

On Wed, 2021-11-03 at 11:53 +0100, Ján Tomko wrote:
> For later QEMUs than 2.11 we do FD passing for character devices,
> so lock the capabilites to this exact version.
> 
> Signed-off-by: Ján Tomko 

Reviewed-by: Tim Wiederhake 

> ---
>  ...-escape.args => name-escape.x86_64-2.11.0.args} |  7 ---
>  tests/qemuxml2argvtest.c   | 14 +---
> --
>  2 files changed, 5 insertions(+), 16 deletions(-)
>  rename tests/qemuxml2argvdata/{name-escape.args => name-
> escape.x86_64-2.11.0.args} (90%)
> 
> diff --git a/tests/qemuxml2argvdata/name-escape.args
> b/tests/qemuxml2argvdata/name-escape.x86_64-2.11.0.args
> similarity index 90%
> rename from tests/qemuxml2argvdata/name-escape.args
> rename to tests/qemuxml2argvdata/name-escape.x86_64-2.11.0.args
> index eb8d9ac10a..71770dc546 100644
> --- a/tests/qemuxml2argvdata/name-escape.args
> +++ b/tests/qemuxml2argvdata/name-escape.x86_64-2.11.0.args
> @@ -11,7 +11,7 @@ QEMU_AUDIO_DRV=spice \
>  -name guest=foo=1,,bar=2,debug-threads=on \
>  -S \
>  -object secret,id=masterKey0,format=raw,file=/tmp/lib/domain--1-
> foo=1,,bar=2/master-key.aes \
> --machine pc,accel=tcg,usb=off,dump-guest-core=off \
> +-machine pc-i440fx-2.11,accel=tcg,usb=off,dump-guest-core=off \
>  -m 214 \
>  -realtime mlock=off \
>  -smp 1,sockets=1,cores=1,threads=1 \
> @@ -24,11 +24,11 @@ QEMU_AUDIO_DRV=spice \
>  -no-shutdown \
>  -no-acpi \
>  -boot strict=on \
> +-device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \
>  -device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x3 \
> --usb \
>  -device usb-ccid,id=ccid0,bus=usb.0,port=1 \
>  -drive file=/dev/HostVG/QEMUGuest1,format=qcow2,if=none,id=drive-
> ide0-0-0,cache=none,throttling.bps-total=5000,throttling.iops-
> total=6000,throttling.bps-total-max=1,throttling.iops-total-
> max=11000,throttling.group=libvirt_iotune_group1,,foo \
> --device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-
> 0,bootindex=1 \
> +-device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-
> 0,bootindex=1,write-cache=on \
>  -device ccid-card-
> emulated,backend=certificates,cert1=cert1,,foo,cert2=cert2,cert3=cert
> 3,db=/etc/pki/nssdb,,foo,id=smartcard0,bus=ccid0.0 \
>  -chardev tty,id=charserial0,path=/dev/ttyS2,,foo \
>  -device isa-serial,chardev=charserial0,id=serial0 \
> @@ -42,4 +42,5 @@ QEMU_AUDIO_DRV=spice \
>  -drive
> file.driver=iscsi,file.portal=example.foo.org:3260,file.target=iqn.19
> 92-
> 01.com.example:my,,storage,file.lun=1,file.transport=tcp,if=none,form
> at=raw,id=drive-hostdev0 \
>  -device scsi-generic,drive=drive-
> hostdev0,id=hostdev0,bus=scsi0.0,channel=0,scsi-id=0,lun=4 \
>  -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 \
> +-sandbox
> on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=de
> ny \
>  -msg timestamp=on
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index a171a978bd..a0498a0d92 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -3178,19 +3178,7 @@ mymain(void)
>   ARG_PARSEFLAGS, VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE,
>   ARG_END);
>  
> -    DO_TEST("name-escape",
> -    QEMU_CAPS_VNC,
> -    QEMU_CAPS_DEVICE_CIRRUS_VGA,
> -    QEMU_CAPS_SPICE,
> -    QEMU_CAPS_SPICE_UNIX,
> -    QEMU_CAPS_DEVICE_VIRTIO_GPU,
> -    QEMU_CAPS_VIRTIO_GPU_VIRGL,
> -    QEMU_CAPS_SPICE_GL,
> -    QEMU_CAPS_SPICE_RENDERNODE,
> -    QEMU_CAPS_DEVICE_ISA_SERIAL,
> -    QEMU_CAPS_CHARDEV_FILE_APPEND,
> -    QEMU_CAPS_CCID_EMULATED,
> -    QEMU_CAPS_VIRTIO_SCSI);
> +    DO_TEST_CAPS_VER("name-escape", "2.11.0");
>  
>  DO_TEST_NOCAPS("master-key");
>  DO_TEST("usb-long-port-path",

Re: [PATCH] docs/about/deprecated: Remove empty 'related binaries' section

[Joaquin de Andres]

On 11/5/21 15:26, Philippe Mathieu-Daudé wrote:

Commit 497a30dbb06 ("qemu-img: Require -F with -b backing image")
removed the content of the "Related binaries" section but forgot
to remove the section title. Since it is now empty, remove it too.

Signed-off-by: Philippe Mathieu-Daudé 
---
  docs/about/deprecated.rst | 3 ---
  1 file changed, 3 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 56f9ad15ab5..5e514fb443d 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -370,9 +370,6 @@ The ``I7200`` guest CPU relies on the nanoMIPS ISA, which 
is deprecated
  (the ISA has never been upstreamed to a compiler toolchain). Therefore
  this CPU is also deprecated.
  
-Related binaries

-
-
  Backwards compatibility
  ---
  



Reviewed-by: Joaquin de Andres 

Re: [PATCH] bridge_driver: Drop needless fwd declarations

[Martin Kletzander]

On Mon, Nov 08, 2021 at 11:47:27AM +0100, Michal Privoznik wrote:

Some forward declarations in bridge_driver.c are not needed
really. They only create a noise when trying to jump onto the
correct tag. Drop them.

Signed-off-by: Michal Privoznik 


Reviewed-by: Martin Kletzander 


---
src/network/bridge_driver.c | 14 --
1 file changed, 14 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 498c45d0a7..7e5fab630b 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -259,20 +259,6 @@ static int
networkShutdownNetwork(virNetworkDriverState *driver,
   virNetworkObj *obj);

-static int
-networkStartNetworkVirtual(virNetworkDriverState *driver,
-   virNetworkObj *obj);
-
-static int
-networkShutdownNetworkVirtual(virNetworkDriverState *driver,
-  virNetworkObj *obj);
-
-static int
-networkStartNetworkExternal(virNetworkObj *obj);
-
-static int
-networkShutdownNetworkExternal(virNetworkObj *obj);
-
static void
networkReloadFirewallRules(virNetworkDriverState *driver,
   bool startup,
--
2.32.0



signature.asc
Description: PGP signature

Re: [PATCH] bridge_driver: Drop needless fwd declarations

[Tim Wiederhake]

On Mon, 2021-11-08 at 11:47 +0100, Michal Privoznik wrote:
> Some forward declarations in bridge_driver.c are not needed
> really. They only create a noise when trying to jump onto the
> correct tag. Drop them.
> 
> Signed-off-by: Michal Privoznik 

Reviewed-by: Tim Wiederhake 

> ---
>  src/network/bridge_driver.c | 14 --
>  1 file changed, 14 deletions(-)
> 
> diff --git a/src/network/bridge_driver.c
> b/src/network/bridge_driver.c
> index 498c45d0a7..7e5fab630b 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -259,20 +259,6 @@ static int
>  networkShutdownNetwork(virNetworkDriverState *driver,
>     virNetworkObj *obj);
>  
> -static int
> -networkStartNetworkVirtual(virNetworkDriverState *driver,
> -   virNetworkObj *obj);
> -
> -static int
> -networkShutdownNetworkVirtual(virNetworkDriverState *driver,
> -  virNetworkObj *obj);
> -
> -static int
> -networkStartNetworkExternal(virNetworkObj *obj);
> -
> -static int
> -networkShutdownNetworkExternal(virNetworkObj *obj);
> -
>  static void
>  networkReloadFirewallRules(virNetworkDriverState *driver,
>     bool startup,

[libvirt PATCH 09/10] testLXCCapsInit: Cleanup

[Tim Wiederhake]

Remove unnecessary label and goto. Cleanup line breaks.

Signed-off-by: Tim Wiederhake 
---
 tests/testutilslxc.c | 17 +
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/tests/testutilslxc.c b/tests/testutilslxc.c
index 857407dfb2..ac7a01a4e8 100644
--- a/tests/testutilslxc.c
+++ b/tests/testutilslxc.c
@@ -11,11 +11,10 @@
 virCaps *
 testLXCCapsInit(void)
 {
-virCaps *caps;
+g_autoptr(virCaps) caps = NULL;
 virCapsGuest *guest;
 
-if ((caps = virCapabilitiesNew(VIR_ARCH_X86_64,
-   false, false)) == NULL)
+if ((caps = virCapabilitiesNew(VIR_ARCH_X86_64, false, false)) == NULL)
 return NULL;
 
 guest = virCapabilitiesAddGuest(caps, VIR_DOMAIN_OSTYPE_EXE,
@@ -33,20 +32,14 @@ testLXCCapsInit(void)
 virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_LXC, NULL, NULL, 0, 
NULL);
 
 if (virTestGetDebug()) {
-g_autofree char *caps_str = NULL;
-
-caps_str = virCapabilitiesFormatXML(caps);
+g_autofree char *caps_str = virCapabilitiesFormatXML(caps);
 if (!caps_str)
-goto error;
+return NULL;
 
 VIR_TEST_DEBUG("LXC driver capabilities:\n%s", caps_str);
 }
 
-return caps;
-
- error:
-virObjectUnref(caps);
-return NULL;
+return g_steal_pointer();
 }
 
 
-- 
2.31.1

[libvirt PATCH 10/10] testVshTableHeader: Cleanup

[Tim Wiederhake]

Remove unnecessary label and goto. This also fixes a bug where a
failure to create the table would result in the test passing.

Signed-off-by: Tim Wiederhake 
---
 tests/vshtabletest.c | 12 +---
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/tests/vshtabletest.c b/tests/vshtabletest.c
index 716b11dbc0..41ceec0a51 100644
--- a/tests/vshtabletest.c
+++ b/tests/vshtabletest.c
@@ -45,7 +45,8 @@ static int
 testVshTableHeader(const void *opaque G_GNUC_UNUSED)
 {
 int ret = 0;
-char *act = NULL;
+g_autofree char *act = NULL;
+g_autofree char *act2 = NULL;
 const char *exp =
 " 1   fedora28   running\n"
 " 2   rhel7.5running\n";
@@ -58,7 +59,7 @@ testVshTableHeader(const void *opaque G_GNUC_UNUSED)
 g_autoptr(vshTable) table = vshTableNew("Id", "Name", "State",
 NULL); //to ask about return
 if (!table)
-goto cleanup;
+return -1;
 
 vshTableRowAppend(table, "1", "fedora28", "running", NULL);
 vshTableRowAppend(table, "2", "rhel7.5", "running",
@@ -68,13 +69,10 @@ testVshTableHeader(const void *opaque G_GNUC_UNUSED)
 if (virTestCompareToString(exp, act) < 0)
 ret = -1;
 
-VIR_FREE(act);
-act = vshTablePrintToString(table, true);
-if (virTestCompareToString(exp2, act) < 0)
+act2 = vshTablePrintToString(table, true);
+if (virTestCompareToString(exp2, act2) < 0)
 ret = -1;
 
- cleanup:
-VIR_FREE(act);
 return ret;
 }
 
-- 
2.31.1

[libvirt PATCH 08/10] fillXenCaps: Cleanup

[Tim Wiederhake]

Rework to remove unnecessary label and goto.

Signed-off-by: Tim Wiederhake 
---
 tests/domaincapstest.c | 18 +-
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c
index 9ea5bed5c2..4a46acb9ad 100644
--- a/tests/domaincapstest.c
+++ b/tests/domaincapstest.c
@@ -135,25 +135,17 @@ fillQemuCaps(virDomainCaps *domCaps,
 static int
 fillXenCaps(virDomainCaps *domCaps)
 {
-virFirmware **firmwares;
-int ret = -1;
-
-firmwares = g_new0(virFirmware *, 2);
-
-firmwares[0] = g_new0(virFirmware, 1);
-firmwares[1] = g_new0(virFirmware, 1);
+g_autoptr(virFirmware) fw_hvmloader = g_new0(virFirmware, 1);
+g_autoptr(virFirmware) fw_ovmf = g_new0(virFirmware, 1);
+virFirmware *firmwares[] = { fw_hvmloader, fw_ovmf };
 
 firmwares[0]->name = g_strdup("/usr/lib/xen/boot/hvmloader");
 firmwares[1]->name = g_strdup("/usr/lib/xen/boot/ovmf.bin");
 
 if (libxlMakeDomainCapabilities(domCaps, firmwares, 2) < 0)
-goto cleanup;
-
-ret = 0;
+return -1;
 
- cleanup:
-virFirmwareFreeList(firmwares, 2);
-return ret;
+return 0;
 }
 #endif /* WITH_LIBXL */
 
-- 
2.31.1

[libvirt PATCH 07/10] virSCSIVHostOpenVhostSCSI: Cleanup

[Tim Wiederhake]

Remove unnecessary label, goto, and closing of not-open file descriptor.

Signed-off-by: Tim Wiederhake 
---
 src/util/virscsivhost.c | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/src/util/virscsivhost.c b/src/util/virscsivhost.c
index afbfddb0fb..487301ab64 100644
--- a/src/util/virscsivhost.c
+++ b/src/util/virscsivhost.c
@@ -90,15 +90,10 @@ virSCSIVHostOpenVhostSCSI(int *vhostfd)
 
 if (*vhostfd < 0) {
 virReportSystemError(errno, _("Failed to open %s"), VHOST_SCSI_DEVICE);
-goto error;
+return -1;
 }
 
 return 0;
-
- error:
-VIR_FORCE_CLOSE(*vhostfd);
-
-return -1;
 }
 
 
-- 
2.31.1

[libvirt PATCH 06/10] virBufferAddBuffer: Cleanup

[Tim Wiederhake]

Remove unnecessary label and goto.

Signed-off-by: Tim Wiederhake 
---
 src/util/virbuffer.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c
index 8f9cd57e06..a4834174a1 100644
--- a/src/util/virbuffer.c
+++ b/src/util/virbuffer.c
@@ -185,13 +185,11 @@ virBufferAddBuffer(virBuffer *buf, virBuffer *toadd)
 if (!toadd || !toadd->str)
 return;
 
-if (!buf)
-goto cleanup;
-
-virBufferInitialize(buf);
-g_string_append_len(buf->str, toadd->str->str, toadd->str->len);
+if (buf) {
+virBufferInitialize(buf);
+g_string_append_len(buf->str, toadd->str->str, toadd->str->len);
+}
 
- cleanup:
 virBufferFreeAndReset(toadd);
 }
 
-- 
2.31.1

[libvirt PATCH 05/10] virSaveCookieParse: Cleanup

[Tim Wiederhake]

Remove unnecessary label and goto.

Signed-off-by: Tim Wiederhake 
---
 src/conf/virsavecookie.c | 9 ++---
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/src/conf/virsavecookie.c b/src/conf/virsavecookie.c
index 6cb7fafb1f..c24a292355 100644
--- a/src/conf/virsavecookie.c
+++ b/src/conf/virsavecookie.c
@@ -58,19 +58,14 @@ virSaveCookieParse(xmlXPathContextPtr ctxt,
virSaveCookieCallbacks *saveCookie)
 {
 VIR_XPATH_NODE_AUTORESTORE(ctxt)
-int ret = -1;
 
 *obj = NULL;
 
 if (!(ctxt->node = virXPathNode("./cookie", ctxt))) {
-ret = 0;
-goto cleanup;
+return 0;
 }
 
-ret = virSaveCookieParseNode(ctxt, obj, saveCookie);
-
- cleanup:
-return ret;
+return virSaveCookieParseNode(ctxt, obj, saveCookie);
 }
 
 
-- 
2.31.1

[libvirt PATCH 03/10] virDomainCapsCPUModelsCopy: Cleanup

[Tim Wiederhake]

Remove unnecessary label and goto.

Signed-off-by: Tim Wiederhake 
---
 src/conf/domain_capabilities.c | 10 +++---
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
index 22f0963326..1766129092 100644
--- a/src/conf/domain_capabilities.c
+++ b/src/conf/domain_capabilities.c
@@ -157,7 +157,7 @@ virDomainCapsCPUModelsNew(size_t nmodels)
 virDomainCapsCPUModels *
 virDomainCapsCPUModelsCopy(virDomainCapsCPUModels *old)
 {
-virDomainCapsCPUModels *cpuModels;
+g_autoptr(virDomainCapsCPUModels) cpuModels = NULL;
 size_t i;
 
 if (!(cpuModels = virDomainCapsCPUModelsNew(old->nmodels)))
@@ -169,14 +169,10 @@ virDomainCapsCPUModelsCopy(virDomainCapsCPUModels *old)
   old->models[i].usable,
   old->models[i].blockers,
   old->models[i].deprecated) < 0)
-goto error;
+return NULL;
 }
 
-return cpuModels;
-
- error:
-virObjectUnref(cpuModels);
-return NULL;
+return g_steal_pointer();
 }
 
 
-- 
2.31.1

[libvirt PATCH 04/10] virNetworkEventDispatchDefaultFunc: Cleanup

[Tim Wiederhake]

Remove unnecessary label and goto.

Signed-off-by: Tim Wiederhake 
---
 src/conf/network_event.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/src/conf/network_event.c b/src/conf/network_event.c
index a47bf4dd3e..6335cbf711 100644
--- a/src/conf/network_event.c
+++ b/src/conf/network_event.c
@@ -86,8 +86,8 @@ virNetworkEventDispatchDefaultFunc(virConnectPtr conn,
virConnectObjectEventGenericCallback cb,
void *cbopaque)
 {
-virNetworkPtr net = virGetNetwork(conn, event->meta.name, 
event->meta.uuid);
-if (!net)
+g_autoptr(virNetwork) net = NULL;
+if (!(net = virGetNetwork(conn, event->meta.name, event->meta.uuid)))
 return;
 
 switch ((virNetworkEventID)event->eventID) {
@@ -100,16 +100,13 @@ virNetworkEventDispatchDefaultFunc(virConnectPtr conn,
   
networkLifecycleEvent->type,
   
networkLifecycleEvent->detail,
   cbopaque);
-goto cleanup;
+return;
 }
 
 case VIR_NETWORK_EVENT_ID_LAST:
 break;
 }
 VIR_WARN("Unexpected event ID %d", event->eventID);
-
- cleanup:
-virObjectUnref(net);
 }
 
 
-- 
2.31.1

[libvirt PATCH 01/10] adminConnectListServers: Cleanup

[Tim Wiederhake]

Remove unnecessary label and goto.

Signed-off-by: Tim Wiederhake 
---
 src/admin/admin_server.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/admin/admin_server.c b/src/admin/admin_server.c
index 623c682b2d..6731a366cf 100644
--- a/src/admin/admin_server.c
+++ b/src/admin/admin_server.c
@@ -47,12 +47,12 @@ adminConnectListServers(virNetDaemon *dmn,
 virCheckFlags(0, -1);
 
 if ((ret = virNetDaemonGetServers(dmn, )) < 0)
-goto cleanup;
+return ret;
 
 if (servers) {
 *servers = g_steal_pointer();
 }
- cleanup:
+
 if (ret > 0)
 virObjectListFreeCount(srvs, ret);
 return ret;
-- 
2.31.1

[libvirt PATCH 00/10] Cleanup some cleanup / error paths

[Tim Wiederhake]

Tim Wiederhake (10):
  adminConnectListServers: Cleanup
  virCHDomainObjBeginJob: Cleanup
  virDomainCapsCPUModelsCopy: Cleanup
  virNetworkEventDispatchDefaultFunc: Cleanup
  virSaveCookieParse: Cleanup
  virBufferAddBuffer: Cleanup
  virSCSIVHostOpenVhostSCSI: Cleanup
  fillXenCaps: Cleanup
  testLXCCapsInit: Cleanup
  testVshTableHeader: Cleanup

 src/admin/admin_server.c   |  4 ++--
 src/ch/ch_domain.c | 34 --
 src/conf/domain_capabilities.c | 10 +++---
 src/conf/network_event.c   |  9 +++--
 src/conf/virsavecookie.c   |  9 ++---
 src/util/virbuffer.c   | 10 --
 src/util/virscsivhost.c|  7 +--
 tests/domaincapstest.c | 18 +-
 tests/testutilslxc.c   | 17 +
 tests/vshtabletest.c   | 12 +---
 10 files changed, 46 insertions(+), 84 deletions(-)

-- 
2.31.1

[libvirt PATCH 02/10] virCHDomainObjBeginJob: Cleanup

[Tim Wiederhake]

Remove unnecessary label and goto.

Signed-off-by: Tim Wiederhake 
---
 src/ch/ch_domain.c | 34 --
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/src/ch/ch_domain.c b/src/ch/ch_domain.c
index 9d32d8669a..dd4de9e1ea 100644
--- a/src/ch/ch_domain.c
+++ b/src/ch/ch_domain.c
@@ -87,8 +87,22 @@ virCHDomainObjBeginJob(virDomainObj *obj, enum 
virCHDomainJob job)
 while (priv->job.active) {
 VIR_DEBUG("Wait normal job condition for starting job: %s",
   virCHDomainJobTypeToString(job));
-if (virCondWaitUntil(>job.cond, >parent.lock, then) < 0)
-goto error;
+if (virCondWaitUntil(>job.cond, >parent.lock, then) < 0) {
+VIR_WARN("Cannot start job (%s) for domain %s;"
+ " current job is (%s) owned by (%d)",
+ virCHDomainJobTypeToString(job),
+ obj->def->name,
+ virCHDomainJobTypeToString(priv->job.active),
+ priv->job.owner);
+
+if (errno == ETIMEDOUT)
+virReportError(VIR_ERR_OPERATION_TIMEOUT,
+   "%s", _("cannot acquire state change lock"));
+else
+virReportSystemError(errno,
+ "%s", _("cannot acquire job mutex"));
+return -1;
+}
 }
 
 virCHDomainObjResetJob(priv);
@@ -98,22 +112,6 @@ virCHDomainObjBeginJob(virDomainObj *obj, enum 
virCHDomainJob job)
 priv->job.owner = virThreadSelfID();
 
 return 0;
-
- error:
-VIR_WARN("Cannot start job (%s) for domain %s;"
- " current job is (%s) owned by (%d)",
- virCHDomainJobTypeToString(job),
- obj->def->name,
- virCHDomainJobTypeToString(priv->job.active),
- priv->job.owner);
-
-if (errno == ETIMEDOUT)
-virReportError(VIR_ERR_OPERATION_TIMEOUT,
-   "%s", _("cannot acquire state change lock"));
-else
-virReportSystemError(errno,
- "%s", _("cannot acquire job mutex"));
-return -1;
 }
 
 /*
-- 
2.31.1

Re: [PATCH v2 1/2] tests/acceptance: introduce new check-avocado tartget

[Willian Rampazzo]

On Mon, Nov 8, 2021 at 6:49 AM Philippe Mathieu-Daudé  wrote:
>
> On 11/8/21 10:24, Daniel P. Berrangé wrote:
> > On Mon, Nov 08, 2021 at 08:59:51AM +0100, Thomas Huth wrote:
> >> On 05/11/2021 16.53, Willian Rampazzo wrote:
> >>> This introduces a new `make` target, `check-avocado`, and adds a
> >>> deprecation message about the `check-acceptance` target. This is
> >>> a preparation for renaming the `tests/acceptance` folder to
> >>>   `tests/avocado`.
> >>>
> >>> The plan is to remove the call to the `check-avocado` target one
> >>> or two months after the release and leave the warning to force
> >>> people to move to the new `check-avocado` target.
> >>>
> >>> Later, the `check-acceptance` target can be removed. The intent
> >>> is to avoid a direct impact during the current soft freeze.
> >>>
> >>> Suggested-by: Philippe Mathieu-Daudé 
> >>> Signed-off-by: Willian Rampazzo 
> >>> ---
> >>>   docs/about/deprecated.rst | 13 +
> >>>   tests/Makefile.include| 17 -
> >>>   2 files changed, 25 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> >>> index 56f9ad15ab..7bf8da8325 100644
> >>> --- a/docs/about/deprecated.rst
> >>> +++ b/docs/about/deprecated.rst
> >>> @@ -410,3 +410,16 @@ nanoMIPS ISA
> >>>   The ``nanoMIPS`` ISA has never been upstreamed to any compiler 
> >>> toolchain.
> >>>   As it is hard to generate binaries for it, declare it deprecated.
> >>> +
> >>> +Testing
> >>> +---
> >>> +
> >>> +Renaming of the acceptance folder to avocado
> >>> +
> >>> +
> >>> +The ``tests/acceptance`` folder was never used to store acceptance tests
> >>> +in terms of software engineering. This naming can confuse developers
> >>> +adding tests using the Avocado Framework to this folder. The folder
> >>> +name change to ``tests/avocado`` also changed the ``make`` target from
> >>> +``check-acceptance`` to ``check-avocado``. In this case, the use of the
> >>> +``check-acceptance`` target is deprecated.
> >>
> >> Not sure whether we need  to document this in deprecated.rst, too, since
> >> we're normally only listing the things here that affect the users of the
> >> qemu binaries, not the people who want to recompile and run the tests...
> >> OTOH, I don't mind too much either if we list it here... Anybody else got 
> >> an
> >> opinion on this?
> >
> > Deprecations are only things for user facing changes in the apps.
>
> OK.
>
> > For build system changes we don't bother with any deprecation cycle.
> > Just make the change immediately and document it in the release notes.
>
> Understood.
>
> Willian, do you mind updating the release notes?
> https://wiki.qemu.org/ChangeLog/6.2#Testing_and_CI
>

Sure, I can do that, but I think I need to wait for the patch to be
merged, right?

[PATCH] bridge_driver: Drop needless fwd declarations

[Michal Privoznik]

Some forward declarations in bridge_driver.c are not needed
really. They only create a noise when trying to jump onto the
correct tag. Drop them.

Signed-off-by: Michal Privoznik 
---
 src/network/bridge_driver.c | 14 --
 1 file changed, 14 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 498c45d0a7..7e5fab630b 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -259,20 +259,6 @@ static int
 networkShutdownNetwork(virNetworkDriverState *driver,
virNetworkObj *obj);
 
-static int
-networkStartNetworkVirtual(virNetworkDriverState *driver,
-   virNetworkObj *obj);
-
-static int
-networkShutdownNetworkVirtual(virNetworkDriverState *driver,
-  virNetworkObj *obj);
-
-static int
-networkStartNetworkExternal(virNetworkObj *obj);
-
-static int
-networkShutdownNetworkExternal(virNetworkObj *obj);
-
 static void
 networkReloadFirewallRules(virNetworkDriverState *driver,
bool startup,
-- 
2.32.0

Re: [PATCH 1/1] bridge_driver: use ovs-vsctl to setup and clean Qos when

[Michal Prívozník]

On 11/3/21 4:11 AM, jx8zjs wrote:
> From: zhangjl02 
> 
> Fix bug 1826168: bridge type network with ovs bridge can start with Qos
> setting which do not take any effect
> 
> Resolves:https://bugzilla.redhat.com/show_bug.cgi?id=1826168
> Signed-off-by: zhangjl02 
> ---
>  src/network/bridge_driver.c | 65 +++--
>  1 file changed, 55 insertions(+), 10 deletions(-)
> 
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 498c45d0a7..d0627848cd 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -2305,6 +2305,15 @@ networkAddRouteToBridge(virNetworkObj *obj,
>  }
>  
>  
> +static int
> +networkDefIsOvsBridge(virNetworkDef *def)

This @def should have been const too. And function returns a bool,
effectively.

> +{
> +const virNetDevVPortProfile *vport = def->virtPortProfile;
> +return vport &&
> +vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH;
> +}
> +
> +
>  static int
>  networkStartNetworkVirtual(virNetworkDriverState *driver,
> virNetworkObj *obj)
> @@ -2320,6 +2329,7 @@ networkStartNetworkVirtual(virNetworkDriverState 
> *driver,
>  bool dnsmasqStarted = false;
>  bool devOnline = false;
>  bool firewalRulesAdded = false;
> +bool ovsType = networkDefIsOvsBridge(def);
>  
>  /* Check to see if any network IP collides with an existing route */
>  if (networkCheckRouteCollision(def) < 0)
> @@ -2439,15 +2449,29 @@ networkStartNetworkVirtual(virNetworkDriverState 
> *driver,
>  if (v6present && networkStartRadvd(driver, obj) < 0)
>  goto error;
>  
> -if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0)
> -goto error;
> +if (ovsType) {
> +if (virNetDevOpenvswitchInterfaceSetQos(def->bridge, def->bandwidth,
> +def->uuid,
> +true) < 0)
> +goto error;
> +} else {
> +if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 
> 0)
> +goto error;
> +}

I don't think this should be here. At this point, the bridge was created
using netlink (definitely NOT ovs-vsctl add-br). Therefore,
virNetDevOpenvswitchInterfaceSetQos() must fail because it won't be able
to find any ports/queues/...

>  
>  return 0;
>  
>   error:
>  virErrorPreserveLast(_err);
> -if (def->bandwidth)
> -   virNetDevBandwidthClear(def->bridge);
> +if (ovsType) {
> +if (virNetDevOpenvswitchInterfaceClearQos(def->bridge, def->uuid) < 
> 0)
> +VIR_WARN("cannot clear bandwidth setting for ovs bridge : %s",
> + def->bridge);
> +} else {
> +if (def->bandwidth) {
> +virNetDevBandwidthClear(def->bridge);
> +}
> +}
>  

Similarly, this shouldn't be here either.

>  if (dnsmasqStarted) {
>  pid_t dnsmasqPid = virNetworkObjGetDnsmasqPid(obj);
> @@ -2536,13 +2560,21 @@ static int
>  networkStartNetworkBridge(virNetworkObj *obj)
>  {
>  virNetworkDef *def = virNetworkObjGetDef(obj);
> +bool ovsType = networkDefIsOvsBridge(def);
>  
>  /* put anything here that needs to be done each time a network of
>   * type BRIDGE, is started. On failure, undo anything you've done,
>   * and return -1. On success return 0.
>   */
> -if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0)
> -goto error;
> +if (ovsType) {
> +if (virNetDevOpenvswitchInterfaceSetQos(def->bridge, def->bandwidth,
> +def->uuid,
> +true) < 0)
> +goto error;
> +} else {
> +if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 
> 0)
> +goto error;
> +}
>  
>  if (networkStartHandleMACTableManagerMode(obj) < 0)
>  goto error;
> @@ -2550,8 +2582,15 @@ networkStartNetworkBridge(virNetworkObj *obj)
>  return 0;
>  
>   error:
> -if (def->bandwidth)
> -   virNetDevBandwidthClear(def->bridge);
> +if (ovsType) {
> +if (virNetDevOpenvswitchInterfaceClearQos(def->bridge, def->uuid) < 
> 0)
> +VIR_WARN("cannot clear bandwidth setting for ovs bridge : %s",
> + def->bridge);
> +} else {
> +if (def->bandwidth) {
> +virNetDevBandwidthClear(def->bridge);
> +}
> +}
>  return -1;
>  }
>  
> @@ -2565,9 +2604,15 @@ networkShutdownNetworkBridge(virNetworkObj *obj 
> G_GNUC_UNUSED)
>   * type BRIDGE is shutdown. On failure, undo anything you've done,
>   * and return -1. On success return 0.
>   */
> -if (def->bandwidth)
> -   virNetDevBandwidthClear(def->bridge);
>  
> +if (networkDefIsOvsBridge(def)) {
> +if (virNetDevOpenvswitchInterfaceClearQos(def->bridge, def->uuid) < 
> 0)
> +

Re: [PATCH v2 0/4] Introduce TCG domain features

[Kashyap Chamarthy]

On Fri, Nov 05, 2021 at 10:35:16AM +0100, Michal Privoznik wrote:
> v2 of:
> 
> https://listman.redhat.com/archives/libvir-list/2021-November/msg00143.html
> 
> diff to v1:
> - Patches 1/4 and 2/4 are new, per Dan's suggestion
> - Patch 4/4 was reworked, since after 2/4 we are using -accel
>  unconditionally

I've built from your branch:
https://gitlab.com/MichalPrivoznik/libvirt.git / tcg_tb_size_alt_v2.

(I also needed the other patch when testing from your branch:
"remote_daemon: Validate tcp_min_ssf value only if found in config").

So compiled with:

   $> git describe
   v7.9.0-94-gc39356148c

Still works fine for me.  Two quick tests:

(1) With only , libvirt now uses `-accel tcg`
unconditionally.  (As opposed to `-machine accel`)

(2) And adding the `tb-cache` bit:

   
 
   65536
 
   

... results in QEMU command-line: `-accel tcg,tb-size=64`

Series:

Tested-by: Kashyap Chamarthy   

Thanks for the quick work!

-- 
/kashyap

Re: [PATCH v2 1/2] tests/acceptance: introduce new check-avocado tartget

[Philippe Mathieu-Daudé]

On 11/8/21 10:24, Daniel P. Berrangé wrote:
> On Mon, Nov 08, 2021 at 08:59:51AM +0100, Thomas Huth wrote:
>> On 05/11/2021 16.53, Willian Rampazzo wrote:
>>> This introduces a new `make` target, `check-avocado`, and adds a
>>> deprecation message about the `check-acceptance` target. This is
>>> a preparation for renaming the `tests/acceptance` folder to
>>>   `tests/avocado`.
>>>
>>> The plan is to remove the call to the `check-avocado` target one
>>> or two months after the release and leave the warning to force
>>> people to move to the new `check-avocado` target.
>>>
>>> Later, the `check-acceptance` target can be removed. The intent
>>> is to avoid a direct impact during the current soft freeze.
>>>
>>> Suggested-by: Philippe Mathieu-Daudé 
>>> Signed-off-by: Willian Rampazzo 
>>> ---
>>>   docs/about/deprecated.rst | 13 +
>>>   tests/Makefile.include| 17 -
>>>   2 files changed, 25 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
>>> index 56f9ad15ab..7bf8da8325 100644
>>> --- a/docs/about/deprecated.rst
>>> +++ b/docs/about/deprecated.rst
>>> @@ -410,3 +410,16 @@ nanoMIPS ISA
>>>   The ``nanoMIPS`` ISA has never been upstreamed to any compiler toolchain.
>>>   As it is hard to generate binaries for it, declare it deprecated.
>>> +
>>> +Testing
>>> +---
>>> +
>>> +Renaming of the acceptance folder to avocado
>>> +
>>> +
>>> +The ``tests/acceptance`` folder was never used to store acceptance tests
>>> +in terms of software engineering. This naming can confuse developers
>>> +adding tests using the Avocado Framework to this folder. The folder
>>> +name change to ``tests/avocado`` also changed the ``make`` target from
>>> +``check-acceptance`` to ``check-avocado``. In this case, the use of the
>>> +``check-acceptance`` target is deprecated.
>>
>> Not sure whether we need  to document this in deprecated.rst, too, since
>> we're normally only listing the things here that affect the users of the
>> qemu binaries, not the people who want to recompile and run the tests...
>> OTOH, I don't mind too much either if we list it here... Anybody else got an
>> opinion on this?
> 
> Deprecations are only things for user facing changes in the apps.

OK.

> For build system changes we don't bother with any deprecation cycle.
> Just make the change immediately and document it in the release notes.

Understood.

Willian, do you mind updating the release notes?
https://wiki.qemu.org/ChangeLog/6.2#Testing_and_CI

Re: [PATCH v2 1/2] tests/acceptance: introduce new check-avocado tartget

[Daniel P . Berrangé]

On Mon, Nov 08, 2021 at 08:59:51AM +0100, Thomas Huth wrote:
> On 05/11/2021 16.53, Willian Rampazzo wrote:
> > This introduces a new `make` target, `check-avocado`, and adds a
> > deprecation message about the `check-acceptance` target. This is
> > a preparation for renaming the `tests/acceptance` folder to
> >   `tests/avocado`.
> > 
> > The plan is to remove the call to the `check-avocado` target one
> > or two months after the release and leave the warning to force
> > people to move to the new `check-avocado` target.
> > 
> > Later, the `check-acceptance` target can be removed. The intent
> > is to avoid a direct impact during the current soft freeze.
> > 
> > Suggested-by: Philippe Mathieu-Daudé 
> > Signed-off-by: Willian Rampazzo 
> > ---
> >   docs/about/deprecated.rst | 13 +
> >   tests/Makefile.include| 17 -
> >   2 files changed, 25 insertions(+), 5 deletions(-)
> > 
> > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> > index 56f9ad15ab..7bf8da8325 100644
> > --- a/docs/about/deprecated.rst
> > +++ b/docs/about/deprecated.rst
> > @@ -410,3 +410,16 @@ nanoMIPS ISA
> >   The ``nanoMIPS`` ISA has never been upstreamed to any compiler toolchain.
> >   As it is hard to generate binaries for it, declare it deprecated.
> > +
> > +Testing
> > +---
> > +
> > +Renaming of the acceptance folder to avocado
> > +
> > +
> > +The ``tests/acceptance`` folder was never used to store acceptance tests
> > +in terms of software engineering. This naming can confuse developers
> > +adding tests using the Avocado Framework to this folder. The folder
> > +name change to ``tests/avocado`` also changed the ``make`` target from
> > +``check-acceptance`` to ``check-avocado``. In this case, the use of the
> > +``check-acceptance`` target is deprecated.
> 
> Not sure whether we need  to document this in deprecated.rst, too, since
> we're normally only listing the things here that affect the users of the
> qemu binaries, not the people who want to recompile and run the tests...
> OTOH, I don't mind too much either if we list it here... Anybody else got an
> opinion on this?

Deprecations are only things for user facing changes in the apps.

For build system changes we don't bother with any deprecation cycle.
Just make the change immediately and document it in the release notes.


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 :|

Re: [PATCH v2 1/2] tests/acceptance: introduce new check-avocado tartget

[Philippe Mathieu-Daudé]

On 11/8/21 08:59, Thomas Huth wrote:
> On 05/11/2021 16.53, Willian Rampazzo wrote:
>> This introduces a new `make` target, `check-avocado`, and adds a
>> deprecation message about the `check-acceptance` target. This is
>> a preparation for renaming the `tests/acceptance` folder to
>>   `tests/avocado`.
>>
>> The plan is to remove the call to the `check-avocado` target one
>> or two months after the release and leave the warning to force
>> people to move to the new `check-avocado` target.
>>
>> Later, the `check-acceptance` target can be removed. The intent
>> is to avoid a direct impact during the current soft freeze.
>>
>> Suggested-by: Philippe Mathieu-Daudé 
>> Signed-off-by: Willian Rampazzo 
>> ---
>>   docs/about/deprecated.rst | 13 +
>>   tests/Makefile.include    | 17 -
>>   2 files changed, 25 insertions(+), 5 deletions(-)
>>
>> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
>> index 56f9ad15ab..7bf8da8325 100644
>> --- a/docs/about/deprecated.rst
>> +++ b/docs/about/deprecated.rst
>> @@ -410,3 +410,16 @@ nanoMIPS ISA
>>     The ``nanoMIPS`` ISA has never been upstreamed to any compiler
>> toolchain.
>>   As it is hard to generate binaries for it, declare it deprecated.
>> +
>> +Testing
>> +---
>> +
>> +Renaming of the acceptance folder to avocado
>> +
>> +
>> +The ``tests/acceptance`` folder was never used to store acceptance tests
>> +in terms of software engineering. This naming can confuse developers
>> +adding tests using the Avocado Framework to this folder. The folder
>> +name change to ``tests/avocado`` also changed the ``make`` target from
>> +``check-acceptance`` to ``check-avocado``. In this case, the use of the
>> +``check-acceptance`` target is deprecated.
> 
> Not sure whether we need  to document this in deprecated.rst, too, since
> we're normally only listing the things here that affect the users of the
> qemu binaries, not the people who want to recompile and run the tests...
> OTOH, I don't mind too much either if we list it here... Anybody else
> got an opinion on this?

Hmm OK my bad, I asked Willian to add that without noticing this file
is for "only things that affect the users".

Willian, if you agree with Thomas, I can remove this change from your
patch.

Re: [PATCH v2 1/2] tests/acceptance: introduce new check-avocado tartget

[Thomas Huth]

On 05/11/2021 16.53, Willian Rampazzo wrote:

This introduces a new `make` target, `check-avocado`, and adds a
deprecation message about the `check-acceptance` target. This is
a preparation for renaming the `tests/acceptance` folder to
  `tests/avocado`.

The plan is to remove the call to the `check-avocado` target one
or two months after the release and leave the warning to force
people to move to the new `check-avocado` target.

Later, the `check-acceptance` target can be removed. The intent
is to avoid a direct impact during the current soft freeze.

Suggested-by: Philippe Mathieu-Daudé 
Signed-off-by: Willian Rampazzo 
---
  docs/about/deprecated.rst | 13 +
  tests/Makefile.include| 17 -
  2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 56f9ad15ab..7bf8da8325 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -410,3 +410,16 @@ nanoMIPS ISA
  
  The ``nanoMIPS`` ISA has never been upstreamed to any compiler toolchain.

  As it is hard to generate binaries for it, declare it deprecated.
+
+Testing
+---
+
+Renaming of the acceptance folder to avocado
+
+
+The ``tests/acceptance`` folder was never used to store acceptance tests
+in terms of software engineering. This naming can confuse developers
+adding tests using the Avocado Framework to this folder. The folder
+name change to ``tests/avocado`` also changed the ``make`` target from
+``check-acceptance`` to ``check-avocado``. In this case, the use of the
+``check-acceptance`` target is deprecated.


Not sure whether we need  to document this in deprecated.rst, too, since 
we're normally only listing the things here that affect the users of the 
qemu binaries, not the people who want to recompile and run the tests... 
OTOH, I don't mind too much either if we list it here... Anybody else got an 
opinion on this?



diff --git a/tests/Makefile.include b/tests/Makefile.include
index 8434a33fe6..8e8ee58493 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -16,7 +16,7 @@ ifneq ($(filter $(all-check-targets), check-softfloat),)
@echo " $(MAKE) check-tcgRun TCG tests"
@echo " $(MAKE) check-softfloat  Run FPU emulation tests"
  endif
-   @echo " $(MAKE) check-acceptance Run acceptance (functional) tests for 
currently configured targets"
+   @echo " $(MAKE) check-avocadoRun avocado (integration) tests for 
currently configured targets"
@echo
@echo " $(MAKE) check-report.tap Generates an aggregated TAP test 
report"
@echo " $(MAKE) check-venv   Creates a Python venv for tests"
@@ -24,7 +24,7 @@ endif
@echo
@echo "The following are useful for CI builds"
@echo " $(MAKE) check-build  Build most test binaris"
-   @echo " $(MAKE) get-vm-imagesDownloads all images used by acceptance 
tests, according to configured targets (~350 MB each, 1.5 GB max)"
+   @echo " $(MAKE) get-vm-imagesDownloads all images used by avocado 
tests, according to configured targets (~350 MB each, 1.5 GB max)"
@echo
@echo
@echo "The variable SPEED can be set to control the gtester speed 
setting."
@@ -83,7 +83,7 @@ clean-tcg: $(CLEAN_TCG_TARGET_RULES)
  
  # Python venv for running tests
  
-.PHONY: check-venv check-acceptance

+.PHONY: check-venv check-avocado check-acceptance 
check-acceptance-deprecated-warning
  
  TESTS_VENV_DIR=$(BUILD_DIR)/tests/venv

  TESTS_VENV_REQ=$(SRC_PATH)/tests/requirements.txt
@@ -127,12 +127,12 @@ get-vm-image-fedora-31-%: check-venv
$(call quiet-command, \
   $(TESTS_VENV_DIR)/bin/python -m avocado vmimage get \
   --distro=fedora --distro-version=31 --arch=$*, \
-   "AVOCADO", "Downloading acceptance tests VM image for $*")
+   "AVOCADO", "Downloading avocado tests VM image for $*")
  
  # download all vm images, according to defined targets

  get-vm-images: check-venv $(patsubst %,get-vm-image-fedora-31-%, 
$(FEDORA_31_DOWNLOAD))
  
-check-acceptance: check-venv $(TESTS_RESULTS_DIR) get-vm-images

+check-avocado: check-venv $(TESTS_RESULTS_DIR) get-vm-images
$(call quiet-command, \
  $(TESTS_VENV_DIR)/bin/python -m avocado \
  --show=$(AVOCADO_SHOW) run --job-results-dir=$(TESTS_RESULTS_DIR) 
\
@@ -142,6 +142,13 @@ check-acceptance: check-venv $(TESTS_RESULTS_DIR) 
get-vm-images
  $(if $(GITLAB_CI),,--failfast) $(AVOCADO_TESTS), \
  "AVOCADO", "tests/acceptance")
  
+check-acceptance-deprecated-warning:

+   @echo
+   @echo "Note '$(MAKE) check-acceptance' is deprecated, use '$(MAKE) 
check-avocado' instead."
+   @echo
+
+check-acceptance: check-acceptance-deprecated-warning | check-avocado
+
  # Consolidated targets
  
  .PHONY: check-block check check-clean get-vm-images




Acked-by: