Re: [libvirt] [PATCH v10 1/4] domifaddr: Implement the public APIs
On Mon, Mar 16, 2015 at 08:42:18AM -0400, John Ferlan wrote:
On 03/11/2015 07:09 AM, Daniel P. Berrange wrote:
> From: Nehal J Wani <nehaljw.kkd1(a)gmail.com>
>
> Define helper function virDomainInterfaceFree, which allows
> the upper layer application to free the domain interface object
> conveniently.
>
> The API is going to provide multiple methods by flags, e.g.
> * Query guest agent
> * Parse DHCP lease file
>
> include/libvirt/libvirt-domain.h
> * Define virDomainInterfaceAddresses, virDomainInterfaceFree
> * Define structs virDomainInterface, virDomainIPAddress
>
> src/driver-hypervisor.h:
> * Define domainInterfaceAddresses
>
> src/libvirt-domain.c:
> * Implement virDomainInterfaceAddresses
> * Implement virDomainInterfaceFree
>
> src/libvirt_public.syms:
> * Export the new symbols
>
> Signed-off-by: Nehal J Wani <nehaljw.kkd1(a)gmail.com>
> ---
> include/libvirt/libvirt-domain.h | 32 ++++++++++
> src/driver-hypervisor.h | 6 ++
> src/libvirt-domain.c | 123 +++++++++++++++++++++++++++++++++++++++
> src/libvirt_public.syms | 2 +
> 4 files changed, 163 insertions(+)
>
> + * for (i = 0; i < ifaces_count; i++)
> + * virDomainInterfaceFree(ifaces[i]);
> + * free(ifaces);
> + *
> + * Returns the number of interfaces on success, -1 in case of error.
> + */
> +int
> +virDomainInterfaceAddresses(virDomainPtr dom,
> + virDomainInterfacePtr **ifaces,
> + unsigned int source,
> + unsigned int flags)
> +{
> + virConnectPtr conn;
> +
> + VIR_DOMAIN_DEBUG(dom, "ifaces=%p, source=%d, flags=%x", ifaces,
source, flags);
> +
> + virResetLastError();
> +
> + conn = dom->conn;
If 'dom' is NULL, then there's a pointer deref (considering check below)
> +
> + if (ifaces)
> + *ifaces = NULL;
> + virCheckDomainReturn(dom, -1);
> + virCheckNonNullArgGoto(ifaces, error);
> + virCheckReadOnlyGoto(conn->flags, error);
I think I ran into this when I was adding the libvirt-perl code - since
this is a get interface - does it really matter if it's readonly?
In this case it matters because we need to block access to the QEMU
guest agent.
Also, I'd move both conn = dom->conn and "if (ifaces) *ifaces = NULL;"
to here
I'll just delete the 'conn' var - it is fairly pointless and just causes
problems.
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 :|