RE: [PATCH 2/3]: An implementation of HyperV KVP functionality

2010-12-08 Thread Hank Janssen 

 From: Ky Srinivasan [mailto:ksriniva...@novell.com]
 Sent: Tuesday, December 07, 2010 3:19 PM
  On 12/7/2010 at  5:29 PM, in message
 20101207222933.ga10...@ioremap.net,
 Evgeniy Polyakov z...@ioremap.net wrote:
  On Tue, Dec 07, 2010 at 03:25:56PM -0700, Ky Srinivasan
  (ksriniva...@novell.com) wrote:
  +static void shutdown_onchannelcallback(void *context)
  +{
  +  struct vmbus_channel *channel = context;
  +  u8 *buf;
  +  u32 buflen, recvlen;
  +  u64 requestid;
  +  u8  execute_shutdown = false;
  +
  +  struct shutdown_msg_data *shutdown_msg;
  +
  +  struct icmsg_hdr *icmsghdrp;
  +  struct icmsg_negotiate *negop = NULL;
  +
  +  buflen = PAGE_SIZE;
  +  buf = kmalloc(buflen, GFP_ATOMIC);
  +
  +  vmbus_recvpacket(channel, buf, buflen, recvlen, requestid);
 
  This did not change since previous review and this is wrong.
  It is the right way to crash kernel.
 
  I did not read further since this is a show-stopper imo.
 Hank, do you want to respond to this comment.
 

I will submit a patch for hv_utils.c to check the return value from
kmalloc and vmbus_recvpacket and return if either one of them fail.

The function is a void because they are treated here as fire and
Forget. But it comment about what would happen if kmalloc or 
Vmbus_recvpacket fails is correct. We could cause a crash.

So I will correct that part.

Hank.


___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/3]: An implementation of HyperV KVP functionality

2010-12-07 Thread Evgeniy Polyakov 
On Tue, Dec 07, 2010 at 03:25:56PM -0700, Ky Srinivasan 
(ksriniva...@novell.com) wrote:
 +static void shutdown_onchannelcallback(void *context)
 +{
 + struct vmbus_channel *channel = context;
 + u8 *buf;
 + u32 buflen, recvlen;
 + u64 requestid;
 + u8  execute_shutdown = false;
 +
 + struct shutdown_msg_data *shutdown_msg;
 +
 + struct icmsg_hdr *icmsghdrp;
 + struct icmsg_negotiate *negop = NULL;
 +
 + buflen = PAGE_SIZE;
 + buf = kmalloc(buflen, GFP_ATOMIC);
 +
 + vmbus_recvpacket(channel, buf, buflen, recvlen, requestid);

This did not change since previous review and this is wrong.
It is the right way to crash kernel.

I did not read further since this is a show-stopper imo.

-- 
Evgeniy Polyakov
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/3]: An implementation of HyperV KVP functionality

2010-12-07 Thread Ky Srinivasan 


 On 12/7/2010 at  5:29 PM, in message 20101207222933.ga10...@ioremap.net,
Evgeniy Polyakov z...@ioremap.net wrote: 
 On Tue, Dec 07, 2010 at 03:25:56PM -0700, Ky Srinivasan 
 (ksriniva...@novell.com) wrote:
 +static void shutdown_onchannelcallback(void *context)
 +{
 +struct vmbus_channel *channel = context;
 +u8 *buf;
 +u32 buflen, recvlen;
 +u64 requestid;
 +u8  execute_shutdown = false;
 +
 +struct shutdown_msg_data *shutdown_msg;
 +
 +struct icmsg_hdr *icmsghdrp;
 +struct icmsg_negotiate *negop = NULL;
 +
 +buflen = PAGE_SIZE;
 +buf = kmalloc(buflen, GFP_ATOMIC);
 +
 +vmbus_recvpacket(channel, buf, buflen, recvlen, requestid);
 
 This did not change since previous review and this is wrong.
 It is the right way to crash kernel.
 
 I did not read further since this is a show-stopper imo.
Hank, do you want to respond to this comment.

Regards,

K. Y

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/3]: An implementation of HyperV KVP functionality

2010-12-07 Thread Greg KH 
On Tue, Dec 07, 2010 at 03:25:56PM -0700, Ky Srinivasan wrote:
 This patch is re-based on the latest linux-next tree.
 
 From: K. Y. Srinivasan ksriniva...@novell.com
 
 Subject: The hv_utils module will be composed of more than one file;
 rename hv_utils.c to accommodate this without changing the module name.
 
 Signed-off-by: K. Y. Srinivasan ksriniva...@novell.com
 


Please put the diffstat of the patch in below the signed-off-by: and
after a --- line.


 Index: linux.trees.git/drivers/staging/hv/Makefile
 ===
 --- linux.trees.git.orig/drivers/staging/hv/Makefile  2010-12-07 
 07:04:41.0 -0500
 +++ linux.trees.git/drivers/staging/hv/Makefile   2010-12-07 
 08:00:10.0 -0500
 @@ -10,3 +10,4 @@ hv_vmbus-y := vmbus_drv.o osd.o \
  hv_storvsc-y := storvsc_drv.o storvsc.o
  hv_blkvsc-y := blkvsc_drv.o blkvsc.o
  hv_netvsc-y := netvsc_drv.o netvsc.o rndis_filter.o
 +hv_utils-y := hv_util.o
 Index: linux.trees.git/drivers/staging/hv/hv_util.c
 ===
 --- /dev/null 1970-01-01 00:00:00.0 +
 +++ linux.trees.git/drivers/staging/hv/hv_util.c  2010-12-07 
 08:00:10.0 -0500

Are you renaming the file here?  Are you using git?  If so, it should
produce a patch that shows the rename happened, not this big 'remove it
all and then add it back' type thing.

thanks,

greg k-h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/3]: An Implementation of HyperV KVP functionality

2010-11-29 Thread Ky Srinivasan 


 On 11/24/2010 at  9:56 AM, in message 20101124145617.ga11...@ioremap.net,
Evgeniy Polyakov z...@ioremap.net wrote: 
 Hi.
 
 I will ack connector part of course, but this hunk is actually quite
Thank you.
 bad.
 
 
 +static void shutdown_onchannelcallback(void *context)
 +{
 +struct vmbus_channel *channel = context;
 +u8 *buf;
 +u32 buflen, recvlen;
 +u64 requestid;
 +u8  execute_shutdown = false;
 +
 +struct shutdown_msg_data *shutdown_msg;
 +
 +struct icmsg_hdr *icmsghdrp;
 +struct icmsg_negotiate *negop = NULL;
 +
 +buflen = PAGE_SIZE;
 +buf = kmalloc(buflen, GFP_ATOMIC);
 +
 +vmbus_recvpacket(channel, buf, buflen, recvlen, requestid);
 
 Boom. I did not read further, since this function returns void and thus
 can not propagate error, which is likely not a good idea.

Hank and Haiyang (both copied here) are the authors of this code. My 
contribution to this code (in this patch) has been to change the name of the 
file! I will let Hank and Haiyang comment on your feedback.

Regards,

K. Y 


___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/3]: An Implementation of HyperV KVP functionality

2010-11-24 Thread Evgeniy Polyakov 
Hi.

I will ack connector part of course, but this hunk is actually quite
bad.


 +static void shutdown_onchannelcallback(void *context)
 +{
 + struct vmbus_channel *channel = context;
 + u8 *buf;
 + u32 buflen, recvlen;
 + u64 requestid;
 + u8  execute_shutdown = false;
 +
 + struct shutdown_msg_data *shutdown_msg;
 +
 + struct icmsg_hdr *icmsghdrp;
 + struct icmsg_negotiate *negop = NULL;
 +
 + buflen = PAGE_SIZE;
 + buf = kmalloc(buflen, GFP_ATOMIC);
 +
 + vmbus_recvpacket(channel, buf, buflen, recvlen, requestid);

Boom. I did not read further, since this function returns void and thus
can not propagate error, which is likely not a good idea.

-- 
Evgeniy Polyakov
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization