Re: [PATCH 2/2] winealsa: implement GetPropValue and return device path (try 2)

[Andrew Eikum]

Looking pretty good. Couple notes below.

On Wed, Nov 19, 2014 at 02:35:39PM +0000, Mark Harmstone wrote:
> diff --git a/dlls/winealsa.drv/mmdevdrv.c b/dlls/winealsa.drv/mmdevdrv.c
> index 5ad72f9..c21fd0c 100644
> --- a/dlls/winealsa.drv/mmdevdrv.c
> +++ b/dlls/winealsa.drv/mmdevdrv.c
> @@ -3823,3 +3823,107 @@ HRESULT WINAPI AUDDRV_GetAudioSessionManager(IMMDevice *device,
>  return S_OK;
>
> }
> -
>
> +enum AudioDeviceConnectionType {
> - AudioDeviceConnectionType_Unknown = 0,
> - AudioDeviceConnectionType_PCI,
> - AudioDeviceConnectionType_USB
>
> +};
> -
>
> +HRESULT WINAPI AUDDRV_GetPropValue(GUID *guid, const PROPERTYKEY *prop, PROPVARIANT *out)
> +{
> - static const PROPERTYKEY devicepath_key = { /* undocumented? - {b3f8fa53-0004-438e-9003-51a46e139bfc},2 */
> -
>    {0xb3f8fa53, 0x0004, 0x438e, {0x90, 0x03, 0x51, 0xa4, 0x6e, 0x13, 0x9b, 0xfc}}, 2
> - };
> -
> - static const char dev_info[] = "/sys/class/sound/card%u/device/uevent";
> -

I'd prefer "const char *" and actually, is there any reason not to
just put this string straight in the sprintf?
> - TRACE("%s, (%s,%u), %p\n", wine_dbgstr_guid(guid), wine_dbgstr_guid(&prop->fmtid), prop->pid, out);
> -
> - if(IsEqualGUID(&prop->fmtid, &devicepath_key.fmtid) && prop->pid == devicepath_key.pid)
> - {

I think you can use IsEqualPropertyKey here.
> -
>    char name[256], uevent[MAX_PATH];
> -
>    EDataFlow flow;
> -
>    FILE *fuevent;
> -
>    int card, device;
> -
> -
>    if(!get_alsa_name_by_guid(guid, name, sizeof(name), &flow))
> -
>    {
> -
>        WARN("Unknown interface %s\n", debugstr_guid(guid));
> -
>        return E_NOINTERFACE;
> -
>    }
> -
> -
>    /* only implemented for identifiable devices, i.e. not "default" */
> -
>    if(!sscanf(name, "plughw:%u,%u", &card, &device))
> -
>        return E_NOTIMPL;
> -
> -
>    sprintf(uevent, dev_info, card);
> -
>    fuevent = fopen(uevent, "r");
> -
> -
>    if(fuevent){
> -
>        enum AudioDeviceConnectionType connection = AudioDeviceConnectionType_Unknown;
> -
>        USHORT vendor_id = 0, product_id = 0;
> -
>        char *line = NULL;
> -
> -
>        while (!feof(fuevent)) {
> -
>            char *val;
> -
>            size_t val_len, len = 0;
> -
> -
>            getline(&line, &len, fuevent);
> -

We should probably break on failure here.
> -
>            if((val = strchr(line, '='))) {
> -
>                val[0] = 0;
> -
>                val++;
> -
> -
>                val_len = strlen(val);
> -
>                if (val_len > 0 && val[val_len - 1] == '\n') { val[val_len - 1] = 0; }
> -
> -
>                if(!strcmp(line, "PCI_ID")){
> -
>                    connection = AudioDeviceConnectionType_PCI;
> -
>                    sscanf(val, "%hX:%hX", &vendor_id, &product_id);
> -
>                }else if (!strcmp(line, "DEVTYPE") && !strcmp(val,"usb_interface"))
> -
>                    connection = AudioDeviceConnectionType_USB;
> -
>                else if (!strcmp(line, "PRODUCT"))
> -
>                    sscanf(val, "%hx/%hx/", &vendor_id, &product_id);
> -
>            }
> -
>        }

Can we do anything sane on sscanf failure here? Maybe dump a WARN and
revert back to AudioDeviceConnectionType_Unknown. Is that preferable
to falling back on 0 for one or more of the IDs?

Andrew