Re: [akaros] [PATCH 4/4] capability device: get it to compile

[ron minnich]

On Thu, Oct 13, 2016 at 3:47 PM barret rhoden  wrote:

> Hi -
>
> Couple minor things:
>
> > @@ -99,8 +85,8 @@ static int32_t capstat(struct chan *c, uint8_t *db,
> int32_t n)
> >  static struct chan *capopen(struct chan *c, int omode)
> >  {
> >   if (c->qid.type & QTDIR) {
> > - if (omode != OREAD)
> > - error(Ebadarg);
> > + if (omode != O_RDONLY)
> > + error(EISDIR, "Is a directory");
> >   c->mode = omode;
>
> I think we might want/need to run omode through openmode().  The other
> drivers do something like:
>

I'd like to not mess with this but just now. But later, yes. This device is
pretty subtle so I don't like to make such changes right off.

I changed user back to user[64].

ron

-- 
You received this message because you are subscribed to the Google Groups 
"Akaros" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to akaros+unsubscr...@googlegroups.com.
To post to this group, send email to akaros@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [akaros] [PATCH 4/4] capability device: get it to compile

[barret rhoden]

Hi -

Couple minor things:

> @@ -99,8 +85,8 @@ static int32_t capstat(struct chan *c, uint8_t *db, int32_t 
> n)
>  static struct chan *capopen(struct chan *c, int omode)
>  {
>   if (c->qid.type & QTDIR) {
> - if (omode != OREAD)
> - error(Ebadarg);
> + if (omode != O_RDONLY)
> + error(EISDIR, "Is a directory");
>   c->mode = omode;

I think we might want/need to run omode through openmode().  The other
drivers do something like:

kern/drivers/dev/mnt.c  c->mode = openmode(omode);

Technically, I guess we don't, since we error if omode isn't O_RDONLY.
(In which case, we can just as easily say c->mode = O_RDONLY).  

Btw, restricting flags to only O_RDONLY (a.k.a. O_READ) might have some
issues too, since the omode that comes in might have other flags set.
e.g. O_REMCLO, O_ASYNC, etc.  That's why openmode() filters omode with
O_ACCMODE.  I'm sure we can deal with those cases if they ever pop up.

> diff --git a/kern/include/env.h b/kern/include/env.h
> index 9dad848..dca5aee 100644
> --- a/kern/include/env.h
> +++ b/kern/include/env.h
> @@ -33,7 +33,7 @@ struct proc {
>   TAILQ_ENTRY(proc) sibling_link;
>   spinlock_t proc_lock;
>   struct user_context scp_ctx;/* context for an SCP.  TODO: move to 
> vc0 */
> - char user[64]; /* user name */
> + char *user;

I think this will break things elsewhere, since p->user is dereferenced
in a lot of places.  Easiest thing for now is to leave it at user[64],
and then in a later patch we dynamically allocate it.

What's your plan for that anyways?  Have 'user' point to some static
string by default, then dynamically allocate if we change the name?
(and then free in __proc_free())?


Otherwise, this patch set looks fine.  I didn't actually look at how
the cap device works - I just scanned for porting issues.  I figure
once we get it and sha256/crypto in and we think it all is working,
then I'll take a closer look.

Thanks,

Barret

-- 
You received this message because you are subscribed to the Google Groups 
"Akaros" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to akaros+unsubscr...@googlegroups.com.
To post to this group, send email to akaros@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.