Re: [PATCH 1/1] Drivers: hv: util: Fix a bug in version negotiationcode for util services

[Olaf Hering]

On Wed, Sep 04, Olaf Hering wrote:

> I suggest to let callers deal with error handling. Also as a cleanup,
> vmbus_prep_negotiate_resp does not make use of the negop passed to it.
> So that arg can be removed.

Simplify vmbus_prep_negotiate_resp. If we take the optimistic approach
that negotiation will not fail for any of the callers of
vmbus_prep_negotiate_resp this patch on top of yours should fix 2008 and
2012r2.

Olaf

---
 drivers/hv/channel_mgmt.c | 22 +++++++++-------------
 drivers/hv/hv_kvp.c       |  8 ++++----
 drivers/hv/hv_snapshot.c  |  3 +--
 drivers/hv/hv_util.c      |  7 +++----
 include/linux/hyperv.h    |  4 +---
 5 files changed, 18 insertions(+), 26 deletions(-)

diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 12ec8c8..dcaad3e 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -41,7 +41,6 @@ struct vmbus_channel_message_table_entry {
 /**
  * vmbus_prep_negotiate_resp() - Create default response for Hyper-V Negotiate message
  * @icmsghdrp: Pointer to msg header structure
- * @icmsg_negotiate: Pointer to negotiate message structure
  * @buf: Raw buffer channel data
  *
  * @icmsghdrp is of type &struct icmsg_hdr.
@@ -54,10 +53,10 @@ struct vmbus_channel_message_table_entry {
  *
  * Mainly used by Hyper-V drivers.
  */
-bool vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp,
-				struct icmsg_negotiate *negop, u8 *buf,
+bool vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp, u8 *buf,
 				int fw_version, int srv_version)
 {
+	struct icmsg_negotiate *negop;
 	int icframe_major, icframe_minor;
 	int icmsg_major, icmsg_minor;
 	int fw_major, fw_minor;
@@ -65,7 +64,6 @@ bool vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp,
 	int i;
 	bool found_match = false;
 
-	icmsghdrp->icmsgsize = 0x10;
 	fw_major = (fw_version >> 16);
 	fw_minor = (fw_version & 0xFFFF);
 
@@ -116,19 +114,17 @@ bool vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp,
 	 * version numbers we can support.
 	 */
 
-fw_error:
-	if (!found_match) {
-		negop->icframe_vercnt = 0;
-		negop->icmsg_vercnt = 0;
-	} else {
+	if (found_match) {
+		icmsghdrp->icmsgsize = 0x10;
 		negop->icframe_vercnt = 1;
 		negop->icmsg_vercnt = 1;
+		negop->icversion_data[0].major = icframe_major;
+		negop->icversion_data[0].minor = icframe_minor;
+		negop->icversion_data[1].major = icmsg_major;
+		negop->icversion_data[1].minor = icmsg_minor;
 	}
 
-	negop->icversion_data[0].major = icframe_major;
-	negop->icversion_data[0].minor = icframe_minor;
-	negop->icversion_data[1].major = icmsg_major;
-	negop->icversion_data[1].minor = icmsg_minor;
+fw_error:
 	return found_match;
 }
 
diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
index 5312720..31e338a 100644
--- a/drivers/hv/hv_kvp.c
+++ b/drivers/hv/hv_kvp.c
@@ -584,7 +584,6 @@ void hv_kvp_onchannelcallback(void *context)
 	struct hv_kvp_msg *kvp_msg;
 
 	struct icmsg_hdr *icmsghdrp;
-	struct icmsg_negotiate *negop = NULL;
 
 	if (kvp_transaction.active) {
 		/*
@@ -607,14 +606,15 @@ void hv_kvp_onchannelcallback(void *context)
 			 * We start with win8 version and if the host cannot
 			 * support that we use the previous version.
 			 */
-			if (vmbus_prep_negotiate_resp(icmsghdrp, negop,
+			if (vmbus_prep_negotiate_resp(icmsghdrp,
 				 recv_buffer, UTIL_FW_MAJOR_MINOR,
 				 WIN8_SRV_MAJOR_MINOR))
 				goto done;
 
-			vmbus_prep_negotiate_resp(icmsghdrp, negop,
+			if (vmbus_prep_negotiate_resp(icmsghdrp,
 				 recv_buffer, UTIL_FW_MAJOR_MINOR,
-				 WIN7_SRV_MAJOR_MINOR);
+				 WIN7_SRV_MAJOR_MINOR))
+				goto done;
 
 		} else {
 			kvp_msg = (struct hv_kvp_msg *)&recv_buffer[
diff --git a/drivers/hv/hv_snapshot.c b/drivers/hv/hv_snapshot.c
index e4572f3..51e8203 100644
--- a/drivers/hv/hv_snapshot.c
+++ b/drivers/hv/hv_snapshot.c
@@ -170,7 +170,6 @@ void hv_vss_onchannelcallback(void *context)
 
 
 	struct icmsg_hdr *icmsghdrp;
-	struct icmsg_negotiate *negop = NULL;
 
 	if (vss_transaction.active) {
 		/*
@@ -189,7 +188,7 @@ void hv_vss_onchannelcallback(void *context)
 			sizeof(struct vmbuspipe_hdr)];
 
 		if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
-			vmbus_prep_negotiate_resp(icmsghdrp, negop,
+			vmbus_prep_negotiate_resp(icmsghdrp,
 				 recv_buffer, UTIL_FW_MAJOR_MINOR,
 				 VSS_MAJOR_MINOR);
 		} else {
diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
index c16164d..01d7b17 100644
--- a/drivers/hv/hv_util.c
+++ b/drivers/hv/hv_util.c
@@ -88,7 +88,6 @@ static void shutdown_onchannelcallback(void *context)
 	struct shutdown_msg_data *shutdown_msg;
 
 	struct icmsg_hdr *icmsghdrp;
-	struct icmsg_negotiate *negop = NULL;
 
 	vmbus_recvpacket(channel, shut_txf_buf,
 			 PAGE_SIZE, &recvlen, &requestid);
@@ -98,7 +97,7 @@ static void shutdown_onchannelcallback(void *context)
 			sizeof(struct vmbuspipe_hdr)];
 
 		if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
-			vmbus_prep_negotiate_resp(icmsghdrp, negop,
+			vmbus_prep_negotiate_resp(icmsghdrp,
 					shut_txf_buf, UTIL_FW_MAJOR_MINOR,
 					SHUTDOWN_MAJOR_MINOR);
 		} else {
@@ -225,7 +224,7 @@ static void timesync_onchannelcallback(void *context)
 				sizeof(struct vmbuspipe_hdr)];
 
 		if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
-			vmbus_prep_negotiate_resp(icmsghdrp, NULL, time_txf_buf,
+			vmbus_prep_negotiate_resp(icmsghdrp, time_txf_buf,
 						UTIL_FW_MAJOR_MINOR,
 						TIMESYNCH_MAJOR_MINOR);
 		} else {
@@ -266,7 +265,7 @@ static void heartbeat_onchannelcallback(void *context)
 				sizeof(struct vmbuspipe_hdr)];
 
 		if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
-			vmbus_prep_negotiate_resp(icmsghdrp, NULL,
+			vmbus_prep_negotiate_resp(icmsghdrp,
 				hbeat_txf_buf, UTIL_FW_MAJOR_MINOR,
 				HEARTBEAT_MAJOR_MINOR);
 		} else {
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 4994907..084a858 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1502,9 +1502,7 @@ struct hyperv_service_callback {
 };
 
 #define MAX_SRV_VER	0x7ffffff
-extern bool vmbus_prep_negotiate_resp(struct icmsg_hdr *,
-					struct icmsg_negotiate *, u8 *, int,
-					int);
+extern bool vmbus_prep_negotiate_resp(struct icmsg_hdr *, u8 *, int, int);
 
 int hv_kvp_init(struct hv_util_service *);
 void hv_kvp_deinit(void);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/