XML refactors: Simplify some attribute-related helpers

[nrwahl2]

And other assorted changes. This is the next batch from #4013.

[clumens]

I'm always a fan of a function that takes a function as an argument.

I'd like to see this one look more like pcmk__xe_foreach_const_attr and pcmk__xe_foreach_child - those use a for loop but this one uses a while loop. pcmk__xe_foreach_child asserts if its function is NULL, returns an int instead of a bool, and its prototype gives names to the arguments of its handler argument.

On those last two points, I don't care which way we go with it. I just like the consistency.

[nrwahl2]

I almost always prefer for-loops. However, the for-loop header is a bit awkward if the body can delete the current element:

bool
pcmk__xe_foreach_attr(xmlNode *xml, bool (*fn)(xmlAttr *, void *),
                      void *user_data)
{
    for (xmlAttr *attr = pcmk__xe_first_attr(xml),
                 *next = ((attr != NULL)? attr->next : NULL),
         attr != NULL;
         attr = next, next = next->next) {

        if (!fn(attr, user_data)) {
            return false;
        }
    }

    return true;
}

I'm happy to do the above if you're good with it. You've complained about the comma construction before.

There is no such complication for const for-loops though.

[nrwahl2]

I think this would also work, though I don't like having part of the iteration machinery in the body and part in the header:

    for (xmlAttr *attr = pcmk__xe_first_attr(xml), *next = NULL;
         attr != NULL; attr = next) {

        next = attr->next;

        if (!fn(attr, user_data)) {
            return false;
        }
    }

I haven't tested either of the above, just wrote them real quick. Typos are possible.

[nrwahl2]

pcmk__xe_foreach_child asserts if its function is NULL, returns an int instead of a bool, and its prototype gives names to the arguments of its handler argument.

I like the assertion point. I feel like I did that somewhere else recently. Maybe I added it to these in a later commit (not in this PR), or maybe it was in Booth.

The return type inconsistency does bug me a little bit. We occasionally use the return value from pcmk__xe_foreach_child(). It's possible to get rid of that (whether by storing the return code in user data or by letting the caller figure it out somehow), so that it can return bool. I didn't really want to mess with that function though.

I usually don't clutter an "outer" function definition with argument names within a function-type argument. Besides taking up space, it can get confusing when for example the "outer" function and its argument function both take user data. I didn't want to modify pcmk__xe_foreach_child() just for that though.

[clumens]

Hrm, I must have glossed right over the \note that pcmk__xe_foreach_attr can remove elements from the list. In that case the while loop does read more clearly than the for loop versions.

Also in that case, maybe my problem is with the name. When I think of a function named foreach, I think of something that will walk the whole list. Fair, pcmk__xe_foreach_child doesn't necessarily do that, but I think it's okay because at least the list isn't changed. From that perspective, pcmk__xe_foreach_const_attr is fine to be named foreach too.

It also kind of annoys me that we don't have anything in the standard Pacemaker return code space to indicate stop vs. continue so we have to use a bool here. I think I added codes for that in a PR at some point, but one of us didn't especially like it so I got rid of it.

I think everything in this PR is fine, I'm just waffling a little bit on the particulars of these functions. I'll have to think about it some more.

[nrwahl2]

Hrm, I must have glossed right over the \note that pcmk__xe_foreach_attr can remove elements from the list. In that case the while loop does read more clearly than the for loop versions.

Yeah, I used the g_list_foreach() implementation as the basis, since its function argument is allowed to remove items from the list.

Also in that case, maybe my problem is with the name. When I think of a function named foreach, I think of something that will walk the whole list. Fair, pcmk__xe_foreach_child doesn't necessarily do that, but I think it's okay because at least the list isn't changed. From that perspective, pcmk__xe_foreach_const_attr is fine to be named foreach too.

I agree with you. However, I also am reluctant to make the function names even longer. It would be more accurate to have something like pcmk__xe_foreach_const_attr_until(), to indicate that we might stop before hitting each attribute. (And similar for the other foreach functions.)

It also kind of annoys me that we don't have anything in the standard Pacemaker return code space to indicate stop vs. continue so we have to use a bool here. I think I added codes for that in a PR at some point, but one of us didn't especially like it so I got rid of it.

Yeah, it just feels silly to have a two-value enum, even if it is clearer. On the other hand, we definitely COULD do that. After all, GLib has G_SOURCE_CONTINUE as an alias for TRUE and G_SOURCE_REMOVE as an alias for FALSE.

[clumens]

Yeah, I used the g_list_foreach() implementation as the basis, since its function argument is allowed to remove items from the list.

Bah, I don't like all these mutable lists. Okay well I guess in that case I guess it makes sense to move existing _foreach_ functions to support being able to remove items from their list. There's a smattering of these functions right now but I think pcmk__xe_foreach_child is the most important, even if we don't have any need to remove items with that function now. I haven't looked into whether that's the case or not.

I just want all these similarly named functions to also function similarly.

I don't see that pcmk__xe_foreach_const_attr is being used yet, but I imagine that's going to come up soon enough. I also see there's a pe__foreach_const_bundle_replica (that's a mouthful). Needing const vs. non-const versions of these is kind of frustrating.

Yeah, it just feels silly to have a two-value enum, even if it is clearer. On the other hand, we definitely COULD do that. After all, GLib has G_SOURCE_CONTINUE as an alias for TRUE and G_SOURCE_REMOVE as an alias for FALSE.

Personally, I'd just make pcmk__xe_foreach_attr and pcmk__xe_foreach_const_attr return a standard pacemaker return code, and make their function arguments do the same just like pcmk__xe_foreach_child is doing. Most of the time you're not actually going to care about the return value of the overall operation, but when you do, we'll have the richer set of return values just in case.

[nrwahl2]

Bah, I don't like all these mutable lists.

FWIW, g_list_foreach() DOES hit every item in the list, even if it removes some along the way. Our "early stopping" thing is something that I don't think I've seen in a "foreach" construct before. But it is definitely convenient for us. Otherwise we end up having to store a "keep going" value in the user data and return immediately if it's been set to false. Which is a pain.

I just want all these similarly named functions to also function similarly.

I get that. On the other hand I kind of hate adding functionality that will remain unused for the foreseeable future. In general I try to remove clutter like that.

I don't see that pcmk__xe_foreach_const_attr is being used yet, but I imagine that's going to come up soon enough.

Yeah if it hasn't been used yet, then this PR just doesn't include the commits that use it.

Personally, I'd just make pcmk__xe_foreach_attr and pcmk__xe_foreach_const_attr return a standard pacemaker return code

I can give that some thought and see what it looks like in practice. Maybe with a pcmk_rc_stop that acts like pcmk_rc_ok... but there may be some times when we need to store an error and still continue iterating over the rest of the list, and some times when we store an error and stop. I could see that getting confusing. But might be fine and handle-able within the passed-in function.