On Mon, Feb 20, 2017 at 11:55:06AM +0000, Daniel P. Berrange wrote:
On Mon, Feb 20, 2017 at 12:50:23PM +0100, Peter Krempa wrote:On Tue, Feb 14, 2017 at 15:30:44 +0100, Michal Privoznik wrote: > Now that we have some qemuSecurity wrappers over > virSecurityManager APIs, lets make sure everybody sticks with > them. We have them for a reason and calling virSecurityManager > API directly instead of wrapper may lead into accidentally > labelling a file on the host instead of namespace. > > Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx> > --- > > This is an alternative approach to: > > https://www.redhat.com/archives/libvir-list/2017-February/msg00271.html > > cfg.mk | 5 ++++ > src/qemu/qemu_command.c | 7 +++--- > src/qemu/qemu_conf.c | 9 ++++--- > src/qemu/qemu_domain.c | 17 ++++++------- > src/qemu/qemu_driver.c | 63 ++++++++++++++++++++++------------------------- > src/qemu/qemu_hotplug.c | 4 +-- > src/qemu/qemu_migration.c | 13 +++++----- > src/qemu/qemu_process.c | 61 ++++++++++++++++++++++----------------------- > src/qemu/qemu_security.h | 32 ++++++++++++++++++++++++ > 9 files changed, 122 insertions(+), 89 deletions(-) > [...] > diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h > index 54638908d..d86db3f6b 100644 > --- a/src/qemu/qemu_security.h > +++ b/src/qemu/qemu_security.h > @@ -28,6 +28,7 @@ > > # include "qemu_conf.h" > # include "domain_conf.h" > +# include "security/security_manager.h" > > int qemuSecuritySetAllLabel(virQEMUDriverPtr driver, > virDomainObjPtr vm, > @@ -60,4 +61,35 @@ int qemuSecuritySetHostdevLabel(virQEMUDriverPtr driver, > int qemuSecurityRestoreHostdevLabel(virQEMUDriverPtr driver, > virDomainObjPtr vm, > virDomainHostdevDefPtr hostdev); > + > +/* Please note that for these APIs there is no wrapper yet. Do NOT blindly add > + * new APIs here. If an API can touch a /dev file add a proper wrapper instead. > + */ > +# define qemuSecurityCheckAllLabel virSecurityManagerCheckAllLabel > +# define qemuSecurityClearSocketLabel virSecurityManagerClearSocketLabel > +# define qemuSecurityDomainSetPathLabel virSecurityManagerDomainSetPathLabel > +# define qemuSecurityGenLabel virSecurityManagerGenLabel > +# define qemuSecurityGetBaseLabel virSecurityManagerGetBaseLabel > +# define qemuSecurityGetDOI virSecurityManagerGetDOI > +# define qemuSecurityGetModel virSecurityManagerGetModel > +# define qemuSecurityGetMountOptions virSecurityManagerGetMountOptions > +# define qemuSecurityGetNested virSecurityManagerGetNested > +# define qemuSecurityGetProcessLabel virSecurityManagerGetProcessLabel > +# define qemuSecurityNew virSecurityManagerNew > +# define qemuSecurityNewDAC virSecurityManagerNewDAC > +# define qemuSecurityNewStack virSecurityManagerNewStack > +# define qemuSecurityPostFork virSecurityManagerPostFork > +# define qemuSecurityPreFork virSecurityManagerPreFork > +# define qemuSecurityReleaseLabel virSecurityManagerReleaseLabel > +# define qemuSecurityReserveLabel virSecurityManagerReserveLabel > +# define qemuSecurityRestoreSavedStateLabel virSecurityManagerRestoreSavedStateLabel > +# define qemuSecuritySetChildProcessLabel virSecurityManagerSetChildProcessLabel > +# define qemuSecuritySetDaemonSocketLabel virSecurityManagerSetDaemonSocketLabel > +# define qemuSecuritySetImageFDLabel virSecurityManagerSetImageFDLabel > +# define qemuSecuritySetSavedStateLabel virSecurityManagerSetSavedStateLabel > +# define qemuSecuritySetSocketLabel virSecurityManagerSetSocketLabel > +# define qemuSecuritySetTapFDLabel virSecurityManagerSetTapFDLabel > +# define qemuSecurityStackAddNested virSecurityManagerStackAddNested > +# define qemuSecurityVerify virSecurityManagerVerify I don't like this either for similar reasons that I've stated on the original series.I think the interesting question here is whether we can figure out a way to push the qemuSecurity* functionality into the security drivers and thus remove the need for all these wrappers.... Currently all the wrappers look the same doing if (namespace enabled) { ...start transaction... } ... call real method... if (namespace enabled) { ...commit transaction... } The key observation to me is that there is only ever a single method called inside the transaction. That ought to mean we can push the transaction functionality down a layer. eg in the virSecurityManagerNew* methods we could pass in a "bool useTransactions" flag and then the security_manager.c could take care of start/commit/rollback
Yeah, I was thinking about that too before, one of the ideas I had before. However, you need the bool to be there per-domain, not per-driver, and I haven't found out how to automatically take care of commit (and rollback). You'd have to call that anyway.
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
