This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v3 1/7] Add __vfscanf_internal and __vfwscanf_internal with flags arguments.



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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]