Re: [Qemu-devel] [PATCH] uselib, mincore and readahead syscalls

[Riku Voipio]

On Fri, Sep 19, 2008 at 11:05:35AM +0300, Kirill A. Shutemov wrote:
> On Wed, Sep 17, 2008 at 10:45:42PM +0300, Riku Voipio wrote:
> > These have been carried in Debian since forever. Added lock_user()
> > calls for mincore before submitting, I'm not sure if that's the correct
> > way?
> > 
> > Signed-off-by: Riku Voipio <address@hidden>

> I think this patch should be splited by syscall.

Yes, but expecting that these patches as going to be ignored I didn't
want to put too much effort in them..

> > Index: trunk/linux-user/syscall.c
> > ===================================================================
> > --- trunk.orig/linux-user/syscall.c 2008-09-17 20:07:40.000000000 +0300
> > +++ trunk/linux-user/syscall.c      2008-09-17 21:43:47.000000000 +0300
> > @@ -276,6 +276,7 @@
> >  extern int setfsuid(int);
> >  extern int setfsgid(int);
> >  extern int setgroups(int, gid_t *);
> > +extern int uselib(const char*);

> Probably, we should use _syscall1 macros instead.

Why? More specifically, in which cases should we use _syscall macros
and which cases the libc versions? I don't see much logic in the current
mix of both being used in syscall.c

> >  #define ERRNO_TABLE_SIZE 1200
> >  
> > @@ -4226,7 +4227,13 @@
> >  #endif
> >  #ifdef TARGET_NR_uselib
> >      case TARGET_NR_uselib:
> > -        goto unimplemented;
> > +           {
> > +                   if(!(p = lock_user_string(arg1)))
> > +                           goto efault;
> > +           ret = get_errno(uselib(path(p)));
> > +                   unlock_user(p, arg1, 0);
> > +           }
> > +        break;
> >  #endif
> >  #ifdef TARGET_NR_swapon
> >      case TARGET_NR_swapon:
> > @@ -5512,7 +5519,18 @@
> >          goto unimplemented;
> >  #ifdef TARGET_NR_mincore
> >      case TARGET_NR_mincore:
> > -        goto unimplemented;
> > +           {
> > +                   void *a;
> > +                   if (!(a = lock_user(VERIFY_READ, arg1,arg2, 0)))
> > +                   goto efault;
> > +           if (!(p = lock_user_string(arg3)))
> > +                   goto mincore_fail;
> > +           ret = get_errno(mincore((void*)a, (size_t)arg2, (unsigned 
> > char*)p));
> 
> Type casting is unneeded here.

Will fix

> > +                   unlock_user(p, arg3, ret);
> > +                   mincore_fail:
> > +                   unlock_user(a, arg1, 0);

> You should set ret here.

Will fix

> > +           }
> > +        break;
> >  #endif
> >  #ifdef TARGET_NR_madvise
> >      case TARGET_NR_madvise:
> > @@ -5652,7 +5670,8 @@
> >          break;
> >  #ifdef TARGET_NR_readahead
> >      case TARGET_NR_readahead:
> > -        goto unimplemented;
> > +        ret = get_errno(readahead((int)arg1, (off64_t)arg2, (size_t)arg3));
> 
> Type casting is unneeded here.

Will fix

> > +        break;
> >  #endif
> >  #ifdef TARGET_NR_setxattr
> >      case TARGET_NR_setxattr:
> 
> -- 
> Regards,  Kirill A. Shutemov
>  + Belarus, Minsk
>  + ALT Linux Team, http://www.altlinux.com/



-- 
"rm -rf" only sounds scary if you don't have backups