Re: [PATCH 1/2] mountmgr: Create devices and registry entries for serial ports.

[Hugh McMaster]

Hi Alex,

Just a few things I noticed.

On Monday, 17 April 2017 2:23 PM, Alex Henrie wrote:
> +    /* create registry entry */
> +    sprintfW( reg_value, reg_value_format, n );
> +    RegSetValueExW( windows_ports_key, nt_name.Buffer, 0, REG_SZ,
> +                    (BYTE *)reg_value, strlenW( reg_value ) * sizeof(WCHAR) );
> +
> +    return TRUE;
> +}

You need to pass RegSetValueExW() the string length plus the null-terminating character.
> +/* find and create serial or parallel ports */
> +static void create_port_devices( DRIVER_OBJECT *driver )
> +{
> +    static const char *serial_search_paths[] = {
> +#ifdef linux
> +        "/dev/ttyS%u",
> +        "/dev/ttyUSB%u",
> +#elif defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
> +        "/dev/cuau%u",
> +#elif defined(__DragonFly__)
> +        "/dev/cuaa%u",
> +#else
> +        "",
> +#endif
> +    };
> +    static const WCHAR serialcomm_keyW[] = {'H','A','R','D','W','A','R','E','\\',
> +                                            'D','E','V','I','C','E','M','A','P','\\',
> +                                            'S','E','R','I','A','L','C','O','M','M',0};
> +    const char **search_paths;
> +    const WCHAR *windows_ports_key_name;
> +    char *dosdevices_path, *p;
> +    HKEY wine_ports_key = NULL, windows_ports_key = NULL;
> +    char unix_path[256];
> +    int i, j, n = 1;
> +
> +    if (!(dosdevices_path = get_dosdevices_path( &p )))
> +        return;
> +
> +    if (driver == serial_driver)
> +    {
> +        p[0] = 'c';
> +        p[1] = 'o';
> +        p[2] = 'm';
> +        search_paths = serial_search_paths;
> +        windows_ports_key_name = serialcomm_keyW;
> +    }
> +    else
> +    {
> +        /* TODO: support parallel ports */
> +    }
> +    p += 3;
> +
> +    RegOpenKeyW( HKEY_LOCAL_MACHINE, ports_keyW, &wine_ports_key );
> +    RegOpenKeyW( HKEY_LOCAL_MACHINE, windows_ports_key_name, &windows_ports_key );

Please use RegOpenKeyExW() and check for any failures from these functions.
> +    /* remove old symlinks */
> +    for (n = 1; n <= MAX_PORTS; n++)
> +    {
> +        sprintf( p, "%u", n );
> +        if (unlink( dosdevices_path ) != 0 && !errno)
> +            break;
> +    }
> +
> +    /* look for ports in the usual places */
> +    n = 1;
> +    for (i = 0; i < sizeof(search_paths)/sizeof(search_paths[0]); i++)

This won't do what you expect. sizeof(search_paths)/sizeof(search_paths[0]) is 1.
You need to pass in the number of elements in the array. Also, it's better to avoid making this calculation
during each loop.

Hugh

[PATCH 1/2] mountmgr: Create devices and registry entries for serial ports.

[Alex Henrie]

2017-04-18 7:00 GMT-06:00 Hugh McMaster <hugh.mcmaster(a)outlook.com>:
> Hi Alex,
>
> Just a few things I noticed.
>
> On Monday, 17 April 2017 2:23 PM, Alex Henrie wrote:
> > +    /* create registry entry */
> > +    sprintfW( reg_value, reg_value_format, n );
> > +    RegSetValueExW( windows_ports_key, nt_name.Buffer, 0, REG_SZ,
> > +                    (BYTE *)reg_value, strlenW( reg_value ) * sizeof(WCHAR) );
> > +
> > +    return TRUE;
> > +}
>
> You need to pass RegSetValueExW() the string length plus the null-terminating character.

Good catch.
> > +/* find and create serial or parallel ports */
> > +static void create_port_devices( DRIVER_OBJECT *driver )
> > +{
> > +    static const char *serial_search_paths[] = {
> > +#ifdef linux
> > +        "/dev/ttyS%u",
> > +        "/dev/ttyUSB%u",
> > +#elif defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
> > +        "/dev/cuau%u",
> > +#elif defined(__DragonFly__)
> > +        "/dev/cuaa%u",
> > +#else
> > +        "",
> > +#endif
> > +    };
> > +    static const WCHAR serialcomm_keyW[] = {'H','A','R','D','W','A','R','E','\\',
> > +                                            'D','E','V','I','C','E','M','A','P','\\',
> > +                                            'S','E','R','I','A','L','C','O','M','M',0};
> > +    const char **search_paths;
> > +    const WCHAR *windows_ports_key_name;
> > +    char *dosdevices_path, *p;
> > +    HKEY wine_ports_key = NULL, windows_ports_key = NULL;
> > +    char unix_path[256];
> > +    int i, j, n = 1;
> > +
> > +    if (!(dosdevices_path = get_dosdevices_path( &p )))
> > +        return;
> > +
> > +    if (driver == serial_driver)
> > +    {
> > +        p[0] = 'c';
> > +        p[1] = 'o';
> > +        p[2] = 'm';
> > +        search_paths = serial_search_paths;
> > +        windows_ports_key_name = serialcomm_keyW;
> > +    }
> > +    else
> > +    {
> > +        /* TODO: support parallel ports */
> > +    }
> > +    p += 3;
> > +
> > +    RegOpenKeyW( HKEY_LOCAL_MACHINE, ports_keyW, &wine_ports_key );
> > +    RegOpenKeyW( HKEY_LOCAL_MACHINE, windows_ports_key_name, &windows_ports_key );
>
> Please use RegOpenKeyExW() and check for any failures from these functions.

If these functions fail, wine_ports_key and windows_ports_key will
still be NULL, and symlinks will be created for the detected devices
even though registry entries will not. I think that is desired
behavior.
> > +    /* remove old symlinks */
> > +    for (n = 1; n <= MAX_PORTS; n++)
> > +    {
> > +        sprintf( p, "%u", n );
> > +        if (unlink( dosdevices_path ) != 0 && !errno)
> > +            break;
> > +    }
> > +
> > +    /* look for ports in the usual places */
> > +    n = 1;
> > +    for (i = 0; i < sizeof(search_paths)/sizeof(search_paths[0]); i++)
>
> This won't do what you expect. sizeof(search_paths)/sizeof(search_paths[0]) is 1.
> You need to pass in the number of elements in the array. Also, it's better to avoid making this calculation
> during each loop.

Good catch.

Thanks for the feedback, that was very helpful!

-Alex