[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 10/16] Introduce OS specific cmdline argument ha
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 10/16] Introduce OS specific cmdline argument handling and move SMB arg to os-posix.c |
Date: |
Fri, 04 Jun 2010 14:04:02 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) |
Jes Sorensen <address@hidden> writes:
> On 06/04/10 10:15, Markus Armbruster wrote:
>> address@hidden writes:
>>> + * Parse OS specific command line options.
>>> + * return 0 if option handled, -1 otherwise
>>> + */
>>> +int os_parse_cmd_args(const QEMUOption *popt, const char *optarg)
>>> +{
>>> + int ret = 0;
>>> + switch (popt->index) {
>>> +#ifdef CONFIG_SLIRP
>>> + case QEMU_OPTION_smb:
>>> + if (net_slirp_smb(optarg) < 0)
>>> + exit(1);
>>> + break;
>>> +#endif
>>
>> Was #ifndef _WIN32 before. Impact?
>
> It was moved to os-posix.c which is only built for non _WIN32, so it has
> the same effect, except it's not full of ugly #ifdef's
I missed the fact that it is under #ifdef CONFIG_SLIRP in the current
code. Sorry for the noise.
>>> +/*
>>> + * Duplicate definition from vl.c to avoid messing up the entire build
>>> + */
>>> +enum {
>>> +#define DEF(option, opt_arg, opt_enum, opt_help, arch_mask) \
>>> + opt_enum,
>>> +#define DEFHEADING(text)
>>> +#include "qemu-options.h"
>>> +#undef DEF
>>> +#undef DEFHEADING
>>> +#undef GEN_DOCS
>>> +};
>>
>> I agree with Richard: this is gross.
>
> The enum creation is gross by itself. Only way to get around not
> duplicating it is to create a new header file to hold just that?
>
>>> +/* This is needed for vl.c and the OS specific files */
>>> +typedef struct QEMUOption {
>>> + const char *name;
>>> + int flags;
>>> + int index;
>>> + uint32_t arch_mask;
>>> +} QEMUOption;
>>> +
>>
>> Ugh.
>
> What do you mean? The real ugh! here is that it was created as a
> typedef. I can change the function to pass in just the index, but I
> don't know if we will have cases where the rest is needed.
Moving stuff out of the vl.c grabbag is cool. Moving stuff into the
sysemu.h grabbag is very uncool.
>> Is this minor improvement of vl.c really worth the headaches elsewhere?
>
> vl.c as it is today is gross and un-maintainable. This patch gets rid of
> a lot of the ugly #ifdefs and makes the code easier to read and maintain.
I'm not arguing against your patch, just trying to help making it even
better.
[Qemu-devel] [PATCH 13/16] Move daemonize handling to OS specific files, Jes . Sorensen, 2010/06/03
[Qemu-devel] [PATCH 15/16] Move line-buffering setup to OS specific files., Jes . Sorensen, 2010/06/03
[Qemu-devel] [PATCH 16/16] Move set_proc_name() to OS specific files., Jes . Sorensen, 2010/06/03
Re: [Qemu-devel] [PATCH 00/16] clean up vl.c code, Markus Armbruster, 2010/06/04