Patchwork [04/14] Zero initialize timespec struct explicitly

login
register
mail settings
Submitter Jes Sorensen
Date Aug. 30, 2010, 3:35 p.m.
Message ID <1283182547-26116-5-git-send-email-Jes.Sorensen@redhat.com>
Download mbox | patch
Permalink /patch/63061/
State New
Headers show

Comments

Jes Sorensen - Aug. 30, 2010, 3:35 p.m.
From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 linux-aio.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Anthony Liguori - Aug. 30, 2010, 3:43 p.m.
On 08/30/2010 10:35 AM, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>
> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com>
> ---
>   linux-aio.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/linux-aio.c b/linux-aio.c
> index 68f4b3d..3240996 100644
> --- a/linux-aio.c
> +++ b/linux-aio.c
> @@ -118,7 +118,7 @@ static void qemu_laio_completion_cb(void *opaque)
>           struct io_event events[MAX_EVENTS];
>           uint64_t val;
>           ssize_t ret;
> -        struct timespec ts = { 0 };
> +        struct timespec ts = { 0, 0 };
>    

I don't like these.  What's wrong with { } or { 0 }?  Implicit zeroing 
of members is a critical feature of structure initialization so if there 
is something wrong with this, it's important to know why because 
otherwise we've got a massive amount of broken code.

Regards,

Anthony Liguori

>           int nevents, i;
>
>           do {
>
Jes Sorensen - Aug. 30, 2010, 3:55 p.m.
On 08/30/10 17:43, Anthony Liguori wrote:
> On 08/30/2010 10:35 AM, Jes.Sorensen@redhat.com wrote:
>> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>>
>> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com>
>> ---
>>   linux-aio.c |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/linux-aio.c b/linux-aio.c
>> index 68f4b3d..3240996 100644
>> --- a/linux-aio.c
>> +++ b/linux-aio.c
>> @@ -118,7 +118,7 @@ static void qemu_laio_completion_cb(void *opaque)
>>           struct io_event events[MAX_EVENTS];
>>           uint64_t val;
>>           ssize_t ret;
>> -        struct timespec ts = { 0 };
>> +        struct timespec ts = { 0, 0 };
>>    
> 
> I don't like these.  What's wrong with { } or { 0 }?  Implicit zeroing
> of members is a critical feature of structure initialization so if there
> is something wrong with this, it's important to know why because
> otherwise we've got a massive amount of broken code.

The specific case above is really inconsistent. Either do {} or {0, 0},
doing just {0} means it is initializing just one element in the struct.
That is broken IMHO.

Cheers,
Jes
malc - Aug. 30, 2010, 4:56 p.m.
On Mon, 30 Aug 2010, Anthony Liguori wrote:

> On 08/30/2010 10:35 AM, Jes.Sorensen@redhat.com wrote:
> > From: Jes Sorensen<Jes.Sorensen@redhat.com>
> > 
> > Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com>
> > ---
> >   linux-aio.c |    2 +-
> >   1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/linux-aio.c b/linux-aio.c
> > index 68f4b3d..3240996 100644
> > --- a/linux-aio.c
> > +++ b/linux-aio.c
> > @@ -118,7 +118,7 @@ static void qemu_laio_completion_cb(void *opaque)
> >           struct io_event events[MAX_EVENTS];
> >           uint64_t val;
> >           ssize_t ret;
> > -        struct timespec ts = { 0 };
> > +        struct timespec ts = { 0, 0 };
> >    
> 
> I don't like these.  What's wrong with { } or { 0 }?  Implicit zeroing of
> members is a critical feature of structure initialization so if there is
> something wrong with this, it's important to know why because otherwise we've
> got a massive amount of broken code.
> 

Apart from gcc complaining about fields not being initialized explicitly
there's nothing wrong with it.
malc - Aug. 30, 2010, 5:04 p.m.
On Mon, 30 Aug 2010, Jes Sorensen wrote:

> On 08/30/10 17:43, Anthony Liguori wrote:
> > On 08/30/2010 10:35 AM, Jes.Sorensen@redhat.com wrote:
> >> From: Jes Sorensen<Jes.Sorensen@redhat.com>
> >>
> >> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com>
> >> ---
> >>   linux-aio.c |    2 +-
> >>   1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/linux-aio.c b/linux-aio.c
> >> index 68f4b3d..3240996 100644
> >> --- a/linux-aio.c
> >> +++ b/linux-aio.c
> >> @@ -118,7 +118,7 @@ static void qemu_laio_completion_cb(void *opaque)
> >>           struct io_event events[MAX_EVENTS];
> >>           uint64_t val;
> >>           ssize_t ret;
> >> -        struct timespec ts = { 0 };
> >> +        struct timespec ts = { 0, 0 };
> >>    
> > 
> > I don't like these.  What's wrong with { } or { 0 }?  Implicit zeroing
> > of members is a critical feature of structure initialization so if there
> > is something wrong with this, it's important to know why because
> > otherwise we've got a massive amount of broken code.
> 
> The specific case above is really inconsistent. Either do {} or {0, 0},
> doing just {0} means it is initializing just one element in the struct.
> That is broken IMHO.
> 

No it doesn't mean that. In this particular case all the fields of ts
will be set to zero, for specific wording look at 6.7.9#21
Jes Sorensen - Aug. 30, 2010, 5:38 p.m.
On 08/30/10 18:56, malc wrote:
> On Mon, 30 Aug 2010, Anthony Liguori wrote:
> 
>> On 08/30/2010 10:35 AM, Jes.Sorensen@redhat.com wrote:
>>> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>>> diff --git a/linux-aio.c b/linux-aio.c
>>> index 68f4b3d..3240996 100644
>>> --- a/linux-aio.c
>>> +++ b/linux-aio.c
>>> @@ -118,7 +118,7 @@ static void qemu_laio_completion_cb(void *opaque)
>>>           struct io_event events[MAX_EVENTS];
>>>           uint64_t val;
>>>           ssize_t ret;
>>> -        struct timespec ts = { 0 };
>>> +        struct timespec ts = { 0, 0 };
>>>    
>>
>> I don't like these.  What's wrong with { } or { 0 }?  Implicit zeroing of
>> members is a critical feature of structure initialization so if there is
>> something wrong with this, it's important to know why because otherwise we've
>> got a massive amount of broken code.
>>
> 
> Apart from gcc complaining about fields not being initialized explicitly
> there's nothing wrong with it.

Sure it compiles, it works, but it's not pretty. What does it mean if
you write = { 1 } in the above case?

Anyway the whole point of my patch is this, if we fix things like these,
we are much more likely to be able to catch real bugs using some of
gcc's checking. The patch I submitted is harmless code wise, but it
makes it that bit easier to catch other bugs in the code. In my book
that adds a lot of value.

Jes
Anthony Liguori - Aug. 30, 2010, 5:41 p.m.
On 08/30/2010 12:38 PM, Jes Sorensen wrote:
> On 08/30/10 18:56, malc wrote:
>    
>> On Mon, 30 Aug 2010, Anthony Liguori wrote:
>>
>>      
>>> On 08/30/2010 10:35 AM, Jes.Sorensen@redhat.com wrote:
>>>        
>>>> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>>>> diff --git a/linux-aio.c b/linux-aio.c
>>>> index 68f4b3d..3240996 100644
>>>> --- a/linux-aio.c
>>>> +++ b/linux-aio.c
>>>> @@ -118,7 +118,7 @@ static void qemu_laio_completion_cb(void *opaque)
>>>>            struct io_event events[MAX_EVENTS];
>>>>            uint64_t val;
>>>>            ssize_t ret;
>>>> -        struct timespec ts = { 0 };
>>>> +        struct timespec ts = { 0, 0 };
>>>>
>>>>          
>>> I don't like these.  What's wrong with { } or { 0 }?  Implicit zeroing of
>>> members is a critical feature of structure initialization so if there is
>>> something wrong with this, it's important to know why because otherwise we've
>>> got a massive amount of broken code.
>>>
>>>        
>> Apart from gcc complaining about fields not being initialized explicitly
>> there's nothing wrong with it.
>>      
> Sure it compiles, it works, but it's not pretty. What does it mean if
> you write = { 1 } in the above case?
>    

Initialize first field to 1 and all remaining fields to 0.

That's precisely what it means and there's a lot of code written today 
that relies on this behavior.  No doubt, c99 initializers are an 
improvement but { 0 } still looks better to me than {}.

However, I wouldn't object to replacing {0} with {}.  Avoiding {} in 
favor of memset is crazy though.

Regards,

Anthony Liguori
malc - Aug. 30, 2010, 5:43 p.m.
On Mon, 30 Aug 2010, Jes Sorensen wrote:

> On 08/30/10 18:56, malc wrote:
> > On Mon, 30 Aug 2010, Anthony Liguori wrote:
> > 
> >> On 08/30/2010 10:35 AM, Jes.Sorensen@redhat.com wrote:
> >>> From: Jes Sorensen<Jes.Sorensen@redhat.com>
> >>> diff --git a/linux-aio.c b/linux-aio.c
> >>> index 68f4b3d..3240996 100644
> >>> --- a/linux-aio.c
> >>> +++ b/linux-aio.c
> >>> @@ -118,7 +118,7 @@ static void qemu_laio_completion_cb(void *opaque)
> >>>           struct io_event events[MAX_EVENTS];
> >>>           uint64_t val;
> >>>           ssize_t ret;
> >>> -        struct timespec ts = { 0 };
> >>> +        struct timespec ts = { 0, 0 };
> >>>    
> >>
> >> I don't like these.  What's wrong with { } or { 0 }?  Implicit zeroing of
> >> members is a critical feature of structure initialization so if there is
> >> something wrong with this, it's important to know why because otherwise we've
> >> got a massive amount of broken code.
> >>
> > 
> > Apart from gcc complaining about fields not being initialized explicitly
> > there's nothing wrong with it.
> 
> Sure it compiles, it works, but it's not pretty. What does it mean if
> you write = { 1 } in the above case?

It means exactly what standard says it should mean, i.e. set tv_sec to 1
and tv_nsec to 0.

> 
> Anyway the whole point of my patch is this, if we fix things like these,
> we are much more likely to be able to catch real bugs using some of
> gcc's checking. The patch I submitted is harmless code wise, but it
> makes it that bit easier to catch other bugs in the code. In my book
> that adds a lot of value.

FWIW I wasn't arguing against the patch.

Patch

diff --git a/linux-aio.c b/linux-aio.c
index 68f4b3d..3240996 100644
--- a/linux-aio.c
+++ b/linux-aio.c
@@ -118,7 +118,7 @@  static void qemu_laio_completion_cb(void *opaque)
         struct io_event events[MAX_EVENTS];
         uint64_t val;
         ssize_t ret;
-        struct timespec ts = { 0 };
+        struct timespec ts = { 0, 0 };
         int nevents, i;
 
         do {