Re: [Xen-devel] [PATCH 5/6] xenbus: process be_watch events in xenwatch multithreading

[Dongli Zhang <dongli.zhang@oracle.com>]

Hi Juergen,

On 09/14/2018 10:44 PM, Juergen Gross wrote:
> On 14/09/18 16:29, Dongli Zhang wrote:
>> Hi Juergen,
>>
>> On 09/14/2018 10:26 PM, Juergen Gross wrote:
>>> On 14/09/18 16:18, Dongli Zhang wrote:
>>>> Hi Juergen,
>>>>
>>>> On 09/14/2018 05:12 PM, Juergen Gross wrote:
>>>>> On 14/09/18 09:34, Dongli Zhang wrote:
>>>>>> This is the 5th patch of a (6-patch) patch set.
>>>>>>
>>>>>> With this patch, watch event in relative path pattern
>>>>>> 'backend/<pvdev>/<domid>i/...' can be processed in per-domU xenwatch
>>>>>
>>>>> superfluous "i" ----------^
>>>>>
>>>>>> thread.
>>>>>>
>>>>>> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
>>>>>> ---
>>>>>>  drivers/xen/xenbus/xenbus_probe.c         |  2 +-
>>>>>>  drivers/xen/xenbus/xenbus_probe_backend.c | 32 +++++++++++++++++++++++++++++++
>>>>>>  include/xen/xenbus.h                      |  2 ++
>>>>>>  3 files changed, 35 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
>>>>>> index ba0644c..aa1b15a 100644
>>>>>> --- a/drivers/xen/xenbus/xenbus_probe.c
>>>>>> +++ b/drivers/xen/xenbus/xenbus_probe.c
>>>>>> @@ -552,7 +552,7 @@ int xenbus_probe_devices(struct xen_bus_type *bus)
>>>>>>  }
>>>>>>  EXPORT_SYMBOL_GPL(xenbus_probe_devices);
>>>>>>  
>>>>>> -static unsigned int char_count(const char *str, char c)
>>>>>> +unsigned int char_count(const char *str, char c)
>>>>>
>>>>> Please change the name of the function when making it globally
>>>>> visible, e.g. by prefixing "xenbus_".
>>>>>
>>>>> Generally I think you don't need to use it below.
>>>>>
>>>>>>  {
>>>>>>  	unsigned int i, ret = 0;
>>>>>>  
>>>>>> diff --git a/drivers/xen/xenbus/xenbus_probe_backend.c b/drivers/xen/xenbus/xenbus_probe_backend.c
>>>>>> index b0bed4f..50df86a 100644
>>>>>> --- a/drivers/xen/xenbus/xenbus_probe_backend.c
>>>>>> +++ b/drivers/xen/xenbus/xenbus_probe_backend.c
>>>>>> @@ -211,9 +211,41 @@ static void backend_changed(struct xenbus_watch *watch,
>>>>>>  	xenbus_dev_changed(path, &xenbus_backend);
>>>>>>  }
>>>>>>  
>>>>>> +static domid_t path_to_domid(const char *path)
>>>>>> +{
>>>>>> +	const char *p = path;
>>>>>> +	domid_t domid = 0;
>>>>>> +
>>>>>> +	while (*p) {
>>>>>> +		if (*p < '0' || *p > '9')
>>>>>> +			break;
>>>>>> +		domid = (domid << 3) + (domid << 1) + (*p - '0');
>>>>>
>>>>> reinventing atoi()?
>>>>>
>>>>> Please don't do that. kstrtou16() seems to be a perfect fit.
>>>>
>>>> I did use kstrtou*() in the early prototype and realized kstrtou16() returns 0
>>>> if the input string contains non-numerical characters.
>>>>
>>>> E.g., the example of input can be "1/0/state", where 1 is fotherend_id
>>>> (frontend_id) and 0 is handle.
>>>>
>>>> When "1/0/state" is used at input, kstrtou16() returns 0 (returned integer) and
>>>> -22 (error).
>>>
>>> Aah, okay. Then simple_strtoul()?
>>
>> I did consider simple_strtoul() initially. Unfortunately, it is obsolete (below
>> line 81). AFAIR, the patch would not be able to pass the check_patch script when
>> this function is used.
> 
> Better use that than open coding a new instance of it.
> 
> Another variant would be to use sscanf() or similar. Then you could even
> drop using strchr() by adding that in the format string:
> 
> return (sscanf(path, "%*u/%u/", &domid) == 1) ? domid : DOMID_SELF;

I recall what was happened.

Suppose one sample of path is "backend/vif/19/3/state". (we would like to obtain
domid=19)

Initially I would like to use sscanf(path, "backend/%*[a-z]/%hu/%*u") to obtain
the domid from xenstore path in one call.

Unfortunately, unlike userspace sscanf(), the version in linux kernel does not
support '[' so that I would not be able to use "%*[a-z]" in sscanf() in linux
kernel.

Finally, I decided to skip 2 '/' to reach at "19/3/state" and I forgot to
continue using sscanf and implemented my own one :(

With sscanf, I think I still need to skip 2 '/' to skip "backend/<pv device>" in
order to reach at "<domid>/xxxxxxx...":

1. return if the number of '/' in the path is less than 2

2. skip 2 '/' (e.g., backend/vif/)

3. process the rest of patch via sscanf.

Dongli Zhang