[PATCH xserver 10/19] loader: Remove silly "unspecified" version handling

[Emil Velikov]

On 23 January 2017 at 19:32, Adam Jackson <ajax at redhat.com> wrote:
> Nobody who is using this functionality is ever not specifying a major
> version, which makes sense. If you don't care about a minor version,
> that's equivalent to saying you require minor >= 0, so just say so;
> likewise patch level.
>
> Likewise nobody using this functionality is ever not specifying an ABI
> class.
>
>From a person how's native language extensively uses double negatives
- the ones used above are a bit confusing.

> Signed-off-by: Adam Jackson <ajax at redhat.com>
> ---
>  hw/xfree86/common/xf86Module.h |  6 ------
>  hw/xfree86/doc/ddxDesign.xml   | 20 +++++++++---------
>  hw/xfree86/loader/loadmod.c    | 47 +++++++++++++++++-------------------------
>  3 files changed, 29 insertions(+), 44 deletions(-)
>
> diff --git a/hw/xfree86/common/xf86Module.h b/hw/xfree86/common/xf86Module.h
> index ff0e23e..faea07f 100644
> --- a/hw/xfree86/common/xf86Module.h
> +++ b/hw/xfree86/common/xf86Module.h
> @@ -141,12 +141,6 @@ typedef struct {
>      const char *moduleclass;    /* module class */
>  } XF86ModReqInfo;
>
> -/* values to indicate unspecified fields in XF86ModReqInfo. */
> -#define MAJOR_UNSPEC           0xFF
> -#define MINOR_UNSPEC           0xFF
> -#define PATCH_UNSPEC           0xFFFF
> -#define ABI_VERS_UNSPEC                0xFFFFFFFF
> -
>  #define MODULE_VERSION_NUMERIC(maj, min, patch) \
>         ((((maj) & 0xFF) << 24) | (((min) & 0xFF) << 16) | (patch & 0xFFFF))
>  #define GET_MODULE_MAJOR_VERSION(vers) (((vers) >> 24) & 0xFF)
> diff --git a/hw/xfree86/doc/ddxDesign.xml b/hw/xfree86/doc/ddxDesign.xml
> index 2ff2894..05ee042 100644
> --- a/hw/xfree86/doc/ddxDesign.xml
> +++ b/hw/xfree86/doc/ddxDesign.xml
> @@ -5293,12 +5293,12 @@ XFree86 common layer.
>                   as follows:
>                       <programlisting>
>  typedef struct {
> -       CARD8        majorversion;  /* MAJOR_UNSPEC */
> -       CARD8        minorversion;  /* MINOR_UNSPEC */
> -       CARD16       patchlevel;    /* PATCH_UNSPEC */
> -       const char * abiclass;      /* ABI_CLASS_NONE */
> -       CARD32       abiversion;    /* ABI_VERS_UNSPEC */
> -       const char * moduleclass;   /* MOD_CLASS_NONE */
> +       CARD8        majorversion;
> +       CARD8        minorversion;
> +       CARD16       patchlevel;
> +       const char * abiclass;
> +       CARD32       abiversion;
> +       const char * moduleclass;
>  } XF86ModReqInfo;
>                         </programlisting>
>
> @@ -5323,8 +5323,8 @@ typedef struct {
>                                    The module's minor version must be
>                                    no less than this value.  This
>                                    comparison is only made if
> -                                  <structfield>majorversion</structfield> is
> -                                  specified and matches.
> +                                  <structfield>majorversion</structfield>
> +                                   matches.
>                             </para></listitem></varlistentry>
>
>                         <varlistentry>
> @@ -5333,8 +5333,8 @@ typedef struct {
>                                    The module's patchlevel must be no
>                                    less than this value.  This comparison
>                                    is only made if
> -                                  <structfield>minorversion</structfield> is
> -                                  specified and matches.
> +                                  <structfield>minorversion</structfield>
> +                                  matches.
>                             </para></listitem></varlistentry>
>
>                         <varlistentry>
> diff --git a/hw/xfree86/loader/loadmod.c b/hw/xfree86/loader/loadmod.c
> index 85689be..528cc88 100644
> --- a/hw/xfree86/loader/loadmod.c
> +++ b/hw/xfree86/loader/loadmod.c
> @@ -617,32 +617,24 @@ CheckVersion(const char *module, XF86ModuleVersionInfo * data,
>
>      /* Check against requirements that the caller has specified */
>      if (req) {
> -        if (req->majorversion != MAJOR_UNSPEC) {
> -            if (data->majorversion != req->majorversion) {
> -                LogMessageVerb(X_WARNING, 2, "%s: module major version (%d) "
> -                               "doesn't match required major version (%d)\n",
> -                               module, data->majorversion, req->majorversion);
> -                return FALSE;
> -            }
> -            else if (req->minorversion != MINOR_UNSPEC) {
> -                if (data->minorversion < req->minorversion) {
> -                    LogMessageVerb(X_WARNING, 2, "%s: module minor version "
> -                                   "(%d) is less than the required minor "
> -                                   "version (%d)\n", module,
> -                                  data->minorversion, req->minorversion);
> -                    return FALSE;
> -                }
> -                else if (data->minorversion == req->minorversion &&
> -                         req->patchlevel != PATCH_UNSPEC) {
> -                    if (data->patchlevel < req->patchlevel) {
> -                        LogMessageVerb(X_WARNING, 2, "%sL module patch level "
> -                                       "(%d) is less than the required patch "
> -                                       "level (%d)\n", module, data->patchlevel,
> -                                       req->patchlevel);
> -                        return FALSE;
> -                    }
> -                }
> -            }
> +        if (data->majorversion != req->majorversion) {
> +            LogMessageVerb(X_WARNING, 2, "%s: module major version (%d) "
> +                           "doesn't match required major version (%d)\n",
> +                           module, data->majorversion, req->majorversion);
> +            return FALSE;
> +        }
> +        else if (data->minorversion < req->minorversion) {
> +            LogMessageVerb(X_WARNING, 2, "%s: module minor version (%d) is "
> +                          "less than the required minor version (%d)\n",
> +                          module, data->minorversion, req->minorversion);
> +            return FALSE;
> +        }
> +        else if (data->minorversion == req->minorversion &&
The above three can get a s/else if/if/.
But even without it (or the commit nitpick) things look a lot better.

Reviewed-by: Emil Velikov <emil.l.velikov at gmail.com>

-Emil