[v1] drivers/xen/xenbus: Replace deprecated strcpy in xenbus_transaction_end

[PATCH] drivers/xen/xenbus: Replace deprecated strcpy in xenbus_transaction_end

Posted by Thorsten Blum 2 weeks, 3 days ago

strcpy() is deprecated; use strscpy() instead. Fix the function comment
and use bool instead of int while we're at it.

Link: https://github.com/KSPP/linux/issues/88
Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
 drivers/xen/xenbus/xenbus_xs.c | 9 +++------
 include/xen/xenbus.h           | 2 +-
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c
index 528682bf0c7f..970302b3dcc6 100644
--- a/drivers/xen/xenbus/xenbus_xs.c
+++ b/drivers/xen/xenbus/xenbus_xs.c
@@ -546,16 +546,13 @@ int xenbus_transaction_start(struct xenbus_transaction *t)
 EXPORT_SYMBOL_GPL(xenbus_transaction_start);
 
 /* End a transaction.
- * If abandon is true, transaction is discarded instead of committed.
+ * If abort is true, transaction is discarded instead of committed.
  */
-int xenbus_transaction_end(struct xenbus_transaction t, int abort)
+int xenbus_transaction_end(struct xenbus_transaction t, bool abort)
 {
 	char abortstr[2];
 
-	if (abort)
-		strcpy(abortstr, "F");
-	else
-		strcpy(abortstr, "T");
+	strscpy(abortstr, abort ? "F" : "T");
 
 	return xs_error(xs_single(t, XS_TRANSACTION_END, abortstr, NULL));
 }
diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h
index 7dab04cf4a36..c94caf852aea 100644
--- a/include/xen/xenbus.h
+++ b/include/xen/xenbus.h
@@ -158,7 +158,7 @@ int xenbus_exists(struct xenbus_transaction t,
 		  const char *dir, const char *node);
 int xenbus_rm(struct xenbus_transaction t, const char *dir, const char *node);
 int xenbus_transaction_start(struct xenbus_transaction *t);
-int xenbus_transaction_end(struct xenbus_transaction t, int abort);
+int xenbus_transaction_end(struct xenbus_transaction t, bool abort);
 
 /* Single read and scanf: returns -errno or num scanned if > 0. */
 __scanf(4, 5)
-- 
2.51.0

Re: [PATCH] drivers/xen/xenbus: Replace deprecated strcpy in xenbus_transaction_end

Posted by Jan Beulich 2 weeks, 3 days ago

On 12.10.2025 21:55, Thorsten Blum wrote:
> --- a/drivers/xen/xenbus/xenbus_xs.c
> +++ b/drivers/xen/xenbus/xenbus_xs.c
> @@ -546,16 +546,13 @@ int xenbus_transaction_start(struct xenbus_transaction *t)
>  EXPORT_SYMBOL_GPL(xenbus_transaction_start);
>  
>  /* End a transaction.
> - * If abandon is true, transaction is discarded instead of committed.
> + * If abort is true, transaction is discarded instead of committed.
>   */
> -int xenbus_transaction_end(struct xenbus_transaction t, int abort)
> +int xenbus_transaction_end(struct xenbus_transaction t, bool abort)
>  {
>  	char abortstr[2];
>  
> -	if (abort)
> -		strcpy(abortstr, "F");
> -	else
> -		strcpy(abortstr, "T");

While at least in principle a compiler might be able to transform this into
code not using any library function at all, ...

> +	strscpy(abortstr, abort ? "F" : "T");

... the use of a n on-standard function (without equivalent compiler builtin)
doesn't permit this. IOW why not simply switch to e.g.

    char abortstr[2] = { [0] = abort ? 'F' : 'T' };

Jan

Re: [PATCH] drivers/xen/xenbus: Replace deprecated strcpy in xenbus_transaction_end

Posted by Jürgen Groß 2 weeks, 3 days ago

On 13.10.25 08:59, Jan Beulich wrote:
> On 12.10.2025 21:55, Thorsten Blum wrote:
>> --- a/drivers/xen/xenbus/xenbus_xs.c
>> +++ b/drivers/xen/xenbus/xenbus_xs.c
>> @@ -546,16 +546,13 @@ int xenbus_transaction_start(struct xenbus_transaction *t)
>>   EXPORT_SYMBOL_GPL(xenbus_transaction_start);
>>   
>>   /* End a transaction.
>> - * If abandon is true, transaction is discarded instead of committed.
>> + * If abort is true, transaction is discarded instead of committed.
>>    */
>> -int xenbus_transaction_end(struct xenbus_transaction t, int abort)
>> +int xenbus_transaction_end(struct xenbus_transaction t, bool abort)
>>   {
>>   	char abortstr[2];
>>   
>> -	if (abort)
>> -		strcpy(abortstr, "F");
>> -	else
>> -		strcpy(abortstr, "T");
> 
> While at least in principle a compiler might be able to transform this into
> code not using any library function at all, ...
> 
>> +	strscpy(abortstr, abort ? "F" : "T");
> 
> ... the use of a n on-standard function (without equivalent compiler builtin)
> doesn't permit this. IOW why not simply switch to e.g.
> 
>      char abortstr[2] = { [0] = abort ? 'F' : 'T' };

I would even go further and drop abortstr[] completely:

diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c
index 528682bf0c7f..c891af7165f5 100644
--- a/drivers/xen/xenbus/xenbus_xs.c
+++ b/drivers/xen/xenbus/xenbus_xs.c
@@ -550,14 +550,8 @@ EXPORT_SYMBOL_GPL(xenbus_transaction_start);
   */
  int xenbus_transaction_end(struct xenbus_transaction t, int abort)
  {
-       char abortstr[2];
-
-       if (abort)
-               strcpy(abortstr, "F");
-       else
-               strcpy(abortstr, "T");
-
-       return xs_error(xs_single(t, XS_TRANSACTION_END, abortstr, NULL));
+       return xs_error(xs_single(t, XS_TRANSACTION_END, abort ? "F" : "T",
+                       NULL));
  }
  EXPORT_SYMBOL_GPL(xenbus_transaction_end);


Juergen

Re: [PATCH] drivers/xen/xenbus: Replace deprecated strcpy in xenbus_transaction_end

Posted by Jan Beulich 2 weeks, 3 days ago

On 13.10.2025 09:36, Jürgen Groß wrote:
> On 13.10.25 08:59, Jan Beulich wrote:
>> On 12.10.2025 21:55, Thorsten Blum wrote:
>>> --- a/drivers/xen/xenbus/xenbus_xs.c
>>> +++ b/drivers/xen/xenbus/xenbus_xs.c
>>> @@ -546,16 +546,13 @@ int xenbus_transaction_start(struct xenbus_transaction *t)
>>>   EXPORT_SYMBOL_GPL(xenbus_transaction_start);
>>>   
>>>   /* End a transaction.
>>> - * If abandon is true, transaction is discarded instead of committed.
>>> + * If abort is true, transaction is discarded instead of committed.
>>>    */
>>> -int xenbus_transaction_end(struct xenbus_transaction t, int abort)
>>> +int xenbus_transaction_end(struct xenbus_transaction t, bool abort)
>>>   {
>>>   	char abortstr[2];
>>>   
>>> -	if (abort)
>>> -		strcpy(abortstr, "F");
>>> -	else
>>> -		strcpy(abortstr, "T");
>>
>> While at least in principle a compiler might be able to transform this into
>> code not using any library function at all, ...
>>
>>> +	strscpy(abortstr, abort ? "F" : "T");
>>
>> ... the use of a n on-standard function (without equivalent compiler builtin)
>> doesn't permit this. IOW why not simply switch to e.g.
>>
>>      char abortstr[2] = { [0] = abort ? 'F' : 'T' };
> 
> I would even go further and drop abortstr[] completely:
> 
> diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c
> index 528682bf0c7f..c891af7165f5 100644
> --- a/drivers/xen/xenbus/xenbus_xs.c
> +++ b/drivers/xen/xenbus/xenbus_xs.c
> @@ -550,14 +550,8 @@ EXPORT_SYMBOL_GPL(xenbus_transaction_start);
>    */
>   int xenbus_transaction_end(struct xenbus_transaction t, int abort)
>   {
> -       char abortstr[2];
> -
> -       if (abort)
> -               strcpy(abortstr, "F");
> -       else
> -               strcpy(abortstr, "T");
> -
> -       return xs_error(xs_single(t, XS_TRANSACTION_END, abortstr, NULL));
> +       return xs_error(xs_single(t, XS_TRANSACTION_END, abort ? "F" : "T",
> +                       NULL));
>   }
>   EXPORT_SYMBOL_GPL(xenbus_transaction_end);

Hmm, which xs_single() indeed takes a const char *, it then casts away const-
ness before handing to xs_talkv().

Jan

Re: [PATCH] drivers/xen/xenbus: Replace deprecated strcpy in xenbus_transaction_end

Posted by Jürgen Groß 2 weeks, 3 days ago

On 13.10.25 10:22, Jan Beulich wrote:
> On 13.10.2025 09:36, Jürgen Groß wrote:
>> On 13.10.25 08:59, Jan Beulich wrote:
>>> On 12.10.2025 21:55, Thorsten Blum wrote:
>>>> --- a/drivers/xen/xenbus/xenbus_xs.c
>>>> +++ b/drivers/xen/xenbus/xenbus_xs.c
>>>> @@ -546,16 +546,13 @@ int xenbus_transaction_start(struct xenbus_transaction *t)
>>>>    EXPORT_SYMBOL_GPL(xenbus_transaction_start);
>>>>    
>>>>    /* End a transaction.
>>>> - * If abandon is true, transaction is discarded instead of committed.
>>>> + * If abort is true, transaction is discarded instead of committed.
>>>>     */
>>>> -int xenbus_transaction_end(struct xenbus_transaction t, int abort)
>>>> +int xenbus_transaction_end(struct xenbus_transaction t, bool abort)
>>>>    {
>>>>    	char abortstr[2];
>>>>    
>>>> -	if (abort)
>>>> -		strcpy(abortstr, "F");
>>>> -	else
>>>> -		strcpy(abortstr, "T");
>>>
>>> While at least in principle a compiler might be able to transform this into
>>> code not using any library function at all, ...
>>>
>>>> +	strscpy(abortstr, abort ? "F" : "T");
>>>
>>> ... the use of a n on-standard function (without equivalent compiler builtin)
>>> doesn't permit this. IOW why not simply switch to e.g.
>>>
>>>       char abortstr[2] = { [0] = abort ? 'F' : 'T' };
>>
>> I would even go further and drop abortstr[] completely:
>>
>> diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c
>> index 528682bf0c7f..c891af7165f5 100644
>> --- a/drivers/xen/xenbus/xenbus_xs.c
>> +++ b/drivers/xen/xenbus/xenbus_xs.c
>> @@ -550,14 +550,8 @@ EXPORT_SYMBOL_GPL(xenbus_transaction_start);
>>     */
>>    int xenbus_transaction_end(struct xenbus_transaction t, int abort)
>>    {
>> -       char abortstr[2];
>> -
>> -       if (abort)
>> -               strcpy(abortstr, "F");
>> -       else
>> -               strcpy(abortstr, "T");
>> -
>> -       return xs_error(xs_single(t, XS_TRANSACTION_END, abortstr, NULL));
>> +       return xs_error(xs_single(t, XS_TRANSACTION_END, abort ? "F" : "T",
>> +                       NULL));
>>    }
>>    EXPORT_SYMBOL_GPL(xenbus_transaction_end);
> 
> Hmm, which xs_single() indeed takes a const char *, it then casts away const-
> ness before handing to xs_talkv().

Yes, the cast is needed as xs_talkv() can handle reads and writes. No problem in
this case, as the string is only read by xs_talkv() (write type operation).


Juergen

Re: [PATCH] drivers/xen/xenbus: Replace deprecated strcpy in xenbus_transaction_end

Posted by Thorsten Blum 2 weeks, 3 days ago

On 13. Oct 2025, at 11:24, Jürgen Groß wrote:
> On 13.10.25 10:22, Jan Beulich wrote:
>> On 13.10.2025 09:36, Jürgen Groß wrote:
>>> I would even go further and drop abortstr[] completely:
>>> 
>>> diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c
>>> index 528682bf0c7f..c891af7165f5 100644
>>> --- a/drivers/xen/xenbus/xenbus_xs.c
>>> +++ b/drivers/xen/xenbus/xenbus_xs.c
>>> @@ -550,14 +550,8 @@ EXPORT_SYMBOL_GPL(xenbus_transaction_start);
>>>    */
>>>   int xenbus_transaction_end(struct xenbus_transaction t, int abort)
>>>   {
>>> -       char abortstr[2];
>>> -
>>> -       if (abort)
>>> -               strcpy(abortstr, "F");
>>> -       else
>>> -               strcpy(abortstr, "T");
>>> -
>>> -       return xs_error(xs_single(t, XS_TRANSACTION_END, abortstr, NULL));
>>> +       return xs_error(xs_single(t, XS_TRANSACTION_END, abort ? "F" : "T",
>>> +                       NULL));
>>>   }
>>>   EXPORT_SYMBOL_GPL(xenbus_transaction_end);
>> Hmm, which xs_single() indeed takes a const char *, it then casts away const-
>> ness before handing to xs_talkv().
> 
> Yes, the cast is needed as xs_talkv() can handle reads and writes. No problem in
> this case, as the string is only read by xs_talkv() (write type operation).

I'll submit a v2.

Thanks,
Thorsten