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

[Juergen Gross <jgross@xxxxxxxx>]

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 ||
> > > >              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? :)
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

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature