Hello everyone,
This is a large patchset that replaces struct memcpy with struct assignment,
whenever possible at drivers/media.
The patches are hand applied and every change has been thoroughly reviewed.
However, to avoid regressions and angry users we'd like to have Acks
from maintainers.
A simplified version of the semantic match that finds
this problem is as follows: (http://coccinelle.lip6.fr/)
// <smpl>
@@
identifier struct_name;
struct struct_name to;
struct struct_name from;
expression E;
@@
-memcpy(&(to), &(from), E);
+to = from;
// </smpl>
If you're thinking this change is very minor and doesn't worh the pain,
you might change your opinion reading this report from Dan Carpenter:
http://comments.gmane.org/gmane.linux.drivers.video-input-infrastructure/49553
The report clearly shows how copy-paste programming paradigm, combined with
lack of memcpy type-safety can lead to very strange code.
Not to mention, using struct assignment instead of memcpy
is by far more readable.
Comments, feedback and flames are welcome. Thanks!
Peter Senna Tschudin, Ezequiel Garcia (23):
wl128x: Replace memcpy with struct assignment
radio-wl1273: Replace memcpy with struct assignment
dvb-frontends: Replace memcpy with struct assignment
dvb-core: Replace memcpy with struct assignment
bttv: Replace memcpy with struct assignment
cx18: Replace memcpy with struct assignment
cx23885: Replace memcpy with struct assignment
cx88: Replace memcpy with struct assignment
ivtv: Replace memcpy with struct assignment
tuners/tda18271: Replace memcpy with struct assignment
tuners/xc2028: Replace memcpy with struct assignment
tuners/xc4000: Replace memcpy with struct assignment
au0828: Replace memcpy with struct assignment
dvb-usb/friio-fe: Replace memcpy with struct assignment
zr36067: Replace memcpy with struct assignment
cx25840: Replace memcpy with struct assignment
hdpvr: Replace memcpy with struct assignment
pvrusb2: Replace memcpy with struct assignment
pwc: Replace memcpy with struct assignment
sn9c102: Replace memcpy with struct assignment
usbvision: Replace memcpy with struct assignment
cx231xx: Replace memcpy with struct assignment
uvc: Replace memcpy with struct assignment
drivers/media/dvb-core/dvb_frontend.c | 2 +-
drivers/media/dvb-frontends/cx24116.c | 2 +-
drivers/media/dvb-frontends/drxd_hard.c | 5 ++---
drivers/media/dvb-frontends/stv0299.c | 2 +-
drivers/media/i2c/cx25840/cx25840-ir.c | 6 ++----
drivers/media/pci/bt8xx/bttv-i2c.c | 3 +--
drivers/media/pci/cx18/cx18-i2c.c | 6 ++----
drivers/media/pci/cx23885/cx23885-video.c | 3 +--
drivers/media/pci/cx23885/cx23888-ir.c | 6 ++----
drivers/media/pci/cx88/cx88-cards.c | 2 +-
drivers/media/pci/cx88/cx88-i2c.c | 3 +--
drivers/media/pci/cx88/cx88-vp3054-i2c.c | 3 +--
drivers/media/pci/ivtv/ivtv-i2c.c | 12 ++++--------
drivers/media/pci/zoran/zoran_card.c | 3 +--
drivers/media/radio/radio-wl1273.c | 3 +--
drivers/media/radio/wl128x/fmdrv_common.c | 3 +--
drivers/media/tuners/tda18271-maps.c | 6 ++----
drivers/media/tuners/tuner-xc2028.c | 2 +-
drivers/media/tuners/xc4000.c | 2 +-
drivers/media/usb/au0828/au0828-cards.c | 2 +-
drivers/media/usb/au0828/au0828-i2c.c | 9 +++------
drivers/media/usb/cx231xx/cx231xx-cards.c | 2 +-
drivers/media/usb/cx231xx/cx231xx-video.c | 3 +--
drivers/media/usb/dvb-usb/friio-fe.c | 5 ++---
drivers/media/usb/hdpvr/hdpvr-i2c.c | 3 +--
drivers/media/usb/pvrusb2/pvrusb2-encoder.c | 3 +--
drivers/media/usb/pvrusb2/pvrusb2-i2c-core.c | 4 ++--
drivers/media/usb/pvrusb2/pvrusb2-v4l2.c | 2 +-
drivers/media/usb/pwc/pwc-if.c | 2 +-
drivers/media/usb/sn9c102/sn9c102_core.c | 4 ++--
drivers/media/usb/usbvision/usbvision-i2c.c | 3 +--
drivers/media/usb/uvc/uvc_v4l2.c | 6 +++---
32 files changed, 47 insertions(+), 75 deletions(-)
Ezequiel
On Tue, Oct 23, 2012 at 4:57 PM, Ezequiel Garcia <[email protected]> wrote:
> Hello everyone,
>
> This is a large patchset that replaces struct memcpy with struct assignment,
> whenever possible at drivers/media.
>
> The patches are hand applied and every change has been thoroughly reviewed.
> However, to avoid regressions and angry users we'd like to have Acks
> from maintainers.
>
> A simplified version of the semantic match that finds
> this problem is as follows: (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @@
> identifier struct_name;
> struct struct_name to;
> struct struct_name from;
> expression E;
> @@
> -memcpy(&(to), &(from), E);
> +to = from;
> // </smpl>
>
> If you're thinking this change is very minor and doesn't worh the pain,
> you might change your opinion reading this report from Dan Carpenter:
>
> http://comments.gmane.org/gmane.linux.drivers.video-input-infrastructure/49553
>
> The report clearly shows how copy-paste programming paradigm, combined with
> lack of memcpy type-safety can lead to very strange code.
>
> Not to mention, using struct assignment instead of memcpy
> is by far more readable.
>
> Comments, feedback and flames are welcome. Thanks!
>
> Peter Senna Tschudin, Ezequiel Garcia (23):
> wl128x: Replace memcpy with struct assignment
> radio-wl1273: Replace memcpy with struct assignment
> dvb-frontends: Replace memcpy with struct assignment
> dvb-core: Replace memcpy with struct assignment
> bttv: Replace memcpy with struct assignment
> cx18: Replace memcpy with struct assignment
> cx23885: Replace memcpy with struct assignment
> cx88: Replace memcpy with struct assignment
> ivtv: Replace memcpy with struct assignment
> tuners/tda18271: Replace memcpy with struct assignment
> tuners/xc2028: Replace memcpy with struct assignment
> tuners/xc4000: Replace memcpy with struct assignment
> au0828: Replace memcpy with struct assignment
> dvb-usb/friio-fe: Replace memcpy with struct assignment
> zr36067: Replace memcpy with struct assignment
> cx25840: Replace memcpy with struct assignment
> hdpvr: Replace memcpy with struct assignment
> pvrusb2: Replace memcpy with struct assignment
> pwc: Replace memcpy with struct assignment
> sn9c102: Replace memcpy with struct assignment
> usbvision: Replace memcpy with struct assignment
> cx231xx: Replace memcpy with struct assignment
> uvc: Replace memcpy with struct assignment
>
> drivers/media/dvb-core/dvb_frontend.c | 2 +-
> drivers/media/dvb-frontends/cx24116.c | 2 +-
> drivers/media/dvb-frontends/drxd_hard.c | 5 ++---
> drivers/media/dvb-frontends/stv0299.c | 2 +-
> drivers/media/i2c/cx25840/cx25840-ir.c | 6 ++----
> drivers/media/pci/bt8xx/bttv-i2c.c | 3 +--
> drivers/media/pci/cx18/cx18-i2c.c | 6 ++----
> drivers/media/pci/cx23885/cx23885-video.c | 3 +--
> drivers/media/pci/cx23885/cx23888-ir.c | 6 ++----
> drivers/media/pci/cx88/cx88-cards.c | 2 +-
> drivers/media/pci/cx88/cx88-i2c.c | 3 +--
> drivers/media/pci/cx88/cx88-vp3054-i2c.c | 3 +--
> drivers/media/pci/ivtv/ivtv-i2c.c | 12 ++++--------
> drivers/media/pci/zoran/zoran_card.c | 3 +--
> drivers/media/radio/radio-wl1273.c | 3 +--
> drivers/media/radio/wl128x/fmdrv_common.c | 3 +--
> drivers/media/tuners/tda18271-maps.c | 6 ++----
> drivers/media/tuners/tuner-xc2028.c | 2 +-
> drivers/media/tuners/xc4000.c | 2 +-
> drivers/media/usb/au0828/au0828-cards.c | 2 +-
> drivers/media/usb/au0828/au0828-i2c.c | 9 +++------
> drivers/media/usb/cx231xx/cx231xx-cards.c | 2 +-
> drivers/media/usb/cx231xx/cx231xx-video.c | 3 +--
> drivers/media/usb/dvb-usb/friio-fe.c | 5 ++---
> drivers/media/usb/hdpvr/hdpvr-i2c.c | 3 +--
> drivers/media/usb/pvrusb2/pvrusb2-encoder.c | 3 +--
> drivers/media/usb/pvrusb2/pvrusb2-i2c-core.c | 4 ++--
> drivers/media/usb/pvrusb2/pvrusb2-v4l2.c | 2 +-
> drivers/media/usb/pwc/pwc-if.c | 2 +-
> drivers/media/usb/sn9c102/sn9c102_core.c | 4 ++--
> drivers/media/usb/usbvision/usbvision-i2c.c | 3 +--
> drivers/media/usb/uvc/uvc_v4l2.c | 6 +++---
> 32 files changed, 47 insertions(+), 75 deletions(-)
>
Hi Mauro,
Given we're very near merge window, I'm wondering if you're
considering picking this series.
There's no rush, but if there's anything to review, please let me know.
Thanks!
Ezequiel
Em Wed, 28 Nov 2012 16:06:31 -0300
Ezequiel Garcia <[email protected]> escreveu:
> On Tue, Oct 23, 2012 at 4:57 PM, Ezequiel Garcia <[email protected]> wrote:
> > Hello everyone,
> >
> > This is a large patchset that replaces struct memcpy with struct assignment,
> > whenever possible at drivers/media.
> >
> > The patches are hand applied and every change has been thoroughly reviewed.
> > However, to avoid regressions and angry users we'd like to have Acks
> > from maintainers.
> >
> > A simplified version of the semantic match that finds
> > this problem is as follows: (http://coccinelle.lip6.fr/)
> >
> > // <smpl>
> > @@
> > identifier struct_name;
> > struct struct_name to;
> > struct struct_name from;
> > expression E;
> > @@
> > -memcpy(&(to), &(from), E);
> > +to = from;
> > // </smpl>
> >
> > If you're thinking this change is very minor and doesn't worh the pain,
> > you might change your opinion reading this report from Dan Carpenter:
> >
> > http://comments.gmane.org/gmane.linux.drivers.video-input-infrastructure/49553
> >
> > The report clearly shows how copy-paste programming paradigm, combined with
> > lack of memcpy type-safety can lead to very strange code.
> >
> > Not to mention, using struct assignment instead of memcpy
> > is by far more readable.
> >
> > Comments, feedback and flames are welcome. Thanks!
> >
> > Peter Senna Tschudin, Ezequiel Garcia (23):
> > wl128x: Replace memcpy with struct assignment
> > radio-wl1273: Replace memcpy with struct assignment
> > dvb-frontends: Replace memcpy with struct assignment
> > dvb-core: Replace memcpy with struct assignment
> > bttv: Replace memcpy with struct assignment
> > cx18: Replace memcpy with struct assignment
> > cx23885: Replace memcpy with struct assignment
> > cx88: Replace memcpy with struct assignment
> > ivtv: Replace memcpy with struct assignment
> > tuners/tda18271: Replace memcpy with struct assignment
> > tuners/xc2028: Replace memcpy with struct assignment
> > tuners/xc4000: Replace memcpy with struct assignment
> > au0828: Replace memcpy with struct assignment
> > dvb-usb/friio-fe: Replace memcpy with struct assignment
> > zr36067: Replace memcpy with struct assignment
> > cx25840: Replace memcpy with struct assignment
> > hdpvr: Replace memcpy with struct assignment
> > pvrusb2: Replace memcpy with struct assignment
> > pwc: Replace memcpy with struct assignment
> > sn9c102: Replace memcpy with struct assignment
> > usbvision: Replace memcpy with struct assignment
> > cx231xx: Replace memcpy with struct assignment
> > uvc: Replace memcpy with struct assignment
> >
> > drivers/media/dvb-core/dvb_frontend.c | 2 +-
> > drivers/media/dvb-frontends/cx24116.c | 2 +-
> > drivers/media/dvb-frontends/drxd_hard.c | 5 ++---
> > drivers/media/dvb-frontends/stv0299.c | 2 +-
> > drivers/media/i2c/cx25840/cx25840-ir.c | 6 ++----
> > drivers/media/pci/bt8xx/bttv-i2c.c | 3 +--
> > drivers/media/pci/cx18/cx18-i2c.c | 6 ++----
> > drivers/media/pci/cx23885/cx23885-video.c | 3 +--
> > drivers/media/pci/cx23885/cx23888-ir.c | 6 ++----
> > drivers/media/pci/cx88/cx88-cards.c | 2 +-
> > drivers/media/pci/cx88/cx88-i2c.c | 3 +--
> > drivers/media/pci/cx88/cx88-vp3054-i2c.c | 3 +--
> > drivers/media/pci/ivtv/ivtv-i2c.c | 12 ++++--------
> > drivers/media/pci/zoran/zoran_card.c | 3 +--
> > drivers/media/radio/radio-wl1273.c | 3 +--
> > drivers/media/radio/wl128x/fmdrv_common.c | 3 +--
> > drivers/media/tuners/tda18271-maps.c | 6 ++----
> > drivers/media/tuners/tuner-xc2028.c | 2 +-
> > drivers/media/tuners/xc4000.c | 2 +-
> > drivers/media/usb/au0828/au0828-cards.c | 2 +-
> > drivers/media/usb/au0828/au0828-i2c.c | 9 +++------
> > drivers/media/usb/cx231xx/cx231xx-cards.c | 2 +-
> > drivers/media/usb/cx231xx/cx231xx-video.c | 3 +--
> > drivers/media/usb/dvb-usb/friio-fe.c | 5 ++---
> > drivers/media/usb/hdpvr/hdpvr-i2c.c | 3 +--
> > drivers/media/usb/pvrusb2/pvrusb2-encoder.c | 3 +--
> > drivers/media/usb/pvrusb2/pvrusb2-i2c-core.c | 4 ++--
> > drivers/media/usb/pvrusb2/pvrusb2-v4l2.c | 2 +-
> > drivers/media/usb/pwc/pwc-if.c | 2 +-
> > drivers/media/usb/sn9c102/sn9c102_core.c | 4 ++--
> > drivers/media/usb/usbvision/usbvision-i2c.c | 3 +--
> > drivers/media/usb/uvc/uvc_v4l2.c | 6 +++---
> > 32 files changed, 47 insertions(+), 75 deletions(-)
> >
>
> Hi Mauro,
>
> Given we're very near merge window, I'm wondering if you're
> considering picking this series.
> There's no rush, but if there's anything to review, please let me know.
Hi Ezequiel,
Thanks for your offering.
There are 400+ patches pending today at patchwork. I doubt I'll have enough
time for all of them, so, I'll skip cleanup patches like the above, in order
to try to focus on bug fixes and patches that brings new functionality to
existing code and with a low risk of breaking anything.
Next year, we'll start implementing the sub-maintainers, and, with their
help, I suspect we'll be able to finally apply those patches.
If you want us to help, feel free to review/test the individual patches
submitted by non-maintainers to the ML. We tend to apply faster patches
that are more reviewed.
Hmm... well, actually it is just the opposite: we explicitly delay
not-reviewed patches for unmaintained/bad maintained drivers,
to see if someone acks or nacks them after testing.
Regards,
Mauro
On Wed, Nov 28, 2012 at 4:55 PM, Mauro Carvalho Chehab
<[email protected]> wrote:
[...]
>
> There are 400+ patches pending today at patchwork. I doubt I'll have enough
> time for all of them, so, I'll skip cleanup patches like the above, in order
> to try to focus on bug fixes and patches that brings new functionality to
> existing code and with a low risk of breaking anything.
>
I understand completely.
> Next year, we'll start implementing the sub-maintainers, and, with their
> help, I suspect we'll be able to finally apply those patches.
>
> If you want us to help, feel free to review/test the individual patches
> submitted by non-maintainers to the ML. We tend to apply faster patches
> that are more reviewed.
>
> Hmm... well, actually it is just the opposite: we explicitly delay
> not-reviewed patches for unmaintained/bad maintained drivers,
> to see if someone acks or nacks them after testing.
>
I'll try to help whenever possible.
Thanks,
Ezequiel
On Wed, 2012-11-28 at 16:06 -0300, Ezequiel Garcia wrote:
> On Tue, Oct 23, 2012 at 4:57 PM, Ezequiel Garcia <[email protected]> wrote:
> > This is a large patchset that replaces struct memcpy with struct assignment,
> > whenever possible at drivers/media.
[]
> > A simplified version of the semantic match that finds
> > this problem is as follows: (http://coccinelle.lip6.fr/)
> >
> > // <smpl>
> > @@
> > identifier struct_name;
> > struct struct_name to;
> > struct struct_name from;
> > expression E;
> > @@
> > -memcpy(&(to), &(from), E);
> > +to = from;
> > // </smpl>
[]
> > Comments, feedback and flames are welcome. Thanks!
[]
> Given we're very near merge window, I'm wondering if you're
> considering picking this series.
I think you should verify that sizeof is any of
sizeof(struct struct_name) or sizeof(*to) or sizeof(*from)
and highlight any entries that aren't.