Re: [libvirt] [PATCH] Allow address lease query with inactive domain

[Amador Pahim]

On Thu, Sep 8, 2016 at 2:46 AM, Laine Stump  wrote:
> On 09/07/2016 05:57 AM, Amador Pahim wrote:
>>
>> On Wed, Sep 7, 2016 at 11:41 AM, Daniel P. Berrange 
>> wrote:
>>>
>>> On Wed, Sep 07, 2016 at 11:18:58AM +0200, Amador Pahim wrote:

 The DomainInterfaceAddresses, when using the source type
 LEASE, does not need the domain to be running. It only checks the
 dhcp lease file and gets the address information from a valid lease,
 if any.

 This patch removes the virDomainObjIsActive(vm) check from the LEASE
 query type on both qemu and lxc drivers, keeping it only for query
 type AGENT on qemu driver (since lxc does not support AGENT type).
>>>
>>> I don't think we should do this. IMHO it only makes sense to ask for
>>> the IP address when the guest is actually running. The fact that we
>>> might still happen to have an old IP address stored in the lease
>>> file is merely a happy accident and not something we should guarantee
>>> by exposing ability to get it in the API.
>>
>> The fact that the guest is running does not mean the lease information is
>> accurate. Guest IP can be manually changed. IP can be allocated by
>> someone else between the true virDomainObjIsActive(vm) and the actual
>> DHCPACK. We don't guarantee the information in any case, unless you
>> change the query type to AGENT. One can parse the lease file by itself
>> (and check if the lease is not expired), it's cheaper than create(), query
>> the lease addresses, destroy(). This patch tries to avoid both.
>
>
> (A short explanation of your use case might help us to understand why you
> would want to learn the former IP address of a non-running guest.)
>
> When the guest is running, of course the information in the lease file might
> be incorrect, but that would most likely be caused by a malicious or
> malfunctioning guest. In general though, the lease information probably is
> correct, and the IP address you get back could be used to contact the guest
> (from the host, at least, and assuming necessary ports were open in guest
> and host firewalls, and that the guest was listening on the appropriate
> port).
>
> If the guest *isn't* running though, then by definition the lease
> information is incorrect - you definitely aren't going to be able to reach
> the guest via that address, so it's a bit misleading for libvirt to suggest
> it.

Ok, thank you both for the feedback.

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

Re: [libvirt] [PATCH] Allow address lease query with inactive domain

[Laine Stump]

On 09/07/2016 05:57 AM, Amador Pahim wrote:

On Wed, Sep 7, 2016 at 11:41 AM, Daniel P. Berrange  wrote:

On Wed, Sep 07, 2016 at 11:18:58AM +0200, Amador Pahim wrote:

The DomainInterfaceAddresses, when using the source type
LEASE, does not need the domain to be running. It only checks the
dhcp lease file and gets the address information from a valid lease,
if any.

This patch removes the virDomainObjIsActive(vm) check from the LEASE
query type on both qemu and lxc drivers, keeping it only for query
type AGENT on qemu driver (since lxc does not support AGENT type).

I don't think we should do this. IMHO it only makes sense to ask for
the IP address when the guest is actually running. The fact that we
might still happen to have an old IP address stored in the lease
file is merely a happy accident and not something we should guarantee
by exposing ability to get it in the API.

The fact that the guest is running does not mean the lease information is
accurate. Guest IP can be manually changed. IP can be allocated by
someone else between the true virDomainObjIsActive(vm) and the actual
DHCPACK. We don't guarantee the information in any case, unless you
change the query type to AGENT. One can parse the lease file by itself
(and check if the lease is not expired), it's cheaper than create(), query
the lease addresses, destroy(). This patch tries to avoid both.


(A short explanation of your use case might help us to understand why 
you would want to learn the former IP address of a non-running guest.)


When the guest is running, of course the information in the lease file 
might be incorrect, but that would most likely be caused by a malicious 
or malfunctioning guest. In general though, the lease information 
probably is correct, and the IP address you get back could be used to 
contact the guest (from the host, at least, and assuming necessary ports 
were open in guest and host firewalls, and that the guest was listening 
on the appropriate port).


If the guest *isn't* running though, then by definition the lease 
information is incorrect - you definitely aren't going to be able to 
reach the guest via that address, so it's a bit misleading for libvirt 
to suggest it.


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

Re: [libvirt] [PATCH] Allow address lease query with inactive domain

[Amador Pahim]

On Wed, Sep 7, 2016 at 11:41 AM, Daniel P. Berrange  wrote:
> On Wed, Sep 07, 2016 at 11:18:58AM +0200, Amador Pahim wrote:
>> The DomainInterfaceAddresses, when using the source type
>> LEASE, does not need the domain to be running. It only checks the
>> dhcp lease file and gets the address information from a valid lease,
>> if any.
>>
>> This patch removes the virDomainObjIsActive(vm) check from the LEASE
>> query type on both qemu and lxc drivers, keeping it only for query
>> type AGENT on qemu driver (since lxc does not support AGENT type).
>
> I don't think we should do this. IMHO it only makes sense to ask for
> the IP address when the guest is actually running. The fact that we
> might still happen to have an old IP address stored in the lease
> file is merely a happy accident and not something we should guarantee
> by exposing ability to get it in the API.

The fact that the guest is running does not mean the lease information is
accurate. Guest IP can be manually changed. IP can be allocated by
someone else between the true virDomainObjIsActive(vm) and the actual
DHCPACK. We don't guarantee the information in any case, unless you
change the query type to AGENT. One can parse the lease file by itself
(and check if the lease is not expired), it's cheaper than create(), query
the lease addresses, destroy(). This patch tries to avoid both.

>
>
> Regards,
> Daniel
> --
> |: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org  -o- http://virt-manager.org :|
> |: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|



-- 
Pahim

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

Re: [libvirt] [PATCH] Allow address lease query with inactive domain

[Daniel P. Berrange]

On Wed, Sep 07, 2016 at 11:18:58AM +0200, Amador Pahim wrote:
> The DomainInterfaceAddresses, when using the source type
> LEASE, does not need the domain to be running. It only checks the
> dhcp lease file and gets the address information from a valid lease,
> if any.
> 
> This patch removes the virDomainObjIsActive(vm) check from the LEASE
> query type on both qemu and lxc drivers, keeping it only for query
> type AGENT on qemu driver (since lxc does not support AGENT type).

I don't think we should do this. IMHO it only makes sense to ask for
the IP address when the guest is actually running. The fact that we
might still happen to have an old IP address stored in the lease
file is merely a happy accident and not something we should guarantee
by exposing ability to get it in the API.


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

[libvirt] [PATCH] Allow address lease query with inactive domain

[Amador Pahim]

The DomainInterfaceAddresses, when using the source type
LEASE, does not need the domain to be running. It only checks the
dhcp lease file and gets the address information from a valid lease,
if any.

This patch removes the virDomainObjIsActive(vm) check from the LEASE
query type on both qemu and lxc drivers, keeping it only for query
type AGENT on qemu driver (since lxc does not support AGENT type).

Signed-off-by: Amador Pahim 
---
 src/libxl/libxl_driver.c |  6 --
 src/qemu/qemu_driver.c   | 12 ++--
 2 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 480fef9..9f8c36a 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -6251,12 +6251,6 @@ libxlDomainInterfaceAddresses(virDomainPtr dom,
 if (virDomainInterfaceAddressesEnsureACL(dom->conn, vm->def) < 0)
 goto cleanup;
 
-if (!virDomainObjIsActive(vm)) {
-virReportError(VIR_ERR_OPERATION_INVALID, "%s",
-   _("domain is not running"));
-goto cleanup;
-}
-
 switch (source) {
 case VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LEASE:
 ret = libxlGetDHCPInterfaces(dom, vm, ifaces);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 807e06d..a8dfa2f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -19600,18 +19600,18 @@ qemuDomainInterfaceAddresses(virDomainPtr dom,
 if (virDomainInterfaceAddressesEnsureACL(dom->conn, vm->def) < 0)
 goto cleanup;
 
-if (!virDomainObjIsActive(vm)) {
-virReportError(VIR_ERR_OPERATION_INVALID, "%s",
-   _("domain is not running"));
-goto cleanup;
-}
-
 switch (source) {
 case VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LEASE:
 ret = qemuGetDHCPInterfaces(dom, vm, ifaces);
 break;
 
 case VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT:
+if (!virDomainObjIsActive(vm)) {
+virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+   _("domain is not running"));
+goto cleanup;
+}
+
 if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
 goto cleanup;
 
-- 
2.7.4

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