Re: [PATCH v10 1/4] domifaddr: Implement the public APIs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]