| Message ID | 1283182547-26116-5-git-send-email-Jes.Sorensen@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 { >
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
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.
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
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
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
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.
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 {