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

[Samuel Thibault <samuel.thibault@xxxxxxxxxxxx>]

Juergen Gross, le lun. 06 févr. 2023 10:22:10 +0100, a ecrit:
> 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.

Ah, indeed. Ok, then.

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

Ah, it's a list of versions?
(I don't know the protocol at all).

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

Not necessarily fooling, but misprogramming :)

> > > +        }
> > > +    }
> > > +    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".

Aah, yes indeed, that was the part I missed for understanding it all :)

Samuel