RE: [PATCH v3 03/10] reg: Add system error printing function

[Hugh McMaster]

On Saturday, 8 Nov 2014 11:26:44 +0100, Jonathan Vollebregt wrote:

+static void reg_print_error(LSTATUS error_code)
+{
+    switch (error_code)
+    {
+        case ERROR_SUCCESS:
+            return;
+        case ERROR_BAD_COMMAND:
+            reg_message(STRING_INVALID_CMDLINE);
+            return;
+        default:
+        {
+            static const WCHAR error_string[] = {'%','0','5','d',':',' ','%','s',0};
+            WCHAR *message = NULL;
+            FormatMessageW(FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_ALLOCATE_BUFFER, NULL,
+                error_code, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), (WCHAR *)&message, 0, NULL);
+
+            reg_message(STRING_ERROR);
+            reg_printfW(error_string, error_code, message);
+            LocalFree(message);
+            return;
+        }
+    }
+}This seems like a step backwards to me. It would be much better to add the required functionality -- e.g. FormatMessage() -- and fix reg.exe printf-style functions in one go.

See http://source.winehq.org/patches/data/107578 for what I mean.

Also, mixing reg_message() and reg_printfW() just seems wrong. A clear resource string incorporating your formatting above would suffice, I expect.

--
Hugh McMaster

[PATCH v3 03/10] reg: Add system error printing function

[Jonathan Vollebregt]

> This seems like a step backwards to me. It would be much better to add the required functionality -- e.g. FormatMessage() -- and fix reg.exe printf-style functions in one go.

I'd still have to use reg_print_error as is because it's there to get 
the error string from standard error codes like ERROR_INVALID_DATA not 
to actually format the output string (FORMAT_MESSAGE_FROM_SYSTEM vs 
FORMAT_MESSAGE_FROM_STRING)

I like splitting the writing part into output_write (That might come in 
handy for printing long strings in reg query if we can't find a way to 
make reg_printfW variable size)

Giving reg_message extra arguments is a good idea, but I'm not sure if 
we're supposed to have format specifiers in LoadString resources.

I'm not sure whether having output_vprintf is really an advantage over 
just calling reg_printf since it uses different format specifiers than 
printf and basically does the same thing (Minus the variable size issue 
of course)

[PATCH v3 03/10] reg: Add system error printing function

[Hugh McMaster]

On Monday, 10 Nov 2014 14:46:49 +0100, Jonathan Vollebregt wrote:
> I'd still have to use reg_print_error as is because it's there to get 
> the error string from standard error codes like ERROR_INVALID_DATA not 
> to actually format the output string (FORMAT_MESSAGE_FROM_SYSTEM vs 
> FORMAT_MESSAGE_FROM_STRING)
>
> Giving reg_message extra arguments is a good idea, but I'm not sure if 
> we're supposed to have format specifiers in LoadString resources.
>
> I'm not sure whether having output_vprintf is really an advantage over 
> just calling reg_printf since it uses different format specifiers than 
> printf and basically does the same thing (Minus the variable size issue 
> of course)

Format specifiers are perfectly valid in resource strings. See programs/regsvr32/regsvr32.rc as one example.

Not having to deal with variable memory allocation is the obvious advantage of using output_vprintf(). My personal choice would be to convert static const WCHAR nonnumber[] and static const WCHAR unhandled[] to resource strings and using reg_message(). These messages really should be translated anyway.

That leaves the three static const WCHAR stubW[] arrays to consider. Personally, I would remove them. They serve no real purpose and native reg doesn't have anything like them.

As to the getting the error message from the system table, I came up with two possibilities.

static void __cdecl reg_printfW(const WCHAR* msg, ...)
{
    __ms_va_list va_args;

    __ms_va_start(va_args, msg);
    output_vprintf(msg, va_args); 
    __ms_va_end(va_args);
}

static void reg_print_error(LSTATUS error_code)
{
    WCHAR *str, *buffer;
    int len, extra_char = 8;
    static const WCHAR error_string1[] = {'%','1','!','*','.','*','d','!',':',' ','%','3',0};
    static const WCHAR error_string2[] = {'%','0','5','d',':',' ','%','s',0};

    len = FormatMessageW(FORMAT_MESSAGE_FROM_SYSTEM|FORMAT_MESSAGE_ALLOCATE_BUFFER,
                         NULL, error_code, 0, (LPWSTR)&str, 0, NULL);
    if (len == 0 && GetLastError() != NO_ERROR)
    {
        WINE_FIXME("Could not format system string: le=%u\n", GetLastError());
        return;
    }
    reg_printfW(error_string1, 0, 5, error_code, str); /* Option 1 - fully automated */
    /* Option 2 */
    buffer = HeapAlloc(GetProcessHeap(), 0, (len + extra_char) * sizeof(WCHAR));
    if (buffer)
    {
        sprintfW(buffer, error_string2, error_code, str);
        output_write(buffer, strlenW(buffer));
        HeapFree(GetProcessHeap(), 0, buffer);
    }
    LocalFree(str);
}

[PATCH v3 03/10] reg: Add system error printing function

[Jonathan Vollebregt]

The reg patches have been languishing on the list for a while now.

As Hugh's example showed format specifiers are the preferred way of 
doing rc strings, it just seems a shame we have to duplicate so much.

Can we have 107580 committed so I can rebase on it?

I still wish there were a better way to deal with the fixed-size 
reg_printfW output but it looks like there is none.