LinuxLists.cc - Eudyptula Challenge (Task 10)

2014-06-12 10:36:22

by Wahib

[permalink] [raw]

Subject: Eudyptula Challenge (Task 10)

Greetings Linux Kernel Developers!

This is Task 10 of the Eudyptula Challenge.

2014-06-12 10:36:26

by Wahib

[permalink] [raw]

Subject: [PATCH] drivers/staging/usbip/userspace/libsrc/usbip_host_driver.c: fix coding style

Fix coding style issues detected by checkpatch.pl:
1. do not use assignment in if condition
2. line over 80 characters

Signed-off-by: Wahib Faizi <[email protected]>
---
.../usbip/userspace/libsrc/usbip_host_driver.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/usbip/userspace/libsrc/usbip_host_driver.c b/drivers/staging/usbip/userspace/libsrc/usbip_host_driver.c
index 92caef7..bef08d5 100644
--- a/drivers/staging/usbip/userspace/libsrc/usbip_host_driver.c
+++ b/drivers/staging/usbip/userspace/libsrc/usbip_host_driver.c
@@ -47,7 +47,8 @@ static int32_t read_attr_usbip_status(struct usbip_usb_device *udev)
snprintf(status_attr_path, SYSFS_PATH_MAX, "%s/usbip_status",
udev->path);

- if ((fd = open(status_attr_path, O_RDONLY)) < 0) {
+ fd = open(status_attr_path, O_RDONLY);
+ if (fd < 0) {
err("error opening attribute %s", status_attr_path);
return -1;
}
@@ -87,8 +88,8 @@ struct usbip_exported_device *usbip_exported_device_new(const char *sdevpath)
goto err;

/* reallocate buffer to include usb interface data */
- size = sizeof(struct usbip_exported_device) + edev->udev.bNumInterfaces *
- sizeof(struct usbip_usb_interface);
+ size = sizeof(struct usbip_exported_device) +
+ edev->udev.bNumInterfaces * sizeof(struct usbip_usb_interface);

edev_old = edev;
edev = realloc(edev, size);
--
1.7.9.5

2014-06-12 14:10:57

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH] drivers/staging/usbip/userspace/libsrc/usbip_host_driver.c: fix coding style

On Thu, Jun 12, 2014 at 02:35:38PM +0400, Wahib Faizi wrote:
> Fix coding style issues detected by checkpatch.pl:
> 1. do not use assignment in if condition
> 2. line over 80 characters

You are doing two different things here, so please break it up into two
different patches. Can you please do this and resend?

thanks,

greg k-h

2014-06-12 17:33:52

by Wahib

[permalink] [raw]

Subject: [PATCH v2 1/2] drivers/staging/usbip/userspace/libsrc/usbip_host_driver.c: fix coding style

Fix coding style issue "do not use assignment in if condition"
detected by checkpatch.pl in usbip_host_driver.c.

Signed-off-by: Wahib Faizi <[email protected]>
---
.../usbip/userspace/libsrc/usbip_host_driver.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/usbip/userspace/libsrc/usbip_host_driver.c b/drivers/staging/usbip/userspace/libsrc/usbip_host_driver.c
index 92caef7..32b8f52 100644
--- a/drivers/staging/usbip/userspace/libsrc/usbip_host_driver.c
+++ b/drivers/staging/usbip/userspace/libsrc/usbip_host_driver.c
@@ -47,7 +47,8 @@ static int32_t read_attr_usbip_status(struct usbip_usb_device *udev)
snprintf(status_attr_path, SYSFS_PATH_MAX, "%s/usbip_status",
udev->path);

- if ((fd = open(status_attr_path, O_RDONLY)) < 0) {
+ fd = open(status_attr_path, O_RDONLY);
+ if (fd < 0) {
err("error opening attribute %s", status_attr_path);
return -1;
}
--
1.7.9.5

2014-06-12 17:33:56

by Wahib

[permalink] [raw]

Subject: Split patch into 2 logical chunks

Fix coding style issues detected by checkpatch.pl in usbip_host_driver.c.

v2: Split patch into logical chunks, as suggested by
Greg Kroah-Hartman <[email protected]>

[PATCH v2 1/2]
Fix coding style issue "do not use assignment in if condition"
detected by checkpatch.pl in usbip_host_driver.c.

[PATCH v2 2/2]
Fix coding style issue "line over 80 characters"
detected by checkpatch.pl in usbip_host_driver.c.

2014-06-12 17:34:27

by Wahib

[permalink] [raw]

Subject: [PATCH v2 2/2] drivers/staging/usbip/userspace/libsrc/usbip_host_driver.c: fix coding style

Fix coding style issue "line over 80 characters"
detected by checkpatch.pl in usbip_host_driver.c.

Signed-off-by: Wahib Faizi <[email protected]>
---
.../usbip/userspace/libsrc/usbip_host_driver.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/usbip/userspace/libsrc/usbip_host_driver.c b/drivers/staging/usbip/userspace/libsrc/usbip_host_driver.c
index 32b8f52..bef08d5 100644
--- a/drivers/staging/usbip/userspace/libsrc/usbip_host_driver.c
+++ b/drivers/staging/usbip/userspace/libsrc/usbip_host_driver.c
@@ -88,8 +88,8 @@ struct usbip_exported_device *usbip_exported_device_new(const char *sdevpath)
goto err;

/* reallocate buffer to include usb interface data */
- size = sizeof(struct usbip_exported_device) + edev->udev.bNumInterfaces *
- sizeof(struct usbip_usb_interface);
+ size = sizeof(struct usbip_exported_device) +
+ edev->udev.bNumInterfaces * sizeof(struct usbip_usb_interface);

edev_old = edev;
edev = realloc(edev, size);
--
1.7.9.5

2014-06-12 17:50:11

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] drivers/staging/usbip/userspace/libsrc/usbip_host_driver.c: fix coding style

On Thu, Jun 12, 2014 at 09:32:19PM +0400, Wahib Faizi wrote:
> Fix coding style issue "do not use assignment in if condition"
> detected by checkpatch.pl in usbip_host_driver.c.
>
> Signed-off-by: Wahib Faizi <[email protected]>

Both of these patches have the same Subject: line, which isn't good as
it doesn't make much sense.

You can also shorten it a lot, for example, this one should be:
Subject: staging: usbip: usbip_host_driver.c: fix if assignment style issue

Care to redo both of these that way?

thanks,

greg k-h

2014-06-12 17:55:21

by Joe Perches

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] drivers/staging/usbip/userspace/libsrc/usbip_host_driver.c: fix coding style

On Thu, 2014-06-12 at 10:54 -0700, Greg Kroah-Hartman wrote:
> On Thu, Jun 12, 2014 at 09:32:19PM +0400, Wahib Faizi wrote:
> > Fix coding style issue "do not use assignment in if condition"
> > detected by checkpatch.pl in usbip_host_driver.c.
> >
> > Signed-off-by: Wahib Faizi <[email protected]>
>
> Both of these patches have the same Subject: line, which isn't good as
> it doesn't make much sense.
>
> You can also shorten it a lot, for example, this one should be:
> Subject: staging: usbip: usbip_host_driver.c: fix if assignment style issue

True. usbip_host_driver.c doesn't add much though.

It could even be something like:

staging: usbip: avoid assignment in if

2014-06-12 19:11:09

by Wahib

[permalink] [raw]

Subject: Fix subject line

Fix coding style issues detected by checkpatch.pl in usbip_host_driver.c.

v3: Shorten subject line, as suggested by
Greg Kroah-Hartman <[email protected]>,
Joe Perches <[email protected]>

v2: Split patch into logical chunks, as suggested by
Greg Kroah-Hartman <[email protected]>

2014-06-12 19:11:21

by Wahib

[permalink] [raw]

Subject: [PATCH 2/2] staging: usbip: usbip_host_driver.c: fix line over 80 characters

2014-06-12 19:11:39

by Wahib

[permalink] [raw]

Subject: [PATCH 1/2] staging: usbip: usbip_host_driver.c: avoid assignment in if

2014-06-12 19:41:09

by Wahib

[permalink] [raw]

Subject: [PATCH v3 0/2] Fix subject line

2014-06-12 19:41:13

by Wahib

[permalink] [raw]

Subject: [PATCH v3 2/2] staging: usbip: usbip_host_driver.c: fix line over 80 characters

2014-06-12 19:41:40

by Wahib

[permalink] [raw]

Subject: [PATCH v3 1/2] staging: usbip: usbip_host_driver.c: avoid assignment in if

2014-06-12 20:25:42

by Davidlohr Bueso

[permalink] [raw]

Subject: Re: [PATCH v3 0/2] Fix subject line

On Thu, 2014-06-12 at 23:40 +0400, Wahib Faizi wrote:
> Fix coding style issues detected by checkpatch.pl in usbip_host_driver.c.

Sorry but unless bundled with something more meaningful, I really don't
see the value in these changes. I certainly don't want to discourage
folks or anything, but just testing other patches is a lot more helpful
than this.

I haven't followed much about the Eudyptula Challenge, but I hope other
assignments are more involved than this.

Thanks,
Davidlohr

2014-06-12 20:31:45

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH v3 0/2] Fix subject line

On Thu, Jun 12, 2014 at 01:25:34PM -0700, Davidlohr Bueso wrote:
> On Thu, 2014-06-12 at 23:40 +0400, Wahib Faizi wrote:
> > Fix coding style issues detected by checkpatch.pl in usbip_host_driver.c.
>
> Sorry but unless bundled with something more meaningful, I really don't
> see the value in these changes. I certainly don't want to discourage
> folks or anything, but just testing other patches is a lot more helpful
> than this.

When the staging code is still needing basic fixes like this, it is
"meaningful" to do patches that clean up stuff like this. That's what
the staging tree is for.

thanks,

greg k-h

2014-06-12 20:48:43

by Davidlohr Bueso

[permalink] [raw]

Subject: Re: [PATCH v3 0/2] Fix subject line

On Thu, 2014-06-12 at 13:35 -0700, Greg Kroah-Hartman wrote:
> On Thu, Jun 12, 2014 at 01:25:34PM -0700, Davidlohr Bueso wrote:
> > On Thu, 2014-06-12 at 23:40 +0400, Wahib Faizi wrote:
> > > Fix coding style issues detected by checkpatch.pl in usbip_host_driver.c.
> >
> > Sorry but unless bundled with something more meaningful, I really don't
> > see the value in these changes. I certainly don't want to discourage
> > folks or anything, but just testing other patches is a lot more helpful
> > than this.
>
> When the staging code is still needing basic fixes like this, it is
> "meaningful" to do patches that clean up stuff like this. That's what
> the staging tree is for.

Sure, but "making checkpatch happy just to make checkpatch happy" isn't
a good justification, even for staging. Patch 1 does have value in that
it helps avoid silly bugs, but take patch 2/2, we end-up saving just a
few spaces... Anyways, just my 2 cents.

Thanks,
Davidlohr

2014-06-12 21:31:00

by Dan Carpenter

[permalink] [raw]

Subject: Re: [PATCH v3 0/2] Fix subject line

On Thu, Jun 12, 2014 at 01:48:38PM -0700, Davidlohr Bueso wrote:
> On Thu, 2014-06-12 at 13:35 -0700, Greg Kroah-Hartman wrote:
> > On Thu, Jun 12, 2014 at 01:25:34PM -0700, Davidlohr Bueso wrote:
> > > On Thu, 2014-06-12 at 23:40 +0400, Wahib Faizi wrote:
> > > > Fix coding style issues detected by checkpatch.pl in usbip_host_driver.c.
> > >
> > > Sorry but unless bundled with something more meaningful, I really don't
> > > see the value in these changes. I certainly don't want to discourage
> > > folks or anything, but just testing other patches is a lot more helpful
> > > than this.
> >
> > When the staging code is still needing basic fixes like this, it is
> > "meaningful" to do patches that clean up stuff like this. That's what
> > the staging tree is for.
>
> Sure, but "making checkpatch happy just to make checkpatch happy" isn't
> a good justification, even for staging.

It actually is. Fighting against checkpatch is a losers battle. Our
way more efficient.

1) We do need to fix this checkpatch warning before it moves out of
staging.
2) NAKing patches is actually a lot of stress for everyone. It stresses
out submitters and drives them away. It stresses out the
maintainers because we feel bad. That stress is bad when it is
pointless.
3) This patch is ok.
4) If we don't apply this patch then someone else will send the exact
same patch or something worse until we apply something.
5) The v3 of this patch takes under 30 seconds to review and apply.
6) Newbies feel happy when their patch gets merged and that is good.

The other thing is that if you start asking "Is this patch meaningful"
then it makes applying any patch into a big question about meaning and
it tires you out.

In staging we have clear rules for when a patch is going to be applied
and everyone understands the rules. It means that if Greg is traveling
then you know which patches he is going to apply in what order and life
is easier because it is more predictable.

regards,
dan carpenter

2014-06-15 20:29:12

by Wahib

[permalink] [raw]

Subject: Re: [PATCH v3 0/2] Fix subject line

Hi Davidlohr!

Don't worry. I am not discouraged. :)

I understand your concern that the patch feels superficial. It's what the task
asked us to do. I suspect the author(s) of the Eudyptula Challenge designed
this task to get us involved with the Linux kernel community.

I have been looking for a bug in 'usbip' to bundle with my patch. The only bug
that I could find (as of 16-Jun-2014) on bugzilla.kernel.org related
to 'usbip' is Bug 77191.

Bug 77191 - ftdi-sio (usbserial) over usbip hung after disconnect while in use
(https://bugzilla.kernel.org/show_bug.cgi?id=77191)

Sadly the bug doesn't seem to be newbie friendly as it requires special setup
to reproduce and requires the developer to know about the intricacies of the
'usbip' subsystem. I understand that people commiting their specialized
knowledge is how the kernel development actually works. But USB is not my
speciality (yet).

Could you point me to more beginner friendly bugs which aren't superficial code
cleanups?

Can I bundle fixes to subsystems other than 'usbip' with the code cleanup patch
for 'usbip'?

Regards,
Wahib

On Fri, Jun 13, 2014 at 12:25 AM, Davidlohr Bueso <[email protected]> wrote:
> On Thu, 2014-06-12 at 23:40 +0400, Wahib Faizi wrote:
>> Fix coding style issues detected by checkpatch.pl in usbip_host_driver.c.
>
> Sorry but unless bundled with something more meaningful, I really don't
> see the value in these changes. I certainly don't want to discourage
> folks or anything, but just testing other patches is a lot more helpful
> than this.
>
> I haven't followed much about the Eudyptula Challenge, but I hope other
> assignments are more involved than this.
>
> Thanks,
> Davidlohr
>

2014-06-18 00:30:22

by Davidlohr Bueso

[permalink] [raw]

Subject: Re: [PATCH v3 0/2] Fix subject line

On Mon, 2014-06-16 at 00:28 +0400, Wahib wrote:
> Hi Davidlohr!
>
> Don't worry. I am not discouraged. :)
>
> I understand your concern that the patch feels superficial. It's what the task
> asked us to do. I suspect the author(s) of the Eudyptula Challenge designed
> this task to get us involved with the Linux kernel community.

If you feel you actually learn something by doing the Eudyptula
Challenge, then good for you, and by all means continue doing so. As I
mentioned, I know almost zero about it, but can say for sure that
cleanups alone won't get you very far in the community and you will
probably get bored sooner or later -- which is unfortunate as kernel
hacking fascinating.

>
> I have been looking for a bug in 'usbip' to bundle with my patch. The only bug
> that I could find (as of 16-Jun-2014) on bugzilla.kernel.org related
> to 'usbip' is Bug 77191.
>
> Bug 77191 - ftdi-sio (usbserial) over usbip hung after disconnect while in use
> (https://bugzilla.kernel.org/show_bug.cgi?id=77191)
>
> Sadly the bug doesn't seem to be newbie friendly as it requires special setup
> to reproduce and requires the developer to know about the intricacies of the
> 'usbip' subsystem. I understand that people commiting their specialized
> knowledge is how the kernel development actually works. But USB is not my
> speciality (yet).
>
> Could you point me to more beginner friendly bugs which aren't superficial code
> cleanups?

There are plenty of bugs out there, and not so hard to find -- just push
some module/filesystem/subsystem/testcase hard enough to make it do
things it shouldn't. Run linux-next on trinity and fix/report the bugs,
etc.

imho testing and reading other people's patches is perhaps the best way
to do useful things, get acquainted with the code and get experience
with lkml and the process behind kernel development.

> Can I bundle fixes to subsystems other than 'usbip' with the code cleanup patch
> for 'usbip'?

Probably not, unless it's a tree-wide change.

Hope this helps,
Davidlohr