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 <jgr...@suse.com>
>>> ---
>>> 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).
~Andrew
