Re: [PATCH v4 01/20] softmmu: split off vl.c:main() into main.c

[Alexander Bulekov <alxndr@bu.edu>]

On 11/5/19 11:41 AM, Darren Kenny wrote:
> On Wed, Oct 30, 2019 at 02:49:48PM +0000, Oleinik, Alexander wrote:
>> From: Alexander Oleinik <alxndr@bu.edu>
>>
>> A program might rely on functions implemented in vl.c, but implement its
>> own main(). By placing main into a separate source file, there are no
>> complaints about duplicate main()s when linking against vl.o. For
>> example, the virtual-device fuzzer uses a main() provided by libfuzzer,
>> and needs to perform some initialization before running the softmmu
>> initialization. Now, main simply calls three vl.c functions which
>> handle the guest initialization, main loop and cleanup.
>>
>> Signed-off-by: Alexander Oleinik <alxndr@bu.edu>
>> ---
>> Makefile                |  1 +
>> Makefile.objs           |  2 ++
>> include/sysemu/sysemu.h |  4 ++++
>> main.c                  | 52 +++++++++++++++++++++++++++++++++++++++++
>> vl.c                    | 36 +++++++---------------------
>> 5 files changed, 68 insertions(+), 27 deletions(-)
>> create mode 100644 main.c
>>
>> diff --git a/Makefile b/Makefile
>> index 0e994a275d..d2b2ecd3c4 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -474,6 +474,7 @@ $(SOFTMMU_ALL_RULES): $(crypto-obj-y)
>> $(SOFTMMU_ALL_RULES): $(io-obj-y)
>> $(SOFTMMU_ALL_RULES): config-all-devices.mak
>> $(SOFTMMU_ALL_RULES): $(edk2-decompressed)
>> +$(SOFTMMU_ALL_RULES): $(softmmu-main-y)
>>
>> .PHONY: $(TARGET_DIRS_RULES)
>> # The $(TARGET_DIRS_RULES) are of the form SUBDIR/GOAL, so that
>> diff --git a/Makefile.objs b/Makefile.objs
>> index 11ba1a36bd..9ff9b0c6f9 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -86,6 +86,8 @@ common-obj-$(CONFIG_FDT) += device_tree.o
>> # qapi
>>
>> common-obj-y += qapi/
>> +
>> +softmmu-main-y = main.o
>> endif
>>
>> #######################################################################
>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
>> index 44f18eb739..03f9838b81 100644
>> --- a/include/sysemu/sysemu.h
>> +++ b/include/sysemu/sysemu.h
>> @@ -114,6 +114,10 @@ QemuOpts *qemu_get_machine_opts(void);
>>
>> bool defaults_enabled(void);
>>
>> +void main_loop(void);
>> +void qemu_init(int argc, char **argv, char **envp);
>> +void qemu_cleanup(void);
>> +
>> extern QemuOptsList qemu_legacy_drive_opts;
>> extern QemuOptsList qemu_common_drive_opts;
>> extern QemuOptsList qemu_drive_opts;
>> diff --git a/main.c b/main.c
>> new file mode 100644
>> index 0000000000..ecd6389424
>> --- /dev/null
>> +++ b/main.c
>> @@ -0,0 +1,52 @@
>> +/*
>> + * QEMU System Emulator
>> + *
>> + * Copyright (c) 2003-2008 Fabrice Bellard
>> + *
>> + * Permission is hereby granted, free of charge, to any person 
>> obtaining a copy
>> + * of this software and associated documentation files (the 
>> "Software"), to deal
>> + * in the Software without restriction, including without limitation 
>> the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, 
>> and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be 
>> included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
>> EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
>> MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT 
>> SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES 
>> OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, 
>> ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER 
>> DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "sysemu/sysemu.h"
>> +
>> +#ifdef CONFIG_SDL
>> +#if defined(__APPLE__) || defined(main)
>> +#include <SDL.h>
>> +int main(int argc, char **argv)
>> +{
>> +    return qemu_main(argc, argv, NULL);
>> +}
>> +#undef main
>> +#define main qemu_main
> 
> This /looks/ wrong, you're defining a function main(), and then
> immediately #undef and #define main again.
> 
> Maybe this could be written differently, or add a comment here as to
> why you need to do this.
> 
>> +#endif
>> +#endif /* CONFIG_SDL */
>> +
>> +#ifdef CONFIG_COCOA
>> +#undef main
>> +#define main qemu_main
>> +#endif /* CONFIG_COCOA */
> 
> I don't really know the combinations that might exist, but it looks
> like if CONFIG_SDL is not defined, then we're redefining main() to be
> qemi_main() - so what main() function will actually be used here?

I tried to copy this straight from vl.c. It seems that this was 
originally added for similar reasons that I added this patch - similarly 
to libfuzzer, SDL has its own main function, and I'm guessing its 
similar for cocoa. With some  preprocessor flags, the result looks like:
int SDL_main(int argc, char **argv)
{
     return qemu_main(argc, argv,
                                 ((void *)0)
                                     );
}
int qemu_main(int argc, char **argv, char **envp)
{
     qemu_init(argc, argv, envp);
     main_loop();
     qemu_cleanup();
     return 0;
}

So it looks like this is there since SDL expects main to have two args. 
Maybe this is something that can be solved by adding separate main-sdl.c 
and main-cocoa.c files and tweaks to the build system, which should be 
possible now that qemu_init is separate from main().

-Alex

> Thanks,
> 
> Darren.
> 
>> +
>> +int main(int argc, char **argv, char **envp)
>> +{
>> +    qemu_init(argc, argv, envp);
>> +    main_loop();
>> +    qemu_cleanup();
>> +
>> +    return 0;
>> +}
>> diff --git a/vl.c b/vl.c
>> index c389d24b2c..472f09e12a 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -36,25 +36,6 @@
>> #include "sysemu/seccomp.h"
>> #include "sysemu/tcg.h"
>>
>> -#ifdef CONFIG_SDL
>> -#if defined(__APPLE__) || defined(main)
>> -#include <SDL.h>
>> -int qemu_main(int argc, char **argv, char **envp);
>> -int main(int argc, char **argv)
>> -{
>> -    return qemu_main(argc, argv, NULL);
>> -}
>> -#undef main
>> -#define main qemu_main
>> -#endif
>> -#endif /* CONFIG_SDL */
>> -
>> -#ifdef CONFIG_COCOA
>> -#undef main
>> -#define main qemu_main
>> -#endif /* CONFIG_COCOA */
>> -
>> -
>> #include "qemu/error-report.h"
>> #include "qemu/sockets.h"
>> #include "sysemu/accel.h"
>> @@ -1797,7 +1778,7 @@ static bool main_loop_should_exit(void)
>>     return false;
>> }
>>
>> -static void main_loop(void)
>> +void main_loop(void)
>> {
>> #ifdef CONFIG_PROFILER
>>     int64_t ti;
>> @@ -2824,7 +2805,7 @@ static void user_register_global_props(void)
>>                       global_init_func, NULL, NULL);
>> }
>>
>> -int main(int argc, char **argv, char **envp)
>> +void qemu_init(int argc, char **argv, char **envp)
>> {
>>     int i;
>>     int snapshot, linux_boot;
>> @@ -3404,7 +3385,7 @@ int main(int argc, char **argv, char **envp)
>>             case QEMU_OPTION_watchdog:
>>                 if (watchdog) {
>>                     error_report("only one watchdog option may be 
>> given");
>> -                    return 1;
>> +                    exit(1);
>>                 }
>>                 watchdog = optarg;
>>                 break;
>> @@ -4440,7 +4421,7 @@ int main(int argc, char **argv, char **envp)
>>     if (vmstate_dump_file) {
>>         /* dump and exit */
>>         dump_vmstate_json_to_file(vmstate_dump_file);
>> -        return 0;
>> +        exit(0);
>>     }
>>
>>     if (incoming) {
>> @@ -4457,8 +4438,11 @@ int main(int argc, char **argv, char **envp)
>>     accel_setup_post(current_machine);
>>     os_setup_post();
>>
>> -    main_loop();
>> +    return;
>> +}
>>
>> +void qemu_cleanup(void)
>> +{
>>     gdbserver_cleanup();
>>
>>     /*
>> @@ -4495,6 +4479,4 @@ int main(int argc, char **argv, char **envp)
>>     qemu_chr_cleanup();
>>     user_creatable_cleanup();
>>     /* TODO: unref root container, check all devices are ok */
>> -
>> -    return 0;
>> }
>> -- 
>> 2.23.0
>>
>>


-- 
===
I recently changed my last name from Oleinik to Bulekov
===