Re: [PATCH 3/5] tools/libs/store: drop read-only functionality

[Wei Liu <wl@xxxxxxx>]

On Wed, Oct 07, 2020 at 02:45:29PM +0200, Jürgen Groß wrote:
> On 07.10.20 13:50, Andrew Cooper wrote:
> > On 07/10/2020 11:57, Jürgen Groß wrote:
> > > On 07.10.20 12:54, Wei Liu wrote:
> > > > On Fri, Oct 02, 2020 at 05:41:39PM +0200, Juergen Gross wrote:
> > > > > Today it is possible to open the connection in read-only mode via
> > > > > xs_daemon_open_readonly(). This is working only with Xenstore running
> > > > > as a daemon in the same domain as the user. Additionally it doesn't
> > > > > add any security as accessing the socket used for that functionality
> > > > > requires the same privileges as the socket used for full Xenstore
> > > > > access.
> > > > > 
> > > > > So just drop the read-only semantics in all cases, leaving the
> > > > > interface existing only for compatibility reasons. This in turn
> > > > > requires to just ignore the XS_OPEN_READONLY flag.
> > > > > 
> > > > > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > > > > ---
> > > > >    tools/libs/store/include/xenstore.h | 8 --------
> > > > >    tools/libs/store/xs.c               | 7 ++-----
> > > > >    2 files changed, 2 insertions(+), 13 deletions(-)
> > > > > 
> > > > > diff --git a/tools/libs/store/include/xenstore.h
> > > > > b/tools/libs/store/include/xenstore.h
> > > > > index cbc7206a0f..158e69ef83 100644
> > > > > --- a/tools/libs/store/include/xenstore.h
> > > > > +++ b/tools/libs/store/include/xenstore.h
> > > > > @@ -60,15 +60,12 @@ typedef uint32_t xs_transaction_t;
> > > > >    /* Open a connection to the xs daemon.
> > > > >     * Attempts to make a connection over the socket interface,
> > > > >     * and if it fails, then over the  xenbus interface.
> > > > > - * Mode 0 specifies read-write access, XS_OPEN_READONLY for
> > > > > - * read-only access.
> > > > >     *
> > > > >     * * Connections made with xs_open(0) (which might be shared page 
> > > > > or
> > > > >     *   socket based) are only guaranteed to work in the parent after
> > > > >     *   fork.
> > > > >     * * xs_daemon_open*() and xs_domain_open() are deprecated synonyms
> > > > >     *   for xs_open(0).
> > > > > - * * XS_OPEN_READONLY has no bearing on any of this.
> > > > >     *
> > > > >     * Returns a handle or NULL.
> > > > >     */
> > > > > @@ -83,11 +80,6 @@ void xs_close(struct xs_handle *xsh /* NULL ok */);
> > > > >     */
> > > > >    struct xs_handle *xs_daemon_open(void);
> > > > >    struct xs_handle *xs_domain_open(void);
> > > > > -
> > > > > -/* Connect to the xs daemon (readonly for non-root clients).
> > > > > - * Returns a handle or NULL.
> > > > > - * Deprecated, please use xs_open(XS_OPEN_READONLY) instead
> > > > > - */
> > > > >    struct xs_handle *xs_daemon_open_readonly(void);
> > > > >      /* Close the connection to the xs daemon.
> > > > > diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
> > > > > index 320734416f..4ac73ec317 100644
> > > > > --- a/tools/libs/store/xs.c
> > > > > +++ b/tools/libs/store/xs.c
> > > > > @@ -302,7 +302,7 @@ struct xs_handle *xs_daemon_open(void)
> > > > >      struct xs_handle *xs_daemon_open_readonly(void)
> > > > >    {
> > > > > -    return xs_open(XS_OPEN_READONLY);
> > > > > +    return xs_open(0);
> > > > >    }
> > > > 
> > > > This changes the semantics of this function, isn't it? In that the user
> > > > expects a readonly connection but in fact a read-write connection is
> > > > returned.
> > > > 
> > > > Maybe we should return an error here?
> > > 
> > > No, as the behavior is this way already if any of the following is true:
> > > 
> > > - a guest is opening the connection
> > > - Xenstore is running in a stubdom
> > > - oxenstored is being used
> > 
> > Right, and these are just some of the reasons why dropping the R/O
> > socket is a sensible thing to do.
> > 
> > However, I do think xenstore.h needs large disclaimers next to the
> > relevant constants and functions that both XS_OPEN_* flags are now
> > obsolete and ignored (merged into appropriate patches in the series).
> 
> Fine with me.

+1 on this. Let me check other patches first. If there is no need for
another round of posting of this series, the addition of the disclaimers
can come in a separate patch.

Wei.

> 
> 
> Juergen
>