[10/16] Introduce OS specific cmdline argument handling and move SMB arg to os-posix.c

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Introduce OS specific cmdline argument handling by calling
os_parse_cmd_args() at the end of switch() statement.

In addition move SMB argument to os-posix.c

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 os-posix.c |   34 ++++++++++++++++++++++++++++++++++
 os-win32.c |   22 ++++++++++++++++++++++
 sysemu.h   |    9 +++++++++
 vl.c       |   15 ++-------------
 4 files changed, 67 insertions(+), 13 deletions(-)

On 06/03/2010 09:48 AM, Jes.Sorensen@redhat.com wrote:
> +/*
> + * 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
> +};

There's no header file you can put this in?  Or invent to put this in?
Cause this is really kinda gross...

> +
> +/*
> + * 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
> +    default:
> +        ret = -1;
> +    }
> +    return ret;
> +}

Why have a return value at all...

> +            default:
> +                os_parse_cmd_args(popt, optarg);

... if you're going to ignore the results?


r~

On 06/03/10 22:58, Richard Henderson wrote:
> On 06/03/2010 09:48 AM, Jes.Sorensen@redhat.com wrote:
>> +/*
>> + * 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
>> +};
> 
> There's no header file you can put this in?  Or invent to put this in?
> Cause this is really kinda gross...
> 

The problem is that it requires qemu-options.h to be included, which
isn't included per default for all the files. If I put it into sysemu.h
at least it's going to require making every .c file build with those flags.

I agree it's gross, but I am not sure what would be a better solution.

>> +    default:
>> +        ret = -1;
>> +    }
>> +    return ret;
>> +}
> 
> Why have a return value at all...
> 
>> +            default:
>> +                os_parse_cmd_args(popt, optarg);
> 
> ... if you're going to ignore the results?

I was trying to make it forward looking, but yeah we can just kill that.

Cheers,
Jes

Jes.Sorensen@redhat.com writes:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> Introduce OS specific cmdline argument handling by calling
> os_parse_cmd_args() at the end of switch() statement.
>
> In addition move SMB argument to os-posix.c
>
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  os-posix.c |   34 ++++++++++++++++++++++++++++++++++
>  os-win32.c |   22 ++++++++++++++++++++++
>  sysemu.h   |    9 +++++++++
>  vl.c       |   15 ++-------------
>  4 files changed, 67 insertions(+), 13 deletions(-)
>
> diff --git a/os-posix.c b/os-posix.c
> index 621ad06..66f2bf5 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -33,6 +33,7 @@
>  /* Needed early for CONFIG_BSD etc. */
>  #include "config-host.h"
>  #include "sysemu.h"
> +#include "net/slirp.h"
>  
>  void os_setup_early_signal_handling(void)
>  {
> @@ -130,3 +131,36 @@ char *os_find_datadir(const char *argv0)
>  }
>  #undef SHARE_SUFFIX
>  #undef BUILD_SUFFIX
> +
> +/*
> + * 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
> +};
> +
> +/*
> + * 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?

> +    default:
> +        ret = -1;
> +    }
> +    return ret;
> +}
> diff --git a/os-win32.c b/os-win32.c
> index 1758538..a311a90 100644
> --- a/os-win32.c
> +++ b/os-win32.c
> @@ -204,3 +204,25 @@ char *os_find_datadir(const char *argv0)
>      }
>      return NULL;
>  }
> +
> +/*
> + * 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.

> +
> +/*
> + * Parse OS specific command line options.
> + * return 0 if option handled, -1 otherwise
> + */
> +int os_parse_cmd_args(const QEMUOption *popt, const char *optarg)
> +{
> +    return -1;
> +}
> diff --git a/sysemu.h b/sysemu.h
> index 72f3734..08ec323 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -79,9 +79,18 @@ int qemu_loadvm_state(QEMUFile *f);
>  /* SLIRP */
>  void do_info_slirp(Monitor *mon);
>  
> +/* 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.

>  /* OS specific functions */
>  void os_setup_early_signal_handling(void);
>  char *os_find_datadir(const char *argv0);
> +int os_parse_cmd_args(const QEMUOption *popt, const char *optarg);
>  
>  typedef enum DisplayType
>  {
> diff --git a/vl.c b/vl.c
> index 7f22733..838e109 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1909,13 +1909,6 @@ enum {
>  #undef GEN_DOCS
>  };
>  
> -typedef struct QEMUOption {
> -    const char *name;
> -    int flags;
> -    int index;
> -    uint32_t arch_mask;
> -} QEMUOption;
> -
>  static const QEMUOption qemu_options[] = {
>      { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL },
>  #define DEF(option, opt_arg, opt_enum, opt_help, arch_mask)     \
> @@ -2624,12 +2617,6 @@ int main(int argc, char **argv, char **envp)
>              case QEMU_OPTION_bootp:
>                  legacy_bootp_filename = optarg;
>                  break;
> -#ifndef _WIN32
> -            case QEMU_OPTION_smb:
> -                if (net_slirp_smb(optarg) < 0)
> -                    exit(1);
> -                break;
> -#endif
>              case QEMU_OPTION_redir:
>                  if (net_slirp_redir(optarg) < 0)
>                      exit(1);
> @@ -3126,6 +3113,8 @@ int main(int argc, char **argv, char **envp)
>                      fclose(fp);
>                      break;
>                  }
> +            default:
> +                os_parse_cmd_args(popt, optarg);
>              }
>          }
>      }

Is this minor improvement of vl.c really worth the headaches elsewhere?

On 06/04/10 10:15, Markus Armbruster wrote:
> Jes.Sorensen@redhat.com 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

>> +/*
>> + * 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.

> 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.

Jes

>>> +/*
>>> + * 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?

I don't think it's particularly gross.  At least you don't have two 
files to keep in sync.

You could rename qemu-options.h to qemu-options.def, and make a real 
header file with the typedef and the enum.  Then include the header from 
vl.c and os-*.c.

BTW from Fedora 11 and newer you can easily build QEMU with a cross 
compiler.  (Running it is a bit harder).  These packages should suffice:

mingw32-w32api mingw32-cpp mingw32-termcap mingw32-runtime
mingw32-binutils mingw32-filesystem mingw32-SDL mingw32-gcc
mingw32-zlib

and you need to configure it with "--cross-prefix=i686-pc-mingw32-" 
(trailing dash included!).

Paolo

On 06/04/10 12:39, Paolo Bonzini wrote:
>>> 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?
> 
> I don't think it's particularly gross.  At least you don't have two
> files to keep in sync.
> 
> You could rename qemu-options.h to qemu-options.def, and make a real
> header file with the typedef and the enum.  Then include the header from
> vl.c and os-*.c.

I like this idea. I was looking at a qemu-options-somethingelse.h but I
couldn't really find any appropriate name for it. Renaming the current
qemu-options.h to qemu-options.def seems correct IMHO as it's not really
a header file that can be included into code by itself.

> BTW from Fedora 11 and newer you can easily build QEMU with a cross
> compiler.  (Running it is a bit harder).  These packages should suffice:
> 
> mingw32-w32api mingw32-cpp mingw32-termcap mingw32-runtime
> mingw32-binutils mingw32-filesystem mingw32-SDL mingw32-gcc
> mingw32-zlib
> 
> and you need to configure it with "--cross-prefix=i686-pc-mingw32-"
> (trailing dash included!).

Thanks! I'll have to take a look at this.

Cheers,
Jes

Jes Sorensen <Jes.Sorensen@redhat.com> writes:

> On 06/04/10 10:15, Markus Armbruster wrote:
>> Jes.Sorensen@redhat.com 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.

On 06/04/10 14:04, Markus Armbruster wrote:
> Jes Sorensen <Jes.Sorensen@redhat.com> writes:
> 
>> On 06/04/10 10:15, Markus Armbruster wrote:
>> 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.

I agree, I have a new version of the patch coming up shortly. I just
want to apply Paolo's idea of moving qemu-options.h around a bit.

>>> 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.

I was gathering that, and your input is much appreciated.

Cheers,
Jes

On 06/03/2010 11:47 PM, Jes Sorensen wrote:
> On 06/03/10 22:58, Richard Henderson wrote:
>> On 06/03/2010 09:48 AM, Jes.Sorensen@redhat.com wrote:
>>> +/*
>>> + * 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
>>> +};
>>
>> There's no header file you can put this in?  Or invent to put this in?
>> Cause this is really kinda gross...
>>
> 
> The problem is that it requires qemu-options.h to be included, which
> isn't included per default for all the files. If I put it into sysemu.h
> at least it's going to require making every .c file build with those flags.
> 
> I agree it's gross, but I am not sure what would be a better solution.

One possible solution is to put this whole block in "qemu-options-enum.h"
(or whatever) and include that in the three places that you have this block.


r~

On 06/04/10 16:49, Richard Henderson wrote:
> On 06/03/2010 11:47 PM, Jes Sorensen wrote:
>> The problem is that it requires qemu-options.h to be included, which
>> isn't included per default for all the files. If I put it into sysemu.h
>> at least it's going to require making every .c file build with those flags.
>>
>> I agree it's gross, but I am not sure what would be a better solution.
> 
> One possible solution is to put this whole block in "qemu-options-enum.h"
> (or whatever) and include that in the three places that you have this block.

I ended up moving qemu-options.h to qemu-options.def and then creating a
proper qemu-options.h per Paolo's suggestion.

Let me know what you think of the v2 patch set, minus the duplicates
some idiot posted because he forgot to remove the Emacs backup files :)

Cheers,
Jes