[PATCH xserver 03/11] modesetting: Simplify mst path blob parsing

[Daniel Martin]

On 7 November 2017 at 14:06, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> On 7 November 2017 at 09:38, Daniel Martin <consume.noise at gmail.com> wrote:
>> The kernel guarantees that the MST path property blob of a connector
>> has a certain format and this property is immutable - can't be changed
>> from user space. With that in mind, reduce the parsing code to a minimum
>> matching the format criteria.
>>
>> (Preparation to fold the parsing into output name creation function.)
>>
>> Signed-off-by: Daniel Martin <consume.noise at gmail.com>
>> ---
>>  hw/xfree86/drivers/modesetting/drmmode_display.c | 41 +++++++++---------------
>>  1 file changed, 15 insertions(+), 26 deletions(-)
>>
>> diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c
>> index 404bb1dc2..7ff55ef24 100644
>> --- a/hw/xfree86/drivers/modesetting/drmmode_display.c
>> +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
>> @@ -1636,48 +1636,37 @@ static xf86OutputPtr find_output(ScrnInfoPtr pScrn, int id)
>>      return NULL;
>>  }
>>
>> -static int parse_path_blob(drmModePropertyBlobPtr path_blob, int *conn_base_id, char **path)
>> +static Bool
>> +parse_path_blob(drmModePropertyBlobPtr path_blob,
>> +        int *conn_base_id, char **extra_path)
>>  {
>> -    char *conn;
>> -    char conn_id[5];
>> -    int id, len;
>> -    char *blob_data;
>> +    char *colon, *dash;
>>
>>      if (!path_blob)
>> -        return -1;
>> +        return FALSE;
>>
>> -    blob_data = path_blob->data;
>> -    /* we only handle MST paths for now */
>> -    if (strncmp(blob_data, "mst:", 4))
>> -        return -1;
>> +    colon = strchr(path_blob->data, ':');
>> +    dash = strchr(path_blob->data, '-');
>>
> Won't the removal of "mst" cause false positives - surely there are
> !MST blobs, right?
> If it's safe a comment will help a lot.

Yes, this property blob is always prefixed with "mst:" and I've added
a comment to the code.
(By skipping the prefix check, we are safe for the future, i.e. if the
prefix becomes mst3k. ;) )

>> -    conn = strchr(blob_data + 4, '-');
>> -    if (!conn)
>> -        return -1;
>> -    len = conn - (blob_data + 4);
>> -    if (len + 1> 5)
>> -        return -1;
>> -    memcpy(conn_id, blob_data + 4, len);
>> -    conn_id[len + 1] = '\0';
>> -    id = strtoul(conn_id, NULL, 10);
>> +    if (colon && dash) {
>> +        *conn_base_id = strtoul(colon + 1, NULL, 10);
>> +        *extra_path = dash + 1;
>>
>> -    *conn_base_id = id;
>> +        return TRUE;
>> +    }
>>
>> -    *path = conn + 1;
>> -    return 0;
>> +    return FALSE;
>>  }
>>
>>  static void
>>  drmmode_create_name(ScrnInfoPtr pScrn, drmModeConnectorPtr koutput, char *name,
>> -                   drmModePropertyBlobPtr path_blob)
>> +        drmModePropertyBlobPtr path_blob)
> Unrelated whitespace change.
>
>>  {
>> -    int ret;
>>      char *extra_path;
>>      int conn_id;
>>      xf86OutputPtr output;
>>
>> -    ret = parse_path_blob(path_blob, &conn_id, &extra_path);
>> -    if (ret == -1)
>> +    if (!parse_path_blob(path_blob, &conn_id, &extra_path))
> The function name doesn't imply a boolean return value, so I'd stick with int.

I've reverted the unnecessary changes.