davidker / unisys

Master repository for new changes to drivers/staging/unisys and drivers/visorbus
Other
2 stars 1 forks source link

Remove unnecesary blank lines and fixup comments #132

Open ghost opened 7 years ago

ghost commented 7 years ago
> }
> 
> /**
>  * parahotplug_process_list() - remove any request from the list that's been on
>  *                              there too long and respond with an error
>  */
> static void
> parahotplug_process_list(void)
> {
>   struct list_head *pos;
>   struct list_head *tmp;
> 
>   spin_lock(¶hotplug_request_list_lock);
> 
>   list_for_each_safe(pos, tmp, ¶hotplug_request_list) {
>       struct parahotplug_request *req =
>           list_entry(pos, struct parahotplug_request, list);
> 
>       if (!time_after_eq(jiffies, req->expiration))
>           continue;
> 
>       list_del(pos);
>       if (req->msg.hdr.flags.response_expected)
>           controlvm_respond_physdev_changestate(
>               &req->msg.hdr,
>               CONTROLVM_RESP_ERROR_DEVICE_UDEV_TIMEOUT,
>               req->msg.cmd.device_change_state.state);
>       parahotplug_request_destroy(req);
>   }
> 
>   spin_unlock(¶hotplug_request_list_lock);
> }
> 
> /**
>  * parahotplug_request_complete() - mark request as complete
>  * @id:     the id of the request
>  * @active: indicates whether the request is assigned to active partition
>  *
>  * Called from the /proc handler, which means the user script has
>  * finished the enable/disable. Find the matching identifier, and
>  * respond to the CONTROLVM message with success.
>  *
>  * Return: 0 on success or -EINVAL on failure
>  */
> static int
> parahotplug_request_complete(int id, u16 active)
> {
>   struct list_head *pos;
>   struct list_head *tmp;
> 
>   spin_lock(¶hotplug_request_list_lock);
> 
>   /* Look for a request matching "id". */
>   list_for_each_safe(pos, tmp, ¶hotplug_request_list) {
>       struct parahotplug_request *req =
>           list_entry(pos, struct parahotplug_request, list);
>       if (req->id == id) {
>           /*
>            * Found a match. Remove it from the list and
>            * respond.
>            */
>           list_del(pos);
>           spin_unlock(¶hotplug_request_list_lock);
>           req->msg.cmd.device_change_state.state.active = active;
>           if (req->msg.hdr.flags.response_expected)
>               controlvm_respond_physdev_changestate(
>                   &req->msg.hdr, CONTROLVM_RESP_SUCCESS,
>                   req->msg.cmd.device_change_state.state);
>           parahotplug_request_destroy(req);
>           return 0;
>       }
>   }
> 
>   spin_unlock(¶hotplug_request_list_lock);
>   return -EINVAL;
> }
> 
> /**
>  * parahotplug_process_message() - enables or disables a PCI device by kicking
>  *                                 off a udev script
>  * @inmsg: the message indicating whether to enable or disable
>  */
> static void
> parahotplug_process_message(struct controlvm_message *inmsg)
> {
>   struct parahotplug_request *req;
> 
>   req = parahotplug_request_create(inmsg);
> 

No blank line needed

> 
>   if (inmsg->cmd.device_change_state.state.active) {
>       /*
>        * For enable messages, just respond with success
>        * right away. This is a bit of a hack, but there are
>        * issues with the early enable messages we get (with
>        * either the udev script not detecting that the device
>        * is up, or not getting called at all). Fortunately
>        * the messages that get lost don't matter anyway, as
>        *
>        * devices are automatically enabled at
>        * initialization.

Blank line in the comment?

ghost commented 7 years ago

The bulk patches that address this issue took a rather unconventional route into being submitted upstream. They were integrated into branch "upstream-next", and reviewed while in that branch. Once these patches reach their final form, I'll link them in this issue. The original patches are kept in branch githubissue-132-upstream-next-TurningWrenches (i.e. these patches don't contain the feedback from the reviews that occurred).

A patch or two got left behind its friends. Those patches will be a part of a new branch; the integration of the aforementioned patches causes merge conflicts in my old local copy, and at this point its more effective from both a functional and organizational perspective to just make a new branch: githubissue-132-upstream-next-TurningWrenches_PART_II.

davidker commented 7 years ago

Is that branch ready for me to pull yet? Or are you still working on it?

ghost commented 7 years ago

Branch: githubissue-132-upstream-next-TurningWrenches Commit(s): 0b2395c7b9747f3378ac0e386b196ab29e3d441a - SUBMITTED + ACCEPTED 8bfaadae2d1c0b40f6bc7b4f8f6a9b63af477809 - SUBMITTED + ACCEPTED e83af743db56964a6087082c8d0e7f118ce4d042 - SUBMITTED + ACCEPTED 119d254a4dbd5b7a881f09d76d42d0e878c626d7 - SUBMITTED + ACCEPTED 4f9e06fa755d0abfa35a6f7d44e46f5671e1f250 - SUBMITTED + ACCEPTED 2217fbb7d3f96269ac08c53180d5aedb86937352 - SUBMITTED + ACCEPTED dc272895de660791fead161f143d4203d9fbf3f7 - SUBMITTED + ACCEPTED 9e39ecb2d9745baa4c053ff5c435eb19ff9bdc58 - SUBMITTED + ACCEPTED d1a0ec501789f57ef9cb73d0bcacd33c110ba511 - SUBMITTED + ACCEPTED 4d460b609f8289bc4b8d3e85588e55eead2ac4e5 - OUTSIDE COMMIT c654155cf22962c0ec0cbd9a71775eafdedf8273 - SUBMITTED + ACCEPTED 3ed24a729342348628b6e74636f38cc4942beaf8 - SUBMITTED + ACCEPTED 6c1abadcbac761d7018578322aa76b7aac9298a1 - OUTSIDE COMMIT 65f4ad2d4d3aefc1fe1dfe756a9bf922fa23158b - SUBMITTED + ACCEPTED 79901a25abda57f240a8f096819342720fcb0839 - SUBMITTED + ACCEPTED T710-1 verified (smoke-test): Yes

ghost commented 7 years ago

@davidker --- NEW STUFF BELOW!

Branch: githubissue-132pt2-upstream-next-TurningWrenches Commit(s): 1a952c74e736292cb960b978e9ae429bcf5af64d fc9a92c47f86454dd75829e527aadda5937c4cf6 T710-1 verified (smoke-test): Yes

ghost commented 7 years ago

This task incurred a bit of scope-creep, but it was well worth the effort (code readability + consistency is up). With respect to the original feedback, I'm not sure what the first comment is referring to in the top code-snippet. For the second comment, that referenced code (comment) seems to have been deleted anyway. Thoughts @davidker?

davidker commented 7 years ago

I thought the first comment was referring to the blank line after req and before the if. I think we had too many blank lines just randomly spaced throughout our code. I've put both patches into upstream-testing.