Re: [PATCH v3 23/25] tools/xenstore: merge is_valid_nodename() into canonicalize()

[Juergen Gross]

On 04.08.23 14:27, Julien Grall wrote:

Hi Juergen,

On 04/08/2023 13:05, Juergen Gross wrote:

On 04.08.23 12:33, Julien Grall wrote:

const char *canonicalize(struct connection *conn, const void *ctx,
                          const char *node, bool allow_special)
{
         const char *name;
         int local_off = 0;
         unsigned int domid;

         /*
          * Invalid if any of:
          * - no node at all
          * - illegal character in node
          * - starts with '@' but no special node allowed
          */
         errno = EINVAL;
         if (!node ||

                          "0123456789-/_@") != strlen(node) ||

             strspn(node, "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"

... I would rather keep calling valid_chars() here. The rest looks fine even though this is definitely not my preference.

I can do that, even if I don't see the real value with the comment above the if.

How about writing Xenstored in a single function then? After all with comments it should be easy to read, right? :)

Yeah, right.

Might come with the downside of a little bit of code duplication. ;-)

There are a few difficulty with the current approach. There are:
   * a large function call that needs to be split over two lines
   * multiple || which also need to split over multiple lines.
   * No parentheses over strspn(....) != strlen(node)

Maybe you can parse/understand this 'if' very quickly. But I can't and this is just slowing down review and increasing the risk of introducing bugs.

Okay, as said above: I can do that.


Juergen

OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

OpenPGP_signature
Description: OpenPGP digital signature