Re: [PATCH v2 0/4] MR10604: setupapi, ntoskrnl: Implement CM_Get_Parent with device tree properties

[Elizabeth Figura (＠zfigura)]

It should be possible to test a lot more of this. 1/4 can be tested in setupapi. 2/4 can be tested in ntoskrnl, not just the CM APIs but also retrieving the devpkeys directly.

From 2/4:

```
 
+    /* Store parent device relationship for CM_Get_Parent. */
```

This comment is unnecessary.

```
+    if (parent_device)
```

Why are we checking this? When can it be NULL?

```
+    {
+        WCHAR parent_id[MAX_DEVICE_ID_LEN];
+        if (!get_device_instance_id( parent_device, parent_id ))
+        {
+            if (!SetupDiSetDevicePropertyW( set, &sp_device, &DEVPKEY_Device_Parent, DEVPROP_TYPE_STRING,
+                        (BYTE *)parent_id, (lstrlenW( parent_id ) + 1) * sizeof(WCHAR), 0 ))
```

wcslen()

```
+                WARN("Failed to set parent device property.\n");
```

Precedent notwithstanding, I would make this an ERR since it should never legitimately happen. Or just delete it.

```
+    DEVICE_OBJECT *original_parent = parent;
```

FDOs are supposed to pass through IRP_MN_QUERY_ID. You'll note that get_device_id() calls IoGetAttachedDevice() anyway, so this isn't actually accomplishing anything.


```
+    /* Store DEVPKEY_Device_Children on the parent and DEVPKEY_Device_Siblings on each child. */
```

That's more than what the commit message says. Mentioning it in the commit message would be an improvement, but splitting the DEVPKEY_Device_Children and DEVPKEY_Device_Siblings parts to their own commits would be even better.

As elsewhere the comment isn't really adding anything.

```
+    if (relations->Count > 0)
```

That's unnecessary; the for loop takes care of it.

```
+            if (!get_device_instance_id( relations->Objects[i], child_ids[valid_count] ))
+            {
+                multi_len += lstrlenW( child_ids[valid_count] ) + 1;
+                valid_count++;
+                if (valid_count >= 64) break;
+            }
```

Error handling here doesn't really make sense. We don't expect get_device_instance_id() to fail other than memory exhaustion, and if it does there's no point proceeding.

As elsewhere I would avoid lstr*() functions in favor of simple wcs*().

```
+                children_multi = HeapAlloc( GetProcessHeap(), HEAP_ZERO_MEMORY, multi_len * sizeof(WCHAR) );
+                if (children_multi)
```

You can use malloc(), which is simpler. I also wouldn't bother with checking for memory exhaustion for small allocations in new code.

```
+            /* Set DEVPKEY_Device_Siblings on each child (all other children). */
+            for (i = 0; i < valid_count; ++i)
```

You can move this out of the "if (valid_count > 0)" block and save a level of indentation.

3/4 should be implemented in cfgmgr32 on top of cfgmgr32 APIs, and setupapi should forward to it. We're in the middle of moving that right now.

4/4:

```
+    ok(ret == CR_SUCCESS || ret == CR_FAILURE || ret == CR_NO_SUCH_DEVNODE
+       || ret == CR_INVALID_DEVNODE,
```

When can all of these happen? Same with the check later.

```
+        ok(ret == CR_SUCCESS, "CM_Get_Device_IDA on parent: got %#lx\n", ret);
+        if (ret == CR_SUCCESS)
+            trace("Parent of ROOT\\LEGACY_BOGUS\\0000: %s\n", buffer);
```

You don't need that if(). What's the parent ID? Can it be checked?

```
+    SetupDiRemoveDevice(set, &device);
```

I would check that this succeeds.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/10604#note_135598