[libvirt] [PATCH] For comment/critique only - netcf backend for virInterface*.
Daniel P. Berrange
berrange at redhat.com
Fri Jun 26 10:20:18 UTC 2009
On Tue, Jun 23, 2009 at 02:08:29PM -0400, Laine Stump wrote:
> This is the backend for the interface driver (virInterface*()) that
> uses netcf. There are a few issues with it:
>
> 1) It doesn't yet implement the backend for
> virConnectListDefinedInterfaces() and
> virConnectNumOfDefinedInterfaces() (although it does use my patched
> netcf API to list only active interfaces for
> virConnectListInterfaces()). (That patch isn't committed to netcf
> yet, btw; it's functional, but a bit rough and hackish)
>
> 2) interfaceLookupByMACString() still uses the old arg convention for
> ncf_lookup_by_mac_string(), which returned a single interface. That
> function has been changed to potentially return multiple
> interfaces. To fully support that, we would need to change the
> libvirt API. What should we do here?
We can't change libvirt API once released, so we'll need to decide on
a mapping for it, my prefernce is to restrict the types of interfaces
that can be returned from it, eg only standalone physical NICs, or
bond devices. ie no vlans, bridges, aliases, etc
> 3) The XML in interfaceDefineXML() isn't validated, although the spot to
> do it is clearly marked.
That's noy the only method - we need to hook in the XML parser and
formatter at every place XML goes in or out of the libvirt API.
>
> 4) There's no comment in the place where we could, in the future, transform
> the XML returned to interfaceGetXMLDesc() by netcf. Currently the two
> use the same RNG. Since it would be a null transform, and that will only
> change under our control, that isn't an *immediate* problem, but shouldn't
> be forgotten.
>
> Parts that I'm unsure of:
>
> 1) changes to the Makefile, particularly surrounding "DRIVER_MODULES".
Looks correct
> 2) Is it registered in the proper places?
Yes
>
> 3) Could it (should it?) be added to other hypervisor drivers?
> (Currently I only put it in qemud.c)
That makes it available to all drivers. Ignore the mis-leading filename :-)
> diff --git a/src/interface_driver.c b/src/interface_driver.c
> new file mode 100644
> index 0000000..b6cf510
> --- /dev/null
> +++ b/src/interface_driver.c
> @@ -0,0 +1,381 @@
> +/*
> + * interface_driver.c: backend driver methods to handle physical
> + * interface configuration using the netcf library.
> + *
> + * Copyright (C) 2006-2009 Red Hat, Inc.
> + * Copyright (C) 2006 Daniel P. Berrange
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + * Author: Daniel P. Berrange <berrange at redhat.com>
> + */
You'll want to update the credits here :-)
> +
> +#include <config.h>
> +
> +#include <sys/types.h>
> +#include <sys/poll.h>
> +#include <dirent.h>
> +#include <limits.h>
> +#include <string.h>
> +#include <stdio.h>
> +#include <strings.h>
> +#include <stdarg.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <errno.h>
> +#include <sys/utsname.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +#include <signal.h>
> +#include <paths.h>
> +#include <pwd.h>
> +#include <stdio.h>
> +#include <sys/wait.h>
> +#include <sys/ioctl.h>
> +#include <netcf.h>
You can probably remove alot of those headers.
> +
> +/* Main driver state */
> +struct interface_driver
> +{
> + virMutex lock;
> + struct netcf *netcf;
> +};
> +
> +
> +static void interfaceDriverLock(struct interface_driver *driver)
> +{
> + virMutexLock(&driver->lock);
> +}
> +
> +static void interfaceDriverUnlock(struct interface_driver *driver)
> +{
> + virMutexUnlock(&driver->lock);
> +}
> +
> +
> +static struct interface_driver *driverState = NULL;
> +
> +static virDrvOpenStatus interfaceOpenInterface(virConnectPtr conn,
> + virConnectAuthPtr auth ATTRIBUTE_UNUSED,
> + int flags ATTRIBUTE_UNUSED)
> +{
> + if (VIR_ALLOC(driverState) < 0)
> + {
> + virReportOOMError(conn);
> + goto alloc_error;
> + }
This is wrong. You can't alloc & free global state, in a per-virConnectPtr
object. Either the state has to be private to each virConnectPtr, or
global and initialized in a virStateDriverPtr initializer (which is run
at driver startup, instead of connection open)
The choice really depends on how much overhead there is on each of the
ncf_init() calls. If its low, then it is fine to have it per-virConnectPtr
> +
> + /* initialize non-0 stuff in driverState */
> + if (virMutexInit(&driverState->lock) < 0)
> + {
> + /* what error to report? */
> + goto mutex_error;
> + }
> +
> + /* open netcf */
> + if (ncf_init(&driverState->netcf, NULL) != 0)
> + {
> + /* what error to report? */
> + goto netcf_error;
> + }
> +
> + conn->interfacePrivateData = driverState;
> + return 0;
> +
> +netcf_error:
> + if (driverState->netcf)
> + {
> + ncf_close(driverState->netcf);
> + }
> + virMutexDestroy (&driverState->lock);
> +mutex_error:
> + VIR_FREE(driverState);
> +alloc_error:
> + return -1;
> +}
> +
> +static int interfaceCloseInterface(virConnectPtr conn)
> +{
> +
> + if (conn->interfacePrivateData != NULL)
> + {
> + struct interface_driver *driver
> + = (struct interface_driver *)conn->interfacePrivateData;
Type casts aren't needed if the source variable is void *.
> +
> + /* close netcf instance */
> + ncf_close(driver->netcf);
> + /* destroy lock */
> + virMutexDestroy(&driver->lock);
> + /* free driverState */
> + VIR_FREE(driver);
> + }
> + conn->interfacePrivateData = NULL;
> + return 0;
> +}
> +
> +static int interfaceNumInterfaces(virConnectPtr conn)
> +{
> + int count;
> + struct interface_driver *driver = conn->interfacePrivateData;
> +
> + interfaceDriverLock(driver);
> + count = ncf_num_of_interfaces(driver->netcf, NETCF_FLAG_ACTIVE);
> + interfaceDriverUnlock(driver);
> + return count;
> +}
Need to report an error here if ncf_num_of_interfaces fails
> +static int interfaceListInterfaces(virConnectPtr conn, char **const names, int nnames)
> +{
> + struct interface_driver *driver = conn->interfacePrivateData;
> + int count;
> +
> + interfaceDriverLock(driver);
> +
> + count = ncf_list_interfaces(driver->netcf, nnames, names, NETCF_FLAG_ACTIVE);
> + /* netcf cleans up in the case of errors */
> + interfaceDriverUnlock(driver);
Need to report an error if the nf_list_interfaces call fails.
> +static char *interfaceGetXMLDesc(virInterfacePtr ifinfo,
> + unsigned int flags ATTRIBUTE_UNUSED)
> +{
> + struct interface_driver *driver = ifinfo->conn->interfacePrivateData;
> + struct netcf_if *iface = NULL;
> + char *ret = NULL;
> +
> + interfaceDriverLock(driver);
> + iface = ncf_lookup_by_name(driver->netcf, ifinfo->name);
> +
> + if (!iface) {
> + /* May want to check by name here, but probably pointless */
> + interfaceReportError(ifinfo->conn, NULL, ifinfo, VIR_ERR_INVALID_INTERFACE,
> + "%s", _("no interface with matching MAC Address"));
> + goto cleanup;
> + }
If we're using name as the unique identfier, then we should change this
error message, and preferrably include the name we faiiled to find in
the error message itself.
> +
> + ret = ncf_if_xml_desc(iface);
> + if (!ret) {
> + interfaceReportError(ifinfo->conn, NULL, ifinfo, VIR_ERR_INVALID_INTERFACE,
> + "%s", _("could not get interface XML description"));
> + goto cleanup;
> + }
Wrong error code for this one. If netcf can't give more info on what
went wrong, then use VIR_ERR_INTERNAL_ERROR as the code.
> +
> +cleanup:
> + ncf_if_free(iface);
> + interfaceDriverUnlock(driver);
> + return ret;
> +}
> +
> +static virInterfacePtr interfaceDefineXML(virConnectPtr conn,
> + const char *xml,
> + unsigned int flags ATTRIBUTE_UNUSED)
> +{
> + struct interface_driver *driver = conn->interfacePrivateData;
> + struct netcf_if *iface = NULL;
> + virInterfacePtr ret = NULL;
> +
> + interfaceDriverLock(driver);
> +
> + /*
> + * This is where we will want to validate the XML, and possibly
> + * transform it before sending it on.
> + */
> +
> + iface = ncf_define(driver->netcf, xml);
> + if (!iface) {
> + interfaceReportError(conn, NULL, NULL, VIR_ERR_INVALID_INTERFACE,
> + "%s", _("failed to define new interface from XML"));
> + goto cleanup;
> + }
Shouldn't use INVALID_INTERFACE here either - this error codeshould only
be used in libvirt.c, where it validate the magic data in the virConnect
or virInterface objects. Thre's a few more of these cases later, I won't
go through every instance of it.
> @@ -0,0 +1,34 @@
> +/*
> + * interface_driver.h: core driver methods for managing physical host interfaces
> + *
> + * Copyright (C) 2006, 2007 Red Hat, Inc.
> + * Copyright (C) 2006 Daniel P. Berrange
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + * Author: Daniel P. Berrange <berrange at redhat.com>
> + */
> +
> +
> +#ifndef __VIR_INTERFACE__DRIVER_H
> +#define __VIR_INTERFACE__DRIVER_H
> +
> +#include <config.h>
This is only needed in the .c files
> +
> +#include "internal.h"
Can drop this too, since nothing in this header file relies o nstuff
defined in internal.h
> +
> +int interfaceRegister(void);
> +
> +#endif /* __VIR_INTERFACE__DRIVER_H */
> --
So mostly just a case of cleaning up the error handling, and then adding
in the XML parser & formatter calls everywhere we have XML coming in or
out of libvirt APIs.
Regards,
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
More information about the libvir-list
mailing list