Re: [PATCH 3/3 v2] Handle Windows CE and rewrite NT version handling.

[Bruno Haible]

Paolo Bonzini wrote:
> * lib/uname.c: Handle Windows CE and its processor types.  Remove
> code for processors never supported by Windows 95/98/ME.  Rewrite
> conversion of NT version numbers to product names.

The large part, the rewrite of the NT version numbers conversion,
looks like this after the other patches are applied. (Using 'const'
and 'unsigned int' where appropriate. Not using "WIN" as a praising
abbreviation for Windows. Using GNU style placement of braces.)

*** lib/uname.c.orig    2009-10-04 11:28:37.000000000 +0200
--- lib/uname.c 2009-10-04 11:24:07.000000000 +0200
***************
*** 129,172 ****
    if (version.dwPlatformId == VER_PLATFORM_WIN32_NT)
      {
        /* Windows NT or newer.  */
!       if (version.dwMajorVersion <= 4)
!       sprintf (buf->release, "Windows NT %u.%u",
!                (unsigned int) version.dwMajorVersion,
!                (unsigned int) version.dwMinorVersion);
!       else if (version.dwMajorVersion == 5)
!       switch (version.dwMinorVersion)
!         {
!         case 0:
!           strcpy (buf->release, "Windows 2000");
!           break;
!         case 1:
!           strcpy (buf->release, "Windows XP");
!           break;
!         case 2:
!           strcpy (buf->release, "Windows Server 2003");
!           break;
!         default:
!           strcpy (buf->release, "Windows");
!           break;
!         }
!       else if (version.dwMajorVersion == 6)
        {
!         if (have_versionex && versionex.wProductType != VER_NT_WORKSTATION)
!           strcpy (buf->release, "Windows Server 2008");
!         else
!           switch (version.dwMinorVersion)
!             {
!             case 0:
!               strcpy (buf->release, "Windows Vista");
!               break;
!             case 1:
!             default: /* versions not yet known */
!               strcpy (buf->release, "Windows 7");
!               break;
!             }
        }
        else
!       strcpy (buf->release, "Windows");
      }
    else if (version.dwPlatformId == VER_PLATFORM_WIN32_CE)
      {
--- 129,178 ----
    if (version.dwPlatformId == VER_PLATFORM_WIN32_NT)
      {
        /* Windows NT or newer.  */
!       struct windows_version
!       {
!         int major;
!         int minor;
!         unsigned int server_offset;
!         const char *name;
!       };
!       #define VERSION1(major, minor, name) \
!       { major, minor, 0, name }
!       #define VERSION2(major, minor, workstation, server) \
!       { major, minor, sizeof workstation, workstation "\0" server }
!       const struct windows_version versions[] =
!       {
!         VERSION2 (3, -1, "Windows NT Workstation", "Windows NT Server"),
!         VERSION2 (4, -1, "Windows NT Workstation", "Windows NT Server"),
!         VERSION1 (5, 0, "Windows 2000"),
!         VERSION1 (5, 1, "Windows XP"),
!         VERSION1 (5, 2, "Windows Server 2003"),
!         VERSION2 (6, 0, "Windows Vista", "Windows Server 2008"),
!         VERSION2 (6, 1, "Windows 7", "Windows Server 2008 R2"),
!         VERSION2 (-1, -1, "Windows", "Windows Server")
!       };
!       const struct windows_version *v;
!       const char *base;
!       unsigned int i;
! 
!       for (i = 0; i < sizeof (versions) / sizeof (versions[0]); i++)
        {
!         v = &versions[i];
!         if ((v->major == version.dwMajorVersion || v->major == -1)
!             && (v->minor == version.dwMinorVersion || v->minor == -1))
!           break;
        }
+       if (have_versionex && version.wProductType != VER_NT_WORKSTATION)
+       base = v->name + v->server_offset;
+       else
+       base = v->name;
+       if (v->major == -1 || v->minor == -1)
+       sprintf (buf->release, "%s %u.%u",
+                base,
+                (unsigned int) version.dwMajorVersion,
+                (unsigned int) version.dwMinorVersion);
        else
!       strcpy (buf->release, base);
      }
    else if (version.dwPlatformId == VER_PLATFORM_WIN32_CE)
      {


Now, looking at the two sides of this patch, honestly I don't find the latter
side more readable. The first code is immediately, it's clear what it does.
For the second code I find myself asking:
  "What if the loop over i falls off the end of the array?"
  "Why are two strings concatenated with a "\0" separator, rather than
   put separately in the struct?"
  "Why is version.wProductType being used in cases like (5,1) which don't
   have a distinction between workstation and server?"

Table-driven code is more readable if there is only one type of table
entries. But when you have 3 different types of table rows, with only
8 table rows in all...

I won't object if you apply this patch, but I wouldn't apply it by myself.

Bruno