| Message ID | 20220615105212.780256-5-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | softmmu: make qemu_find_file more flexible wrt build dir layout | expand |
On 6/15/22 12:52, Daniel P. Berrangé wrote: > > + case QEMU_FILE_TYPE_HELPER: > + rel_install_dir = ""; > + rel_build_dir = ""; > + default_install_dir = default_helper_dir; > + break; > + You're replacing ad hoc rules in Akihiko's meson.build with an ad hoc enum + the corresponding "case"s here in qemu_find_file(). There is duplication anyway, in this case between Meson and QEMU (plus QEMU needs to know about two filesystem layouts). Paolo
On Wed, Jun 15, 2022 at 01:42:58PM +0200, Paolo Bonzini wrote: > On 6/15/22 12:52, Daniel P. Berrangé wrote: > > + case QEMU_FILE_TYPE_HELPER: > > + rel_install_dir = ""; > > + rel_build_dir = ""; > > + default_install_dir = default_helper_dir; > > + break; > > + > > You're replacing ad hoc rules in Akihiko's meson.build with an ad hoc enum + > the corresponding "case"s here in qemu_find_file(). There is duplication > anyway, in this case between Meson and QEMU (plus QEMU needs to know about > two filesystem layouts). IMHO this is simpler to deal with than the meson additions, and also avoids the confusion of having files appearing in two places in the build dir. If we really want to have the build dir look just like the install dir though, why write custom meson commands per file type at all, instead add a rule that always invokes DESTDIR=$(BUILDDIR)/vroot ninja install to populate a dir that's guaranteed identical to the install layout Regards, Daniel
On 2022/06/15 21:04, Daniel P. Berrangé wrote: > On Wed, Jun 15, 2022 at 01:42:58PM +0200, Paolo Bonzini wrote: >> On 6/15/22 12:52, Daniel P. Berrangé wrote: >>> + case QEMU_FILE_TYPE_HELPER: >>> + rel_install_dir = ""; >>> + rel_build_dir = ""; >>> + default_install_dir = default_helper_dir; >>> + break; >>> + >> >> You're replacing ad hoc rules in Akihiko's meson.build with an ad hoc enum + >> the corresponding "case"s here in qemu_find_file(). There is duplication >> anyway, in this case between Meson and QEMU (plus QEMU needs to know about >> two filesystem layouts). > > IMHO this is simpler to deal with than the meson additions, and also > avoids the confusion of having files appearing in two places in the > build dir. Thanks to Paolo's suggestion to unify the common code to build the bundle tree, the required code for each bundled file is just a statement now (something like: bundles += { destination: source }) in the v6. Doing everything in Meson also allows to reuse the knowledge of the build tree Meson already has. I do no longer think my patch series are complicated more than yours. It even has less lines now. There is still a room for improvements though. Particularly, the installing code and bundle-tree code are still duplicate. For example, pc-bios/meson.build now has the following code: install_data(blobs, install_dir: qemu_datadir) foreach f : blobs bundles += { qemu_datadir / f: meson.current_source_dir() / f } endforeach It would be nice if it can be written like: foreach f : blobs bundle_data(qemu_datadir / f, f) endforeach Unfortunately Meson does not allow this. Another problem is that the top-level meson.build is somewhat clutter. In my opinion, it is a persistent problem of the build system but I don't have a solution. Anyway, I think my patch series is as close to the ideal as Meson currently allows. The confusion caused by the files appearing in two places in the build tree should be minimal. qemu-bundle is implemented entirely with symbolic links. If you know what is a symbolic link, you also know it is an alias and the files appear in different places, and I expect everyone hacking QEMU knows symbolic link. > > If we really want to have the build dir look just like the install > dir though, why write custom meson commands per file type at all, > instead add a rule that always invokes > > DESTDIR=$(BUILDDIR)/vroot ninja install > > to populate a dir that's guaranteed identical to the install layout Unfortunately Meson cannot define a rule which will be always invoked as far as I know. Regards, Akihiko Odaki > > Regards, > Daniel
diff --git a/include/net/net.h b/include/net/net.h index 523136c7ac..6a853512ac 100644 --- a/include/net/net.h +++ b/include/net/net.h @@ -228,7 +228,8 @@ NetClientState *net_hub_port_find(int hub_id); #define DEFAULT_NETWORK_SCRIPT CONFIG_SYSCONFDIR "/qemu-ifup" #define DEFAULT_NETWORK_DOWN_SCRIPT CONFIG_SYSCONFDIR "/qemu-ifdown" -#define DEFAULT_BRIDGE_HELPER CONFIG_QEMU_HELPERDIR "/qemu-bridge-helper" +#define DEFAULT_BRIDGE_HELPER "qemu-bridge-helper" +#define DEFAULT_BRIDGE_HELPER_PATH CONFIG_QEMU_HELPERDIR "/qemu-bridge-helper" #define DEFAULT_BRIDGE_INTERFACE "br0" void qdev_set_nic_properties(DeviceState *dev, NICInfo *nd); diff --git a/include/qemu/datadir.h b/include/qemu/datadir.h index 427e90787a..a211b6b235 100644 --- a/include/qemu/datadir.h +++ b/include/qemu/datadir.h @@ -4,6 +4,7 @@ #define QEMU_FILE_TYPE_BIOS 0 #define QEMU_FILE_TYPE_KEYMAP 1 #define QEMU_FILE_TYPE_ICON 2 +#define QEMU_FILE_TYPE_HELPER 3 /** * qemu_find_file: diff --git a/net/tap.c b/net/tap.c index b3ddfd4a74..161608e34a 100644 --- a/net/tap.c +++ b/net/tap.c @@ -42,6 +42,7 @@ #include "qemu/error-report.h" #include "qemu/main-loop.h" #include "qemu/sockets.h" +#include "qemu/datadir.h" #include "net/tap.h" @@ -507,9 +508,11 @@ static int net_bridge_run_helper(const char *helper, const char *bridge, sigprocmask(SIG_BLOCK, &mask, &oldmask); if (!helper) { - helper = default_helper = get_relocated_path(DEFAULT_BRIDGE_HELPER); + helper = default_helper = qemu_find_file(QEMU_FILE_TYPE_HELPER, + DEFAULT_BRIDGE_HELPER); } + g_printerr("Helper %s\n", helper); if (socketpair(PF_UNIX, SOCK_STREAM, 0, sv) == -1) { error_setg_errno(errp, errno, "socketpair() failed"); return -1; diff --git a/qemu-options.hx b/qemu-options.hx index 377d22fbd8..b5b7e75048 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -2665,7 +2665,7 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev, " to configure it and 'dfile' (default=" DEFAULT_NETWORK_DOWN_SCRIPT ")\n" " to deconfigure it\n" " use '[down]script=no' to disable script execution\n" - " use network helper 'helper' (default=" DEFAULT_BRIDGE_HELPER ") to\n" + " use network helper 'helper' (default=" DEFAULT_BRIDGE_HELPER_PATH ") to\n" " configure it\n" " use 'fd=h' to connect to an already opened TAP interface\n" " use 'fds=x:y:...:z' to connect to already opened multiqueue capable TAP interfaces\n" @@ -2684,7 +2684,7 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev, "-netdev bridge,id=str[,br=bridge][,helper=helper]\n" " configure a host TAP network backend with ID 'str' that is\n" " connected to a bridge (default=" DEFAULT_BRIDGE_INTERFACE ")\n" - " using the program 'helper (default=" DEFAULT_BRIDGE_HELPER ")\n" + " using the program 'helper (default=" DEFAULT_BRIDGE_HELPER_PATH ")\n" #endif #ifdef __linux__ "-netdev l2tpv3,id=str,src=srcaddr,dst=dstaddr[,srcport=srcport][,dstport=dstport]\n" diff --git a/softmmu/datadir.c b/softmmu/datadir.c index e5d1fd0116..a68fe7167a 100644 --- a/softmmu/datadir.c +++ b/softmmu/datadir.c @@ -36,6 +36,7 @@ static char **extra_firmware_dirs; /* Default built-in directories */ static char *default_data_dir; static char *default_icon_dir; +static char *default_helper_dir; /* Whether we're known to be executing from a build tree */ static bool in_build_dir; @@ -73,6 +74,12 @@ char *qemu_find_file(int type, const char *name) default_install_dir = default_icon_dir; break; + case QEMU_FILE_TYPE_HELPER: + rel_install_dir = ""; + rel_build_dir = ""; + default_install_dir = default_helper_dir; + break; + default: abort(); } @@ -140,9 +147,11 @@ void qemu_add_default_firmwarepath(void) /* Add default dirs relative to the executable path */ default_data_dir = get_relocated_path(CONFIG_QEMU_DATADIR); default_icon_dir = get_relocated_path(CONFIG_QEMU_ICONDIR); + default_helper_dir = get_relocated_path(CONFIG_QEMU_HELPERDIR); trace_datadir_init(default_data_dir, default_icon_dir, + default_helper_dir, in_build_dir); } diff --git a/softmmu/trace-events b/softmmu/trace-events index 9c00e9f389..b22d7e7714 100644 --- a/softmmu/trace-events +++ b/softmmu/trace-events @@ -6,7 +6,7 @@ balloon_event(void *opaque, unsigned long addr) "opaque %p addr %lu" # datadir.c datadir_load_file(const char *name, const char *path, int err) "name %s location %s errno %d" -datadir_init(const char *defdatadir, const char *deficondir, bool inbuilddir) "default data dir %s icon dir %s in build dir %d" +datadir_init(const char *defdatadir, const char *deficondir, const char *defhelperdir, bool inbuilddir) "default data dir %s icon dir %s helper dir %s in build dir %d" # ioport.c cpu_in(unsigned int addr, char size, unsigned int val) "addr 0x%x(%c) value %u"
The TAP device code currently uses get_relocate_path to find the bridge helper, however, this fails when run from the build dir. Adding support to qemu_find_file for helper binaries, allows it to work from both the (relocated) install tree and build dir. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- include/net/net.h | 3 ++- include/qemu/datadir.h | 1 + net/tap.c | 5 ++++- qemu-options.hx | 4 ++-- softmmu/datadir.c | 9 +++++++++ softmmu/trace-events | 2 +- 6 files changed, 19 insertions(+), 5 deletions(-)