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@xxxxxxxxxx> wrote:
On 12/21/13 05:03, Adam Walters wrote:Again, please trim the commit message (see 2/3 in your other series for
> 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.
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.
Won't this driver be beneficial for session connections too? (they run
>
> Signed-off-by: Adam Walters <adam@xxxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxx>
> + */
> +#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;
> + }
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;
> +}
The comments here should state the release where the API was implemented
> +
> +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 */
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.
Peter
> +};
> +
> +
> +static virStateDriver configStateDriver = {
> + .name = "config",
> + .stateInitialize = configStateInitialize,
> + .stateCleanup = configStateCleanup,
> + .stateReload = configStateReload,
> +};
> +
> +int configRegister(void) {
> + virRegisterDriver(&configDriver);
> + virRegisterStateDriver(&configStateDriver);
> + return 0;
> +}
>
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list