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

[Julien Grall <julien@xxxxxxx>]

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 ||
> > >              strspn(node, 
> > > "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
> > >                           "0123456789-/_@") != strlen(node) ||
> > ... 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? :)
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.
Cheers,

--
Julien Grall