Re: [PATCH 088/117] Staging: hv: netvsc: Inline the code for free_net_device()
On Fri, Jul 15, 2011 at 10:47:16AM -0700, K. Y. Srinivasan wrote:
-static void free_net_device(struct netvsc_device *device)
-{
- WARN_ON(atomic_read(device-refcnt) != 0);
- device-dev-ext = NULL;
device-dev-ext points to device. We set it NULL here to prevent
a use after free bug. What prevents that in the new code?
- kfree(device);
-}
regards,
dan carpenter
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 020/117] Staging: hv: vmbus: Support the notion of id tables in vmbus_match()
-Original Message-
From: Christoph Hellwig [mailto:h...@infradead.org]
Sent: Friday, July 15, 2011 10:03 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang
Subject: Re: [PATCH 020/117] Staging: hv: vmbus: Support the notion of id
tables
in vmbus_match()
On Fri, Jul 15, 2011 at 10:46:08AM -0700, K. Y. Srinivasan wrote:
+static bool is_not_null_guid(const __u8 *guid)
+{
+ int i;
+
+ for (i = 0; i (sizeof(struct hv_vmbus_device_id)); i++)
+ if (guid[i] != 0)
+ return true;
+ return false;
+}
would be nice to add uuid_{le,be}_is_nil helpers to uuid.h. I also
think simply using a memcpy might be more efficient than the hand-rolled
loop.
Having a helper function would be great. With regards to the efficiency of this
loop,
this is not a performance critical path. If people feel strongly about this, I
could
change this; perhaps as a separate patch on top of this series; I don't want to
send a large
patch-set like this again.
Regards,
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 097/117] Staging: hv: storvsc: Add code to handle IDE devices using the storvsc driver
-Original Message-
From: Christoph Hellwig [mailto:h...@infradead.org]
Sent: Friday, July 15, 2011 10:05 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang
Subject: Re: [PATCH 097/117] Staging: hv: storvsc: Add code to handle IDE
devices
using the storvsc driver
Thanks, this looks much cleaner than the initial variant.
+ if (dev_is_ide) {
+ storvsc_get_ide_info(device, target, path);
+ host_dev-path = device_info.path_id;
+ host_dev-target = device_info.target_id;
+ } else {
+ host_dev-path = device_info.path_id;
+ host_dev-target = device_info.target_id;
+ }
Is using the device_info values in both branches intentional? If so
there's no need to have these assignments duplicated.
While we set the values in both the branches, the value set is different;
The IDE side encodes the bits differently and is appropriately parsed in the
function storvsc_get_ide_info().
Regards,
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 088/117] Staging: hv: netvsc: Inline the code for free_net_device()
-Original Message-
From: Dan Carpenter [mailto:erro...@gmail.com]
Sent: Saturday, July 16, 2011 6:34 AM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang
Subject: Re: [PATCH 088/117] Staging: hv: netvsc: Inline the code for
free_net_device()
On Fri, Jul 15, 2011 at 10:47:16AM -0700, K. Y. Srinivasan wrote:
-static void free_net_device(struct netvsc_device *device)
-{
- WARN_ON(atomic_read(device-refcnt) != 0);
- device-dev-ext = NULL;
device-dev-ext points to device. We set it NULL here to prevent
a use after free bug. What prevents that in the new code?
- kfree(device);
-}
This function is called in only two places; a) during adding a device when we
have a failure
(netvsc_device_add()) and during device removal (netvsc_device_remove()). I
have handled
both these cases. In any event when we are freeing the containing object via
kfree(), if somebody
is accessing the ext field via freed object, we have bigger problems to deal
with.
Regards,
K. Y
regards,
dan carpenter
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 097/117] Staging: hv: storvsc: Add code to handle IDE devices using the storvsc driver
-Original Message-
From: Sasha Levin [mailto:levinsasha...@gmail.com]
Sent: Saturday, July 16, 2011 9:02 AM
To: KY Srinivasan
Cc: Christoph Hellwig; gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang
Subject: RE: [PATCH 097/117] Staging: hv: storvsc: Add code to handle IDE
devices
using the storvsc driver
On Sat, 2011-07-16 at 12:57 +, KY Srinivasan wrote:
-Original Message-
From: Christoph Hellwig [mailto:h...@infradead.org]
Sent: Friday, July 15, 2011 10:05 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang
Subject: Re: [PATCH 097/117] Staging: hv: storvsc: Add code to handle IDE
devices
using the storvsc driver
Thanks, this looks much cleaner than the initial variant.
+ if (dev_is_ide) {
+ storvsc_get_ide_info(device, target, path);
+ host_dev-path = device_info.path_id;
+ host_dev-target = device_info.target_id;
+ } else {
+ host_dev-path = device_info.path_id;
+ host_dev-target = device_info.target_id;
+ }
Is using the device_info values in both branches intentional? If so
there's no need to have these assignments duplicated.
While we set the values in both the branches, the value set is different;
The IDE side encodes the bits differently and is appropriately parsed in the
function storvsc_get_ide_info().
Is think that what Christoph meant was simplifying it to:
if (dev_is_ide)
storvsc_get_ide_info(device, target, path);
host_dev-path = device_info.path_id;
host_dev-target = device_info.target_id;
Good point! It is still too early in the morning for me.
Greg, if it is ok with you, in the interest of keeping the patch
cops happy, I can submit a patch for this on top of this series.
Regards,
K. Y
--
Sasha.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 020/117] Staging: hv: vmbus: Support the notion of id tables in vmbus_match()
On Sat, Jul 16, 2011 at 12:54:46PM +, KY Srinivasan wrote:
would be nice to add uuid_{le,be}_is_nil helpers to uuid.h. I also
think simply using a memcpy might be more efficient than the hand-rolled
loop.
Having a helper function would be great. With regards to the efficiency of
this loop,
this is not a performance critical path. If people feel strongly about this,
I could
change this; perhaps as a separate patch on top of this series; I don't want
to send a large
patch-set like this again.
I'm fine with doing it later.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 097/117] Staging: hv: storvsc: Add code to handle IDE devices using the storvsc driver
On Sat, Jul 16, 2011 at 04:01:39PM +0300, Sasha Levin wrote: Is think that what Christoph meant was simplifying it to: if (dev_is_ide) storvsc_get_ide_info(device, target, path); host_dev-path = device_info.path_id; host_dev-target = device_info.target_id; Indeed. In addition path is unused and target is only used in the second dev_is_ide conditional, so both the call and the target and path variables can be moved into that one to further simplify the function. But I'm fine with delaying that cleanup to a follow on patch so that we can make progress in getting this pile merged. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 097/117] Staging: hv: storvsc: Add code to handle IDE devices using the storvsc driver
On Sat, 2011-07-16 at 12:57 +, KY Srinivasan wrote:
-Original Message-
From: Christoph Hellwig [mailto:h...@infradead.org]
Sent: Friday, July 15, 2011 10:05 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang
Subject: Re: [PATCH 097/117] Staging: hv: storvsc: Add code to handle IDE
devices
using the storvsc driver
Thanks, this looks much cleaner than the initial variant.
+ if (dev_is_ide) {
+ storvsc_get_ide_info(device, target, path);
+ host_dev-path = device_info.path_id;
+ host_dev-target = device_info.target_id;
+ } else {
+ host_dev-path = device_info.path_id;
+ host_dev-target = device_info.target_id;
+ }
Is using the device_info values in both branches intentional? If so
there's no need to have these assignments duplicated.
While we set the values in both the branches, the value set is different;
The IDE side encodes the bits differently and is appropriately parsed in the
function storvsc_get_ide_info().
Is think that what Christoph meant was simplifying it to:
if (dev_is_ide)
storvsc_get_ide_info(device, target, path);
host_dev-path = device_info.path_id;
host_dev-target = device_info.target_id;
--
Sasha.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
