Puzzling IPrincipal:UserId check in taskschd

[Francois Gouget]

So I'm a bit puzzled by this check in taskschd/task.c's 
write_principal():

hr = IPrincipal_get_UserId(principal, &bstr);
    if (hr == S_OK && lstrlenW(bstr))
    {
        hr = write_text_value(stream, L"UserId", bstr);

What's puzzling to me is the lstrlenW(bstr) check:

* write_principal() has no such check for Id, GroupId, DisplayName and
  RunLevel.

Maybe that's because they are guaranteed to be set and non empty but I 
  could not find any documentation saying so and they are not 
  implemented in Wine.

* write_registration_info() is very similar but checks bstr, not what is 
  in effect *bstr:

hr = IRegistrationInfo_get_Source(reginfo, &bstr);
    if (hr == S_OK && bstr)
    {
        hr = write_text_value(stream, L"Source", bstr);

* We don't seem to have tests showing how this works.

I would expect all these property getters to work the same way.
So would something like that make more sense?

hr = IPrincipal_get_UserId(principal, &bstr);
    if (hr == S_OK && bstr)
    {
        hr = write_text_value(stream, L"UserId", bstr);

Dmitry: Do you remember if you wrote the lstrlenW(bstr) check 
        because of some test you did?

[Dmitry Timoshkov]

Francois Gouget  wrote:
> I would expect all these property getters to work the same way.
> So would something like that make more sense?
> hr = IPrincipal_get_UserId(principal, &bstr);
> if (hr == S_OK && bstr)
> {
>     hr = write_text_value(stream, L"UserId", bstr);

I'd guess the check is supposed to avoid writing an empty UserId, and
apparently the check for bstr being NULL is not enough. I don't recall
why writing empty UserId is different from other property values.