[libvirt] [PATCH v2 0/4] qemu/virsh/docs: various minor fixes
From: Lin Ma <lma@suse.de> v1 -> v2: * Remove the merged patches. * Follow Ján and Erik's suggestions. Lin Ma (4): virsh: net-port-create: log errors for non-existent xml file virsh: event: options --list, --all and --event are mutually exclusive docs: virsh: Document the IO mode 'io_uring' network: Check for active network during networkGetDHCPLeases docs/manpages/virsh.rst | 3 ++- src/network/bridge_driver.c | 7 +++++++ tools/virsh-domain.c | 4 ++++ tools/virsh-network.c | 1 + 4 files changed, 14 insertions(+), 1 deletion(-) -- 2.26.0
From: Lin Ma <lma@suse.de> Signed-off-by: Lin Ma <lma@suse.de> --- tools/virsh-network.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/virsh-network.c b/tools/virsh-network.c index 0351a93f19..24bc9107fc 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -1523,6 +1523,7 @@ cmdNetworkPortCreate(vshControl *ctl, const vshCmd *cmd) ret = true; cleanup: + vshSaveLibvirtError(); VIR_FREE(buffer); if (port) virNetworkPortFree(port); -- 2.26.0
On 9/16/20 9:17 AM, morecache@gmail.com wrote:
From: Lin Ma <lma@suse.de>
Signed-off-by: Lin Ma <lma@suse.de> --- tools/virsh-network.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/tools/virsh-network.c b/tools/virsh-network.c index 0351a93f19..24bc9107fc 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -1523,6 +1523,7 @@ cmdNetworkPortCreate(vshControl *ctl, const vshCmd *cmd)
ret = true; cleanup: + vshSaveLibvirtError();
I believe this must go under virFileReadAll(): if (virFileReadAll(from, VSH_MAX_XML_FILE, &buffer) < 0) { vshSaveLibvirtError(); goto cleanup; } We don't want to be reporting libvirt error every time we reach the label, because not all jumps onto that label are direct result of a failing function that uses virReportError(). Michal
From: Lin Ma <lma@suse.de> Signed-off-by: Lin Ma <lma@suse.de> --- tools/virsh-domain.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index f0ca6f7b83..32edfc0398 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -13710,6 +13710,10 @@ cmdEvent(vshControl *ctl, const vshCmd *cmd) int count = 0; virshControlPtr priv = ctl->privData; + VSH_EXCLUSIVE_OPTIONS("all", "event"); + VSH_EXCLUSIVE_OPTIONS("list", "all"); + VSH_EXCLUSIVE_OPTIONS("list", "event"); + if (vshCommandOptBool(cmd, "list")) { for (event = 0; event < VIR_DOMAIN_EVENT_ID_LAST; event++) vshPrint(ctl, "%s\n", virshDomainEventCallbacks[event].name); -- 2.26.0
From: Lin Ma <lma@suse.de> Signed-off-by: Lin Ma <lma@suse.de> --- docs/manpages/virsh.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 9fd5efed0f..bf84f95678 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -4521,7 +4521,8 @@ within the existing virtual cdrom or floppy device; consider using *sourcetype* can indicate the type of source (block|file) *cache* can be one of "default", "none", "writethrough", "writeback", "directsync" or "unsafe". -*io* controls specific policies on I/O; QEMU guests support "threads" and "native". +*io* controls specific policies on I/O; QEMU guests support "threads", +"native" and "io_uring". *iothread* is the number within the range of domain IOThreads to which this disk may be attached (QEMU only). *serial* is the serial of disk device. *wwn* is the wwn of disk device. -- 2.26.0
From: Lin Ma <lma@suse.de> It doesn't make sense querying dhcp leases for interfaces against an inactive network, This patch adds a check to see if the network is active. Signed-off-by: Lin Ma <lma@suse.de> --- src/network/bridge_driver.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 87d7acab06..1dffc2309f 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4068,6 +4068,13 @@ networkGetDHCPLeases(virNetworkPtr net, if (virNetworkGetDHCPLeasesEnsureACL(net->conn, def) < 0) goto cleanup; + if (!virNetworkObjIsActive(obj)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("network '%s' is not active"), + def->name); + goto error; + } + /* Retrieve custom leases file location */ custom_lease_file = networkDnsmasqLeaseFileNameCustom(driver, def->bridge); -- 2.26.0
On 9/16/20 9:17 AM, morecache@gmail.com wrote:
From: Lin Ma <lma@suse.de>
It doesn't make sense querying dhcp leases for interfaces against an inactive network, This patch adds a check to see if the network is active.
Signed-off-by: Lin Ma <lma@suse.de> --- src/network/bridge_driver.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 87d7acab06..1dffc2309f 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4068,6 +4068,13 @@ networkGetDHCPLeases(virNetworkPtr net, if (virNetworkGetDHCPLeasesEnsureACL(net->conn, def) < 0) goto cleanup;
+ if (!virNetworkObjIsActive(obj)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("network '%s' is not active"), + def->name); + goto error;
No need to jump all the way to 'error' when 'cleanup' is just fine. @leases_ret wasn't touched until now and hence is still NULL, this 'error' is the same as 'cleanup'. But you can keep it, if you want, we're jumping "randomly" on error and cleanup. Michal
On a Wednesday in 2020, Michal Privoznik wrote:
On 9/16/20 9:17 AM, morecache@gmail.com wrote:
From: Lin Ma <lma@suse.de>
It doesn't make sense querying dhcp leases for interfaces against an inactive network, This patch adds a check to see if the network is active.
NACK, see discussion on v1. https://www.redhat.com/archives/libvir-list/2020-September/msg00673.html Jano
Signed-off-by: Lin Ma <lma@suse.de> --- src/network/bridge_driver.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 87d7acab06..1dffc2309f 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4068,6 +4068,13 @@ networkGetDHCPLeases(virNetworkPtr net, if (virNetworkGetDHCPLeasesEnsureACL(net->conn, def) < 0) goto cleanup; + if (!virNetworkObjIsActive(obj)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("network '%s' is not active"), + def->name); + goto error;
No need to jump all the way to 'error' when 'cleanup' is just fine. @leases_ret wasn't touched until now and hence is still NULL, this 'error' is the same as 'cleanup'. But you can keep it, if you want, we're jumping "randomly" on error and cleanup.
Michal
On 9/16/20 2:18 PM, Ján Tomko wrote:
On a Wednesday in 2020, Michal Privoznik wrote:
On 9/16/20 9:17 AM, morecache@gmail.com wrote:
From: Lin Ma <lma@suse.de>
It doesn't make sense querying dhcp leases for interfaces against an inactive network, This patch adds a check to see if the network is active.
NACK, see discussion on v1.
https://www.redhat.com/archives/libvir-list/2020-September/msg00673.html
So what if I set up a static lease in network XM, start the network, then shut it down and change the XML? Wouldn't I get a different lease until the network is started again? The NSS plugin suffers from the same problem, btw. But it can't know whether the network is still running or not (if we don't want it to do some hacks). Michal
On 9/16/20 9:17 AM, morecache@gmail.com wrote:
From: Lin Ma <lma@suse.de>
v1 -> v2:
* Remove the merged patches. * Follow Ján and Erik's suggestions.
Lin Ma (4): virsh: net-port-create: log errors for non-existent xml file virsh: event: options --list, --all and --event are mutually exclusive docs: virsh: Document the IO mode 'io_uring' network: Check for active network during networkGetDHCPLeases
docs/manpages/virsh.rst | 3 ++- src/network/bridge_driver.c | 7 +++++++ tools/virsh-domain.c | 4 ++++ tools/virsh-network.c | 1 + 4 files changed, 14 insertions(+), 1 deletion(-)
Patches looks good to me, but see my comments. I can do the changes before pushing, if you agree with suggested changes. Michal
On 2020-09-16 10:40, Michal Privoznik wrote:
On 9/16/20 9:17 AM, morecache@gmail.com wrote:
From: Lin Ma <lma@suse.de>
v1 -> v2:
* Remove the merged patches. * Follow Ján and Erik's suggestions.
Lin Ma (4): virsh: net-port-create: log errors for non-existent xml file virsh: event: options --list, --all and --event are mutually exclusive docs: virsh: Document the IO mode 'io_uring' network: Check for active network during networkGetDHCPLeases
docs/manpages/virsh.rst | 3 ++- src/network/bridge_driver.c | 7 +++++++ tools/virsh-domain.c | 4 ++++ tools/virsh-network.c | 1 + 4 files changed, 14 insertions(+), 1 deletion(-)
Patches looks good to me, but see my comments. I can do the changes before pushing, if you agree with suggested changes.
No problem, Thank you very much. Lin
On 9/17/20 5:20 AM, Lin Ma wrote:
On 2020-09-16 10:40, Michal Privoznik wrote:
On 9/16/20 9:17 AM, morecache@gmail.com wrote:
From: Lin Ma <lma@suse.de>
v1 -> v2:
* Remove the merged patches. * Follow Ján and Erik's suggestions.
Lin Ma (4): virsh: net-port-create: log errors for non-existent xml file virsh: event: options --list, --all and --event are mutually exclusive docs: virsh: Document the IO mode 'io_uring' network: Check for active network during networkGetDHCPLeases
docs/manpages/virsh.rst | 3 ++- src/network/bridge_driver.c | 7 +++++++ tools/virsh-domain.c | 4 ++++ tools/virsh-network.c | 1 + 4 files changed, 14 insertions(+), 1 deletion(-)
Patches looks good to me, but see my comments. I can do the changes before pushing, if you agree with suggested changes.
No problem, Thank you very much.
Since Jano didn't like 4/4, I'm pushing 1-3/4 only. Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (4)
-
Ján Tomko -
Lin Ma -
Michal Privoznik -
morecache＠gmail.com