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

[Juergen Gross <jgross@xxxxxxxx>]

On 04.08.23 12:33, Julien Grall wrote:
> Hi Juergen,
>
> On 04/08/2023 11:17, Juergen Gross wrote:
> > On 04.08.23 12:00, Julien Grall wrote:
> > > Hi,
> > >
> > > On 04/08/2023 10:35, Juergen Gross wrote:
> > > > On 03.08.23 23:46, Julien Grall wrote:
> > > > > Hi,
> > > > >
> > > > > On 24/07/2023 12:02, Juergen Gross wrote:
> > > > > > Today is_valid_nodename() is always called directly after calling
> > > > > > canonicalize(), with the exception of do_unwatch(), where the call
> > > > > > is missing (which is not correct, but results just in a wrong error
> > > > > > reason being returned).
> > > > > While this change makes sense...
> > > > >
> > > > > > Merge is_valid_nodename() into canonicalize(). While at it merge
> > > > > > valid_chars() into it, too.
> > > > > ... I am not in favor of folding the code is_valid_nodename() and 
> > > > > valid_chars() into canonicalize() because the code is now more difficult to 
> > > > > read. Also, the keeping the split would allow to free the 'name' in case of 
> > > > > an error without adding too much goto in the code.
> > > > I don't think we can easily free name in an error case, at that would require
> > > > to keep knowledge that name was just allocated in the non-canonical case.
> > > How about this:
> > >
> > > const char *canonicalize(struct connection *conn, const void *ctx,
> > >                           const char *node, bool allow_special)
> > > {
> > >          const char *prefix;
> > >          const char *name;
> > >
> > >          if (!node)
> > >                  return NULL;
> > >
> > >          if (node[0] == '@' && !allow_special)
> > >                  return NULL;
> > >
> > >          if (!node || (node[0] == '/') || (node[0] == '@'))
> > >                  return node;
> > >          prefix = get_implicit_path(conn);
> > >          if (prefix) {
> > >                  name = talloc_asprintf(ctx, "%s/%s", prefix, node);
> > >                  if (name)
> > >                          return NULL;
> > >          } else
> > >                  name = node;
> > >
> > >          if (!is_valid_nodename(conn, node, allow_special)) {
> > >                  /* Release the memory if 'name' was allocated by us */
> > >                  if ( name != node )
> > >                          talloc_free(name);
> > >                  return NULL;
> > >          }
> > >
> > >          return name;
> > > }
> > >
> > > And before you ask, I don't see the benefits to partially validate the name 
> > > before allocating. Hence why I suggest to keep is_valid_nodename() as this 
> > > keep the function small.
> > Partially validating before doing the allocation is a benefit as it doesn't
> > spend cycles on validating the known good prefix.
> Which is pretty much a drop in the ocean in the context of Xenstored :). In 
> reality most of the validation can be done before the allocation with a bit of 
> work.
Yes, but this would need a more complicated logic related to the handling
of local_off. I thought about that, but didn't want to make the patch more
complicated.

And regarding performance - yes, it will be minimal, but it was rather low
hanging fruit.

> > Additionally your example won't validate an absolute pathname at all.
> That's an error in the logic. This can be sorted out.
>
> > What about this variant:
> I still don't see the value of folding is_valid_node_name(). But if the other 
> agrees with you then...
I didn't see the value of keeping them apart, as the only caller of
is_valid_node_name() would be canonicalize() after this patch. :-)

> > 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.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature