Re: [PATCH 7/7] Implement vFile:setfs in gdbserver

[Gary Benson <gbenson at redhat dot com>]

Pedro Alves wrote:
> On 04/16/2015 01:19 PM, Gary Benson wrote:
> > +/* Handle a "vFile:setfs:" packet.  */
> > +
> > +static void
> > +handle_setfs (char *own_buf)
> > +{
[snip]
> > +}
> 
> This should probably return empty if the_target->call_with_fs_of is
> NULL, to disable the packet in GDB?
> 
> >  int
> >  handle_vFile (char *own_buf, int packet_len, int *new_packet_len)
> >  {
> > -  if (startswith (own_buf, "vFile:open:"))
> > +  if (the_target->call_with_fs_of != NULL
> > +      && startswith (own_buf, "vFile:setfs:"))
> > +    handle_setfs (own_buf);
> > +  else if (startswith (own_buf, "vFile:open:"))
> 
> Ah, I see now that it's handled here.  I'd prefer moving that
> check inside the handle_setfs function instead so that the "setfs"
> specifics handling is all localized.

Ok.

> > +struct open_closure
> > +{
> > +  char filename[HOSTIO_PATH_MAX];
> > +  int flags;
> > +  int mode;
> 
> mode_t mode.
> 
> (please check for the the same issue on the native side.)

It's there in both places.  Is mode_t ok to use in gdbserver?
I don't see it anywhere...

> > +  int return_value;
> > +};
> 
> > +/* Implementation of target_ops->call_with_fs_of.  */
> > +
> > +static int
> > +linux_low_call_with_fs_of (int pid, void (*func) (void *), void *arg)
> > +{
> > +  return linux_ns_enter (pid, LINUX_NS_MNT, func, arg);
> > +}
> > +
> 
> So back on linux_ns_enter's naming ---
> 
> I find the abstraction here a bit odd.  It seems to be me that
> a function that does:
> 
>  #1 - select filesystem/namespace
>  #2 - call foo
>  #3 - restore filesystem/namespace
> 
> should be a function in common code, and that the only
> target-specific part is selecting a filesystem/namespace.
> That is, simplifying, something like:
> 
> /* Cause the filesystem to appear as it does to process PID and
>    call FUNC with argument ARG, restoring the filesystem to its
>    original state afterwards.  Return nonzero if FUNC was called,
>    zero otherwise (and set ERRNO). */
> 
> int
> call_with_fs_of (int pid, void (*func) (void *), void *arg)
> {
>   int ret;
> 
>   target->select_fs (pid);
>   ret = func ();
>   target->select_fs (0);
>   return ret;
> }
> 
> Was there a reason this wasn't done that way?
> 
> (maybe make target->select_fs() return the previous
> namespace, instead of passing 0 on the second
> call.)

I'll have a go at doing it that way.

Thanks,
Gary

-- 
http://gbenson.net/