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

[Wei Liu <wl@xxxxxxx>]

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?

Wei.