Re: [PATCH 4/7] Mini-OS: add 9pfs frontend

From: Samuel Thibault <samuel.thibault@ens-lyon.org>
To: Juergen Gross <jgross@suse.com>
Cc: minios-devel@lists.xenproject.org,
	xen-devel@lists.xenproject.org, wl@xen.org
Subject: Re: [PATCH 4/7] Mini-OS: add 9pfs frontend
Date: Mon, 6 Feb 2023 10:01:39 +0100	[thread overview]
Message-ID: <20230206090139.ehvf2czoocn6j7nc@begin> (raw)
In-Reply-To: <20230203091809.14478-5-jgross@suse.com>

Juergen Gross, le ven. 03 févr. 2023 10:18:06 +0100, a ecrit:
> +void *init_9pfront(unsigned int id, const char *mnt)
> +{
[...]
> +    free(xenbus_watch_path_token(XBT_NIL, bepath, bepath, &dev->events));

Better check for errors, otherwise the rest will hang without useful
feedback.

> +    for ( v = version; *v; v++ )
> +    {
> +        if ( strtoul(v, &v, 10) == 1 )
> +        {
> +            v = NULL;
> +            break;

This looks fragile? if version is "2.1" it will accept it apparently? I
guess better check whether strtoul did read a number, and in that case
break the loop anyway, successfully if the number is 1 and with failure
otherwise.

> +        }
> +    }
> +    free(version);
> +    if ( v )
> +    {
> +        reason = "version 1 not supported";
> +        goto err;
> +    }

This looks odd: when number 1 is detected this breaks out successfully,
while the error message otherwise says that it's version 1 which is not
supported? Is the message supposed to be "version greater than 1 not
supported"?

> + err:
> +    if ( bepath[0] )
> +        free(xenbus_unwatch_path_token(XBT_NIL, bepath, bepath));
> +    if ( msg )
> +        printk("9pfsfront add %u failed, error %s accessing Xenstore\n",
> +               id, msg);
> +    else
> +        printk("9pfsfront add %u failed, %s\n", id, reason);
> +    free_9pfront(dev);

In case of early errors, this will try to free uninitialized evtchn,
ring_ref, etc.

> +    free(msg);
> +    return NULL;
> +}

Samuel