Re: [PATCH v2] tools/9pfsd: Fix build error caused by strerror_r()

From: Henry Wang <xin.wang2@amd.com>
To: Juergen Gross <jgross@suse.com>, <xen-devel@lists.xenproject.org>
Cc: Wei Liu <wl@xen.org>, Anthony PERARD <anthony.perard@citrix.com>,
	Jan Beulich <jbeulich@suse.com>
Subject: Re: [PATCH v2] tools/9pfsd: Fix build error caused by strerror_r()
Date: Thu, 7 Mar 2024 20:26:17 +0800	[thread overview]
Message-ID: <65a2cd92-2a7c-43b8-8257-13115b6f7fd5@amd.com> (raw)
In-Reply-To: <833782ae-9699-492a-9d18-2644873c97c7@suse.com>

Hi Juergen,

On 3/7/2024 7:24 PM, Juergen Gross wrote:
> On 07.03.24 11:38, Henry Wang wrote:
>> Below error can be seen when doing Yocto build of the toolstack:
>>
>> | io.c: In function 'p9_error':
>> | io.c:684:5: error: ignoring return value of 'strerror_r' declared
>>    with attribute 'warn_unused_result' [-Werror=unused-result]
>> |   684 |     strerror_r(err, ring->buffer, ring->ring_size);
>> |       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> | cc1: all warnings being treated as errors
>>
>> Fix the build by using strerror() to replace strerror_r(). Since
>> strerror() is thread-unsafe, use a separate local mutex to protect
>> the action. The steps would then become: Acquire the mutex first,
>> invoke strerror(), copy the string from strerror() to the designated
>> buffer and then drop the mutex.
>>
>> Signed-off-by: Henry Wang <xin.wang2@amd.com>
>> ---
>>   tools/9pfsd/io.c | 12 +++++++++++-
>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/9pfsd/io.c b/tools/9pfsd/io.c
>> index adb887c7d9..2b80c9528d 100644
>> --- a/tools/9pfsd/io.c
>> +++ b/tools/9pfsd/io.c
>> @@ -680,8 +680,18 @@ static bool name_ok(const char *str)
>>   static void p9_error(struct ring *ring, uint16_t tag, uint32_t err)
>>   {
>>       unsigned int erroff;
>> +    static pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
>> +    char *strerror_str;
>> +    RING_IDX strerror_len = 0, copy_len = 0;
>> +
>> +    pthread_mutex_lock(&mutex);
>> +    strerror_str = strerror(err);
>> +    strerror_len = strlen(strerror_str) + 1;
>> +    copy_len = min(strerror_len, ring->ring_size);
>
> Hmm, I think we even _need_ to cap the string earlier.
>
> A string in the 9pfs protocol is a 2 byte length field plus the string.
> In case of a ring larger than 65535 bytes this would mean the result of
> strerror() could (in theory) overflow the string format of 9pfs.
>
> Additionally the string should be a _short_ description of the error, so
> I'd like to suggest to not use ring_size as the upper bound for the 
> string
> length, but a fixed value defined as a macro, e.g.:
>
> #define MAX_ERRSTR_LEN 80

I can use a macro-defined value in v3, sure.

Kind regards,
Henry

> Juergen