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
