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: "Gabriel F. T. Gomes" <gabriel at inconstante dot eti dot br>
- To: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- Cc: <libc-alpha at sourceware dot org>
- Date: Tue, 4 Dec 2018 13:14:59 -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>
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?
>> 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.
>> 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.
>> __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.
>> 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.