This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v3 1/7] Add __vfscanf_internal and __vfwscanf_internal with flags arguments.
- From: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: "Gabriel F. T. Gomes" <gabriel at inconstante dot eti dot br>
- Cc: libc-alpha at sourceware dot org
- Date: Tue, 4 Dec 2018 15:37:24 -0200
- Subject: Re: [PATCH v3 1/7] Add __vfscanf_internal and __vfwscanf_internal with flags arguments.
- References: <20181115214449.19262-1-gabriel@inconstante.eti.br> <20181115214449.19262-2-gabriel@inconstante.eti.br> <8b9371a2-2473-fff6-f024-a2c9ed639c18@linaro.org> <20181204131459.48b8bdf1@tereshkova>
On 04/12/2018 13:14, Gabriel F. T. Gomes wrote:
> On Thu, 22 Nov 2018, Adhemerval Zanella wrote:
>
>> On 15/11/2018 19:44, Gabriel F. T. Gomes wrote:
>>> From: Zack Weinberg <zackw@panix.com>
>>>
>>
>> The patch looks ok with the exception of the part where it adapts some symbol
>> to set _IO_FLAGS2_SCANF_STD on _flags2 *without* acquiring the lock previously.
>> I also see the idea of second patch is also to get rid of such mechanism, so
>> maybe one option is to combine both patches.
>>
>>> [...]
>>>
>>> int
>>> __isoc99_sscanf (const char *s, const char *format, ...)
>>> {
>>> va_list arg;
>>> int done;
>>> + _IO_strfile sf;
>>> + FILE *f = _IO_strfile_read (&sf, s);
>>> + f->_flags2 |= _IO_FLAGS2_SCANF_STD;
>>>
>>> va_start (arg, format);
>>> - done = __isoc99_vsscanf (s, format, arg);
>>> + done = __vfscanf_internal (f, format, arg, 0);
>>> va_end (arg);
>>>
>>> return done;
>>
>> I think it requires acquire/release the 'f' lock before manipulating the its _flags2.
>> Wouldn't be more consistent to fold the second patch in this set, which replace
>> _IO_FLAGS2_SCANF_STD by SCANF_ISOC99_A, to avoid add the code which would be just
>> removed in a subsequent patch?
>
> I have no problems with folding the two patches, however I did not
> understand why the lock needs to be acquired/released. In this function,
> `sf' is a variable on the stack, so it cannot be accessed elsewhere by
> another thread (compare that against the `stream' argument to
> __isoc99_vfscanf, below, where I agree that the lock is needed).
>
> Did I miss something?
You are right, the change looks correct.
>
>>> diff --git a/stdio-common/isoc99_vfscanf.c
>>> b/stdio-common/isoc99_vfscanf.c index b80e05f8db..c96ca831ae 100644
>>> --- a/stdio-common/isoc99_vfscanf.c
>>> +++ b/stdio-common/isoc99_vfscanf.c
>>> @@ -27,7 +27,7 @@ __isoc99_vfscanf (FILE *stream, const char *format,
>>> va_list args)
>>> _IO_acquire_lock_clear_flags2 (stream);
>>> stream->_flags2 |= _IO_FLAGS2_SCANF_STD;
>>> - done = _IO_vfscanf (stream, format, args, NULL);
>>> + done = __vfscanf_internal (stream, format, args, 0);
>>> _IO_release_lock (stream);
>>> return done;
>>> }
>
> For reference, __isoc99_vfscanf lock acquire/release.
Ack as before.
>
>>> int
>>> __isoc99_vsscanf (const char *string, const char *format, va_list args)
>>> {
>>> - int ret;
>>> _IO_strfile sf;
>>> -#ifdef _IO_MTSAFE_IO
>>> - sf._sbf._f._lock = NULL;
>>> -#endif
>>> - _IO_no_init (&sf._sbf._f, _IO_USER_LOCK, -1, NULL, NULL);
>>> - _IO_JUMPS (&sf._sbf) = &_IO_str_jumps;
>>> - _IO_str_init_static_internal (&sf, (char*)string, 0, NULL);
>>> - sf._sbf._f._flags2 |= _IO_FLAGS2_SCANF_STD;
>>> - ret = _IO_vfscanf (&sf._sbf._f, format, args, NULL);
>>> - return ret;
>>> + FILE *f = _IO_strfile_read (&sf, string);
>>> + f->_flags2 |= _IO_FLAGS2_SCANF_STD;
>>> + return __vfscanf_internal (f, format, args, 0);
>>> }
>>> libc_hidden_def (__isoc99_vsscanf)
>>
>> As before I think it requires to acquire the file lock before setting
>> _flags2.
>
> Same as __isoc99_sscanf.
Ack as before.
>
>>> __isoc99_swscanf (const wchar_t *s, const wchar_t *format, ...)
>>> {
>>> va_list arg;
>>> int done;
>>> + _IO_strfile sf;
>>> + struct _IO_wide_data wd;
>>> + FILE *f = _IO_strfile_readw (&sf, &wd, s);
>>> + f->_flags2 |= _IO_FLAGS2_SCANF_STD;
>>>
>>> va_start (arg, format);
>>> - done = __isoc99_vswscanf (s, format, arg);
>>> + done = __vfwscanf_internal (f, format, arg, 0);
>>> va_end (arg);
>>>
>>> return done;
>>
>> As before I think it requires to acquire the file lock before setting
>> _flags2.
>
> Likewise.
Ack as before.
>
>>> int
>>> __isoc99_vswscanf (const wchar_t *string, const wchar_t *format,
>>> va_list args) {
>>> - int ret;
>>> _IO_strfile sf;
>>> struct _IO_wide_data wd;
>>> -#ifdef _IO_MTSAFE_IO
>>> - sf._sbf._f._lock = NULL;
>>> -#endif
>>> - _IO_no_init (&sf._sbf._f, _IO_USER_LOCK, 0, &wd, &_IO_wstr_jumps);
>>> - _IO_fwide (&sf._sbf._f, 1);
>>> - _IO_wstr_init_static (&sf._sbf._f, (wchar_t *)string, 0, NULL);
>>> - sf._sbf._f._flags2 |= _IO_FLAGS2_SCANF_STD;
>>> - ret = _IO_vfwscanf ((FILE *) &sf._sbf, format, args, NULL);
>>> - return ret;
>>> + FILE *f = _IO_strfile_readw (&sf, &wd, string);
>>> + f->_flags2 |= _IO_FLAGS2_SCANF_STD;
>>> + return __vfwscanf_internal (f, format, args, 0);
>>> }
>>> libc_hidden_def (__isoc99_vswscanf)
>>
>> As before I think it requires to acquire the file lock before setting
>> _flags2.
>
> Likewise.
>
>
> I'm willing to squash the first and second patches anyway... I just want
> to make sure I understand your concern.
>
Current patch looks sorry, sorry for the noise.