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 <[email protected]> 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 <[email protected]> > > --- > > 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 <[email protected]> > > + */ > > +#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 > >
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
