Re: [PATCH (long)] hello.c follow-up

[PUYDT Julien]

Le lun 28/07/2003 à 14:26, Jeroen Dekkers a écrit : 
> > Notice that in storeio, trivfs_S_io_seek uses open_seek, which doesn't
> > check the offset sanity either; I'll take a look at it when I'll have
> > understood things a little more.
> 
> There are more places where this check doesn't exist.

I saw that. I'm waiting to have this patch+ChangeLog in a commit-able
state before trying to tamper with more important code... let's say
hello is my playground. I will also check that the read functions handle
the offset correctly, since the Single Unix Specification v3 says that
the seek function can set the offset>filesize! It is done right in
hello.c, but I think I saw it wasn't the case somewhere else.

I also wonder if it wouldn't be a good idea to add more comments to the
code, like the ones in hurd-one.c, to make it more friendly... it is
after all a basic example for newcomers like me!

> > +kern_return_t
> > +trivfs_S_io_get_openmodes (struct trivfs_protid *cred,
> > +                           mach_port_t reply,
> > +                           mach_msg_type_name_t replytype,
> > +                           int *bits)
> > +{
> > +  if (!cred)
> > +    return EOPNOTSUPP;
> > +  else
> > +    {
> > +      *bits = cred->po->openmodes;
> > +      return 0;
> > +    }
> > +}
> 
> This function is the same as the one in libtrivfs, no need to duplicate it.

That is true; I added it by comparison with hurd-one.c. Added to my
mental list of things to patch in hurd-one.c: remove that duplication
:-)


> > +trivfs_S_io_clear_some_openmodes (struct trivfs_protid *cred,
> > +trivfs_S_io_set_some_openmodes (struct trivfs_protid *cred,
> > +trivfs_S_io_set_all_openmodes (struct trivfs_protid *cred,
(cut, to keep only functions' names)

> These should return EOPNOTSUPP when !cred and 0 otherwise.

Ok.

> > +kern_return_t
> > +trivfs_S_io_readable (struct trivfs_protid *cred,
> > +                      mach_port_t reply,
> > +                      mach_msg_type_name_t replytype,
> > +                      mach_msg_type_number_t *amount)
> > +{
> > +  struct open *op;
> > +
> > +  if (!cred)
> > +    return EOPNOTSUPP;
> > + 
> > +  if(! (cred->po->openmodes & O_READ))
> > +    return EINVAL;
> 
> This should return EBADF instead of EINVAL.

Ok.

> > +  op=cred->po->hook; 
> > +  *amount=contents_len - op->offs;
> 
> You forgot spaces around the "=". 

True.

> > +  return 0;
> > +
> > +}
> > +
> > +kern_return_t
> > +trivfs_S_io_select (struct trivfs_protid *cred,
> > +                    mach_port_t reply,
> > +                    mach_msg_type_name_t replytype,
> > +                    int *seltype)
> > +{
> > +  if (!cred)
> > +    return EOPNOTSUPP;
> > +
> > +  if ((*seltype & SELECT_READ) && !(cred->po->openmodes & O_READ))
> > +    return EBADF;
> 
> I'm not sure if this is right, somebody else should comment on that.

Well... not sure either, that was done by comparison with hurd-one.c.

> > +  *seltype &= ~SELECT_URG;
> > +  *seltype &= ~SELECT_WRITE;
> > +  
> > +  return 0;
> > +}
> 
> Jeroen Dekkers

Thanks for this useful feedback,

Snark on #hurd, #hurdfr