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

Gary Benson
2015-04-17T16:47:19Z

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

--