linux-kernel - Re: [PATCH 2/2] staging: hv: Remove camel cases from vmbus channel functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20100920232921.GA31204@kroah.com>
Date:	Mon, 20 Sep 2010 16:29:21 -0700
From:	Greg KH <greg@...ah.com>
To:	Haiyang Zhang <haiyangz@...rosoft.com>
Cc:	"'linux-kernel@...r.kernel.org'" <linux-kernel@...r.kernel.org>,
	"'devel@...verdev.osuosl.org'" <devel@...verdev.osuosl.org>,
	"'virtualization@...ts.osdl.org'" <virtualization@...ts.osdl.org>,
	"'gregkh@...e.de'" <gregkh@...e.de>,
	Hank Janssen <hjanssen@...rosoft.com>
Subject: Re: [PATCH 2/2] staging: hv: Remove camel cases from vmbus channel
 functions

On Mon, Sep 20, 2010 at 09:13:39PM +0000, Haiyang Zhang wrote:
> From: Haiyang Zhang <haiyangz@...rosoft.com>
> 
> Remove camel cases from vmbus channel functions
> Converted the function names, local variables to lower cases.

Nice try, but you can do better :)

>  /* Internal routines */
> -static int VmbusChannelCreateGpadlHeader(
> -	void *Kbuffer,	/* must be phys and virt contiguous */
> -	u32 Size,	/* page-size multiple */
> -	struct vmbus_channel_msginfo **msgInfo,
> -	u32 *MessageCount);
> -static void DumpVmbusChannel(struct vmbus_channel *channel);
> -static void VmbusChannelSetEvent(struct vmbus_channel *channel);
> +static int vmbuschannel_creategpadlheader(
> +	void *kbuffer,	/* must be phys and virt contiguous */
> +	u32 size,	/* page-size multiple */
> +	struct vmbus_channel_msginfo **msginfo,
> +	u32 *messagecount);
> +static void dumpvmbuschannel(struct vmbus_channel *channel);
> +static void vmbuschannel_setevent(struct vmbus_channel *channel);

Internal functions are that, internal, why the verbosity for them?  Why
not try the following:
	create_gpad_header()
	dump_channel()
	set_event()

Looks almost readable, right?

> -#if 0
> -static void DumpMonitorPage(struct hv_monitor_page *MonitorPage)
> -{
> -	int i = 0;
> -	int j = 0;
> -
> -	DPRINT_DBG(VMBUS, "monitorPage - %p, trigger state - %d",
> -		   MonitorPage, MonitorPage->TriggerState);
> -
> -	for (i = 0; i < 4; i++)
> -		DPRINT_DBG(VMBUS, "trigger group (%d) - %llx", i,
> -			   MonitorPage->TriggerGroup[i].AsUINT64);
> -
> -	for (i = 0; i < 4; i++) {
> -		for (j = 0; j < 32; j++) {
> -			DPRINT_DBG(VMBUS, "latency (%d)(%d) - %llx", i, j,
> -				   MonitorPage->Latency[i][j]);
> -		}
> -	}
> -	for (i = 0; i < 4; i++) {
> -		for (j = 0; j < 32; j++) {
> -			DPRINT_DBG(VMBUS, "param-conn id (%d)(%d) - %d", i, j,
> -			       MonitorPage->Parameter[i][j].ConnectionId.Asu32);
> -			DPRINT_DBG(VMBUS, "param-flag (%d)(%d) - %d", i, j,
> -				MonitorPage->Parameter[i][j].FlagNumber);
> -		}
> -	}
> -}
> -#endif

Nice fix, but, it has nothing to do with the camelcase changes in this
file.

Please, again, one-patch-per-logical-change.  I'm getting tired of
repeating this...


>  /*
>   * VmbusChannelSetEvent - Trigger an event notification on the specified
>   * channel.
>   */
> -static void VmbusChannelSetEvent(struct vmbus_channel *Channel)
> +static void vmbuschannel_setevent(struct vmbus_channel *Channel)

You forgot to change the function comment name as well.

> -#if 0
> -static void VmbusChannelClearEvent(struct vmbus_channel *channel)
> -{
> -	struct hv_monitor_page *monitorPage;
> -
> -	if (Channel->OfferMsg.MonitorAllocated) {
> -		/* Each u32 represents 32 channels */
> -		clear_bit(Channel->OfferMsg.ChildRelId & 31,
> -			  (unsigned long *)gVmbusConnection.SendInterruptPage +
> -			  (Channel->OfferMsg.ChildRelId >> 5));
> -
> -		monitorPage =
> -			(struct hv_monitor_page *)gVmbusConnection.MonitorPages;
> -		monitorPage++; /* Get the child to parent monitor page */
> -
> -		clear_bit(Channel->MonitorBit,
> -			  (unsigned long *)&monitorPage->TriggerGroup
> -					[Channel->MonitorGroup].Pending);
> -	}
> -}
> -
> -#endif

Again a different thing than the case changing.

>  /*
> - * VmbusChannelGetDebugInfo -Retrieve various channel debug info
> + * vmbuschannel_getdebuginfo -Retrieve various channel debug info
>   */
> -void VmbusChannelGetDebugInfo(struct vmbus_channel *Channel,
> -			      struct vmbus_channel_debug_info *DebugInfo)
> +void vmbuschannel_getdebuginfo(struct vmbus_channel *channel,
> +			      struct vmbus_channel_debug_info *debuginfo)

Do you really need the vmbuschannel prefix here?

How about hyperv_get_debuginfo()?
Or vmbus_get_debuginfo()?

Although you might get vmbus confused with the vme bus, right?  Care to
find a consistant prefix for all hyperv bus functions and stick to it?

Oh, and what's wrong with "debug_info" instead of "debuginfo"?  You
alredy have it split up in the structure name, consistancy is good.


>  {
>  	struct hv_monitor_page *monitorPage;
> -	u8 monitorGroup = (u8)Channel->OfferMsg.MonitorId / 32;
> -	u8 monitorOffset = (u8)Channel->OfferMsg.MonitorId % 32;
> +	u8 monitorGroup = (u8)channel->OfferMsg.MonitorId / 32;
> +	u8 monitorOffset = (u8)channel->OfferMsg.MonitorId % 32;
>  	/* u32 monitorBit	= 1 << monitorOffset; */

Function name changes are one thing.

Variable names are another thing.

Again, one patch per thing please.

It's easier to read and verify that everything is sane.

Please try this one again.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/