[libvirt] [RFC PATCH 1/7] config: Adding source for the config driver

[Adam Walters]

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 at 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 at 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 at 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140121/3e3fcf90/attachment-0001.htm>