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

[Juergen Gross <jgross@xxxxxxxx>]

On 06.02.23 10:01, Samuel Thibault wrote:
> 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.
This is a common pattern in Mini-OS frontends.

I can add an error check, of course.

> > +    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.
Versions are defined to be integers.

I can add checks for sanitizing backend written data, but I'm not sure we
need that. In case the backend wants to fool us, it can easily tell us to
support version 1 even if it doesn't.

> > +        }
> > +    }
> > +    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"?
I can change the message to "Backend doesn't support version 1".

> > + 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.
Oh right, I need to check those for being not 0.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature