Re: [PATCH v8 0/5] MR2091: windows.media.speech: Initial Unixlib work.

[Zebediah Figura (＠zfigura)]

Some more comments from patch 5/5:
> +static inline vosk_handle vosk_recognizer_to_handle( VoskRecognizer *recognizer )
> +{
> - return (vosk_handle)(UINT_PTR)recognizer;
>
> +}
> -
>
> +static inline VoskRecognizer *vosk_recognizer_from_handle( vosk_handle handle )
> +{
> - return (VoskRecognizer *)(UINT_PTR)handle;
>
> +}

These don't need to be inline, do they?
> +static NTSTATUS vosk_create( void *args )

vosk uses vosk_ as a namespace prefix, so it seems that for readability it would be better to avoid anything beginning with that name in our code. That includes other cases in this file, e.g. 
"vosk_handle", "vosk_model_identifier".
> - if (!path || !locale || strlen(locale) < 4)
> -
>    return STATUS_UNSUCCESSFUL;

Do you need these checks?
> - if ((dir = opendir(path)) == NULL)
> -
>    return errno_to_status(errno);

Is there some legitimate scenario where this can fail? (And if not, shouldn't we print an ERR?) Is it worth pulling in errno_to_status() for this?
> -
>    if (!strcmp(dir_name = dirent->d_name, ".."))
> -
>        continue;
> -
> -
>    if (!strncmp(dir_name, vosk_model_identifier_small, ident_small_len))
> -
>        dir_name += ident_small_len;
> -
>    else if (!strncmp(dir_name, vosk_model_identifier, ident_len))
> -
>        dir_name += ident_len;
> -
>    else
> -
>        continue;

Isn't the ".." check redundant here?

Also, looking at this function, this looks like a very odd way to search for models. What's expected to follow the language and region, and why don't we care about it? Or, if the answer is "nothing", why are we doing an entire readdir() loop instead of just trying the 4 different possibilities?

Does Vosk really expect every API consumer to do this all manually? This looks extremely suspicious.
> -
>    if(!(model_path = malloc(path_len + 1 /* '/' */ + strlen(dirent->d_name) + 1)))
> -
>    {
> -
>        status = STATUS_MEMORY_NOT_ALLOCATED;
> -
>        break;
> -
>    }

You want STATUS_NO_MEMORY. STATUS_MEMORY_NOT_ALLOCATED is, from a brief search, supposed to be returned when trying to free memory that was not allocated.
> +static NTSTATUS find_model_by_locale( const char *locale, VoskModel **model )
> +{
> - NTSTATUS status = STATUS_UNSUCCESSFUL;
> - const char *suffix = NULL;
> - char *env, *path;
> -
> - TRACE("locale %s, model %p.\n", debugstr_a(locale), model);
> -
> - if (!model)
> -
>    return STATUS_UNSUCCESSFUL;

Why do you need this check?
> - if ((env = getenv("XDG_CACHE_HOME")))
> -
>    suffix = "/vosk";
> - else if ((env = getenv("HOME")))
> -
>    suffix = "/.cache/vosk";

It seems mildly clearer to add an "else" case here rather than checking for a previous initialized variable below.
> - if (suffix && (path = malloc(strlen(env) + strlen(suffix) + 1)))

And probably better to return STATUS_NO_MEMORY if malloc() fails.
> - if (!(recognizer = vosk_recognizer_new(model, params->sample_rate)))
> -
>    goto error;
> -
> - /* The model is kept alive inside the recognizer, so we can safely free our ref here. */
> - vosk_model_free(model);
> -
> - params->vosk_handle = vosk_recognizer_to_handle(recognizer);
> - return STATUS_SUCCESS;
> -
>
> +error:
> - if (model) vosk_model_free(model);
> - return STATUS_UNSUCCESSFUL;
>
> +}

This "error" label is only used in one place and only has one line of cleanup, making it look a little redundant. I also believe that "model" cannot be NULL in this case, can it?