2:37 p.m.
Peter,
Thank you for the feedback on my patches, and sorry for the delay. I was
away all weekend, since it was a holiday weekend (definitely a plus, having
my day job in education). I figured it would be a bit spammy to reply to
each individual message, but I did read each of them, and will be working
to implement the suggestions tonight and/or tomorrow night.
On Mon, Jan 20, 2014 at 10:34 AM, Peter Krempa <pkrempa(a)redhat.com> wrote:
On 12/21/13 05:03, Adam Walters wrote:
> This is the source code to the config driver. This driver is a
hypervisor driver that does not support any domain operations. The sole
purpose of this driver is to allow access to various bits of configuration
information, such as secret or network definitions, from the initialization
and auto-start routines of other drivers. This is the first step toward
breaking the circular dependencies present in QEMU and the Storage driver,
in addition to preventing future cases.
Again, please trim the commit message (see 2/3 in your other series for
instructions).
I apologize for the long lines. I had forgotten about reading the log
messages
in a standard terminal, as I mostly read those in a web interface.
>
> Signed-off-by: Adam Walters <adam(a)pandorasboxen.com>
> ---
> src/config/config_driver.c | 237
+++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 237 insertions(+)
> create mode 100644 src/config/config_driver.c
>
> diff --git a/src/config/config_driver.c b/src/config/config_driver.c
> new file mode 100644
> index 0000000..a057300
> --- /dev/null
> +++ b/src/config/config_driver.c
> @@ -0,0 +1,237 @@
> +/*
> + * config_driver.c: core driver methods for accessing configs
> + *
> + * Copyright (C) 2013 Adam Walters
> + *
> + * 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, see
> + * <http://www.gnu.org/licenses/>;.
> + *
> + * Author: Adam Walters <adam(a)pandorasboxen.com>
> + */
> +#include <config.h>
> +#include <string.h>
> +
> +#include "internal.h"
> +#include "datatypes.h"
> +#include "driver.h"
> +#include "virlog.h"
> +#include "viralloc.h"
> +#include "virerror.h"
> +#include "virstring.h"
> +#include "viraccessapicheck.h"
> +#include "nodeinfo.h"
> +#include "config_driver.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_CONFIG
> +
> +virConfigDriverPtr config_driver = NULL;
> +
> +static int configStateCleanup(void) {
> + if (config_driver == NULL)
> + return -1;
> +
> + virObjectUnref(config_driver->closeCallbacks);
> +
> + virSysinfoDefFree(config_driver->hostsysinfo);
> +
> + virMutexDestroy(&config_driver->lock);
> + VIR_FREE(config_driver);
> +
> + return 0;
> +}
> +
> +static int configStateInitialize(bool privileged,
> + virStateInhibitCallback callback
ATTRIBUTE_UNUSED,
> + void *opaque ATTRIBUTE_UNUSED)
> +{
> + if (!privileged) {
> + VIR_INFO("Not running privileged, disabling driver");
> + return 0;
> + }
Won't this driver be beneficial for session connections too? (they run
unprivileged.
As for denying unprivileged connections, I'm honestly not sure if it would
be useful. All this driver does is grant access to definitions of networks,
storage pools, secrets, etc. I just ensured that this driver is loaded very
early in the load order, and has zero external dependencies. I suppose
that I made an assumption that the privileged variable indicated a
privileged
connection, but thinking about it more, I suppose it could also mean running
privileged in the OS. I simply disabled the driver if unprivileged so that I
would not leak any privileged information to an unprivileged connection
by accident. If that isn't a concern (either I misunderstood the meaning of
the privileged variable, or the code wouldn't leak information), I can
certainly
drop those lines to allow the driver when unprivileged. As I've mentioned
before, I am not intimately familiar with the libvirt code, so I tend to err
on the side of caution.
> +
> + if (VIR_ALLOC(config_driver) < 0)
> + return -1;
> +
> + if (virMutexInit(&config_driver->lock) < 0) {
> + VIR_ERROR(_("cannot initialize mutex"));
> + VIR_FREE(config_driver);
> + return -1;
> + }
> +
> + config_driver->hostsysinfo = virSysinfoRead();
> +
> + if (!(config_driver->closeCallbacks = virCloseCallbacksNew()))
> + goto cleanup;
> +
> + return 0;
> +
> +cleanup:
> + configStateCleanup();
> + return -1;
> +}
...
> +
> +static virDriver configDriver = {
> + .name = "config",
> + .connectOpen = configConnectOpen, /* 0.2.0 */
> + .connectClose = configConnectClose, /* 0.2.0 */
> + .connectSupportsFeature = configConnectSupportsFeature, /* 0.5.0 */
> + .connectGetType = configConnectGetType, /* 0.2.0 */
> + .connectGetHostname = configConnectGetHostname, /* 0.3.3 */
> + .connectGetSysinfo = configConnectGetSysinfo, /* 0.8.8 */
> + .nodeGetInfo = configNodeGetInfo, /* 0.2.0 */
> + .connectIsEncrypted = configConnectIsEncrypted, /* 0.7.3 */
> + .connectIsSecure = configConnectIsSecure, /* 0.7.3 */
> + .connectIsAlive = configConnectIsAlive, /* 0.9.8 */
The comments here should state the release where the API was implemented
for the driver, thus you need to change them to /* 1.2.2 */.
I hadn't forgotten about the API version comment, but when I wrote the
patches,
I simply had no idea when version would be next. Never know if there will
be a minor
version increment instead of the standard micro. Since it was more of an
RFC-type
patch, I didn't even have an idea of a timeframe for submission, since I
thought there
might be more discussion prior to looking at pulling a change of this type
into the code.
> +};
> +
> +
> +static virStateDriver configStateDriver = {
> + .name = "config",
> + .stateInitialize = configStateInitialize,
> + .stateCleanup = configStateCleanup,
> + .stateReload = configStateReload,
> +};
> +
> +int configRegister(void) {
> + virRegisterDriver(&configDriver);
> + virRegisterStateDriver(&configStateDriver);
> + return 0;
> +}
>
Peter