[v1] qemu/virsh/docs: various minor fixes

[libvirt] [PATCH 03/12] network: Check for active network during networkGetDHCPLeases

Posted by Lin Ma 5 years, 5 months ago

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

Re: [libvirt] [PATCH 03/12] network: Check for active network during networkGetDHCPLeases

Posted by Erik Skultety 5 years, 5 months ago

On Fri, Sep 11, 2020 at 03:06:10PM +0800, Lin Ma wrote:
> 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>
> ---
Reviewed-by: Erik Skultety <eskultet@redhat.com>

Re: [libvirt] [PATCH 03/12] network: Check for active network during networkGetDHCPLeases

Posted by Ján Tomko 5 years, 5 months ago

On a Friday in 2020, Lin Ma wrote:
>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.
>

Why would the network need to be active? the leases are still there:

$ virsh net-destroy default
Network default destroyed

$ virsh net-dhcp-leases default
  Expiry Time           MAC address         Protocol   IP address           Hostname   Client ID or DUID
------------------------------------------------------------------------------------------------------------
  2020-09-11 16:16:59   52:54:00:55:7c:df   ipv4       192.168.122.183/24   -          01:52:54:00:55:7c:df

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;
>+    }
>+
>     /* Retrieve custom leases file location */
>     custom_lease_file = networkDnsmasqLeaseFileNameCustom(driver, def->bridge);
>
>-- 
>2.26.0
>

Re: [libvirt] [PATCH 03/12] network: Check for active network during networkGetDHCPLeases

Posted by Lin Ma 5 years, 4 months ago

On 2020-09-11 13:22, Ján Tomko wrote:
> On a Friday in 2020, Lin Ma wrote:
>> 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.
>> 
> 
> Why would the network need to be active? the leases are still there:
> 
> $ virsh net-destroy default
> Network default destroyed
> 
> $ virsh net-dhcp-leases default
>  Expiry Time           MAC address         Protocol   IP address
>     Hostname   Client ID or DUID
> ------------------------------------------------------------------------------------------------------------
>  2020-09-11 16:16:59   52:54:00:55:7c:df   ipv4
> 192.168.122.183/24   -          01:52:54:00:55:7c:df
> 

IMO the conception of leases doesn't make sense in case of no dhcp 
server daemon running.
I'll carry this patch in V2 patch set, Please feel free to decline it.

Thanks,
Lin

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

Re: [libvirt] [PATCH 03/12] network: Check for active network during networkGetDHCPLeases

Posted by Ján Tomko 5 years, 4 months ago

On a Wednesday in 2020, Lin Ma wrote:
>On 2020-09-11 13:22, Ján Tomko wrote:
>>On a Friday in 2020, Lin Ma wrote:
>>>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.
>>>
>>
>>Why would the network need to be active? the leases are still there:
>>
>>$ virsh net-destroy default
>>Network default destroyed
>>
>>$ virsh net-dhcp-leases default
>> Expiry Time           MAC address         Protocol   IP address
>>    Hostname   Client ID or DUID
>>------------------------------------------------------------------------------------------------------------
>> 2020-09-11 16:16:59   52:54:00:55:7c:df   ipv4
>>192.168.122.183/24   -          01:52:54:00:55:7c:df
>>
>
>IMO the conception of leases doesn't make sense in case of no dhcp 
>server daemon running.

The leases are still there and will be picked up by the DHCP server
after it's started again. We do have access to the leases even if
the server is not running. Why would we deny querying it?

Jano

>I'll carry this patch in V2 patch set, Please feel free to decline it.
>
>Thanks,
>Lin
>
>>>---
>>>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
>>>
>
>