Re: [libvirt] [PATCH 3/5] qemu_conf: add new configuration key bridge_helper

On 03/25/2013 10:25 AM, Paolo Bonzini wrote: > Signed-off-by: Paolo Bonzini <pbonzini(a)redhat.com&gt; > --- > src/qemu/libvirtd_qemu.aug | 1 + > src/qemu/qemu.conf | 8 ++++++++ > src/qemu/qemu_conf.c | 3 +++ > src/qemu/qemu_conf.h | 1 + > src/qemu/test_libvirtd_qemu.aug.in | 1 + > 5 files changed, 14 insertions(+) > > diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug > index 91f5f77..61740a9 100644 > --- a/src/qemu/libvirtd_qemu.aug > +++ b/src/qemu/libvirtd_qemu.aug > @@ -60,6 +60,7 @@ module Libvirtd_qemu = > > let process_entry = str_entry "hugetlbfs_mount" > | bool_entry "clear_emulator_capabilities" > + | str_entry "bridge_helper" > | bool_entry "set_process_name" > | int_entry "max_processes" > | int_entry "max_files" > diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf > index dd853c8..87bdf70 100644 > --- a/src/qemu/qemu.conf > +++ b/src/qemu/qemu.conf > @@ -301,6 +301,14 @@ > #hugetlbfs_mount = "/dev/hugepages" > > > +# Path to the setuid helper for creating tap devices. This executable > +# is used to create <source type='bridge'> interfaces when libvirtd is > +# running unprivileged. libvirt invokes the helper directly, instead > +# of using "-netdev bridge", for security reasons. > +#bridge_helper = "/usr/libexec/qemu-bridge-helper" > + > + Are we sure we want to allow this to be configured? That could lead to some "interesting" troubleshooting incidents :-) On the other hand, I guess the path to qemu itself is right there in the domain config file, so how much worse could this be... > + > # If clear_emulator_capabilities is enabled, libvirt will drop all > # privileged capabilities of the QEmu/KVM emulator. This is enabled by > # default. > diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c > index c2e2e10..f648fc3 100644 > --- a/src/qemu/qemu_conf.c > +++ b/src/qemu/qemu_conf.c > @@ -248,6 +248,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) > } > } > #endif > + cfg->bridgeHelperName = strdup("/usr/libexec/qemu-bridge-helper"); > > cfg->clearEmulatorCapabilities = true; > > @@ -297,6 +298,7 @@ static void virQEMUDriverConfigDispose(void *obj) > > VIR_FREE(cfg->hugetlbfsMount); > VIR_FREE(cfg->hugepagePath); > + VIR_FREE(cfg->bridgeHelperName); > > VIR_FREE(cfg->saveImageFormat); > VIR_FREE(cfg->dumpImageFormat); > @@ -509,6 +511,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, > GET_VALUE_BOOL("auto_start_bypass_cache", cfg->autoStartBypassCache); > > GET_VALUE_STR("hugetlbfs_mount", cfg->hugetlbfsMount); > + GET_VALUE_STR("bridge_helper", cfg->bridgeHelperName); > > GET_VALUE_BOOL("mac_filter", cfg->macFilter); > > diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h > index c5ddaad..666ac33 100644 > --- a/src/qemu/qemu_conf.h > +++ b/src/qemu/qemu_conf.h > @@ -117,6 +117,7 @@ struct _virQEMUDriverConfig { > > char *hugetlbfsMount; > char *hugepagePath; > + char *bridgeHelperName; > > bool macFilter; > > diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in > index 2892204..0aec997 100644 > --- a/src/qemu/test_libvirtd_qemu.aug.in > +++ b/src/qemu/test_libvirtd_qemu.aug.in > @@ -49,6 +49,7 @@ module Test_libvirtd_qemu = > { "auto_dump_bypass_cache" = "0" } > { "auto_start_bypass_cache" = "0" } > { "hugetlbfs_mount" = "/dev/hugepages" } > +{ "bridge_helper" = "/usr/libexec/qemu-bridge-helper" } > { "clear_emulator_capabilities" = "1" } > { "set_process_name" = "1" } > { "max_processes" = "0" } ACK. (But I'd like at least one other ACK from someone else due to the fact that this is polluting the config namespace with something we would like to eventually eliminate.)