Re: [PATCH 2/2] nbd: change a parameter's type to remove a memcpy call
On 7/20/07, Paul Clements <[EMAIL PROTECTED]> wrote: Denis Cheng wrote: > this memcpy looks so strange, in fact it's merely a pointer dereference, > so I change the parameter's type to refer it more directly, > this could make the memcpy not needed anymore. > > in the function nbd_read_stat where nbd_find_request is only once called, > the parameter served should be transformed accordingly. This is really a matter of preference. The generated code ends up being about the same, I think, while your patch makes the call to nbd_find_request kind of obtuse. Also, the memcpy's are balanced between send_req and find_request, so you can quickly see how the data is being transferred (from req into handle, and then back again). Your patch makes this less clear, at least to me. With one explicit memcpy stripped out, I think it's more clear to nbd_find_request. In nbd_read_stat, the cast to (struct request **) is not apparent, I must admit; but I think the best solution is declaring other few structs to make it clear, it's due to the lack of description of nbd client and server communication protocol. BTW, I think the nbd driver needs a clear documentation (its main site http://nbd.sourceforge.net/ does not give it): 1. When nbd_find_request is needed to call, the 8 byte memory of char handle[8] field in struct nbd_reply actually stores a pointer (struct request *), that pointer is received from the network. Since a pointer is only meaningful to the host, transfering it over the network will be unreliable, I don't think it's a good design, -- Denis Cheng Linux Application Developer "One of my most productive days was throwing away 1000 lines of code." - Ken Thompson. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] nbd: change a parameter's type to remove a memcpy call
Denis Cheng wrote:
this memcpy looks so strange, in fact it's merely a pointer dereference,
so I change the parameter's type to refer it more directly,
this could make the memcpy not needed anymore.
in the function nbd_read_stat where nbd_find_request is only once called,
the parameter served should be transformed accordingly.
This is really a matter of preference. The generated code ends up being
about the same, I think, while your patch makes the call to
nbd_find_request kind of obtuse. Also, the memcpy's are balanced between
send_req and find_request, so you can quickly see how the data is being
transferred (from req into handle, and then back again). Your patch
makes this less clear, at least to me.
--
Paul
Signed-off-by: Denis Cheng <[EMAIL PROTECTED]>
---
drivers/block/nbd.c |7 ++-
1 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 86639c0..a4d8508 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -235,14 +235,11 @@ error_out:
return 1;
}
-static struct request *nbd_find_request(struct nbd_device *lo, char *handle)
+static struct request *nbd_find_request(struct nbd_device *lo, struct request
*xreq)
{
struct request *req, *n;
- struct request *xreq;
int err;
- memcpy(, handle, sizeof(xreq));
-
err = wait_event_interruptible(lo->active_wq, lo->active_req != xreq);
if (unlikely(err))
goto out;
@@ -297,7 +294,7 @@ static struct request *nbd_read_stat(struct nbd_device *lo)
goto harderror;
}
- req = nbd_find_request(lo, reply.handle);
+ req = nbd_find_request(lo, *(struct request **)reply.handle);
if (unlikely(IS_ERR(req))) {
result = PTR_ERR(req);
if (result != -ENOENT)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] nbd: change a parameter's type to remove a memcpy call
this memcpy looks so strange, in fact it's merely a pointer dereference,
so I change the parameter's type to refer it more directly,
this could make the memcpy not needed anymore.
in the function nbd_read_stat where nbd_find_request is only once called,
the parameter served should be transformed accordingly.
Signed-off-by: Denis Cheng <[EMAIL PROTECTED]>
---
drivers/block/nbd.c |7 ++-
1 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 86639c0..a4d8508 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -235,14 +235,11 @@ error_out:
return 1;
}
-static struct request *nbd_find_request(struct nbd_device *lo, char *handle)
+static struct request *nbd_find_request(struct nbd_device *lo, struct request
*xreq)
{
struct request *req, *n;
- struct request *xreq;
int err;
- memcpy(, handle, sizeof(xreq));
-
err = wait_event_interruptible(lo->active_wq, lo->active_req != xreq);
if (unlikely(err))
goto out;
@@ -297,7 +294,7 @@ static struct request *nbd_read_stat(struct nbd_device *lo)
goto harderror;
}
- req = nbd_find_request(lo, reply.handle);
+ req = nbd_find_request(lo, *(struct request **)reply.handle);
if (unlikely(IS_ERR(req))) {
result = PTR_ERR(req);
if (result != -ENOENT)
--
1.5.2.2
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] nbd: change a parameter's type to remove a memcpy call
this memcpy looks so strange, in fact it's merely a pointer dereference,
so I change the parameter's type to refer it more directly,
this could make the memcpy not needed anymore.
in the function nbd_read_stat where nbd_find_request is only once called,
the parameter served should be transformed accordingly.
Signed-off-by: Denis Cheng [EMAIL PROTECTED]
---
drivers/block/nbd.c |7 ++-
1 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 86639c0..a4d8508 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -235,14 +235,11 @@ error_out:
return 1;
}
-static struct request *nbd_find_request(struct nbd_device *lo, char *handle)
+static struct request *nbd_find_request(struct nbd_device *lo, struct request
*xreq)
{
struct request *req, *n;
- struct request *xreq;
int err;
- memcpy(xreq, handle, sizeof(xreq));
-
err = wait_event_interruptible(lo-active_wq, lo-active_req != xreq);
if (unlikely(err))
goto out;
@@ -297,7 +294,7 @@ static struct request *nbd_read_stat(struct nbd_device *lo)
goto harderror;
}
- req = nbd_find_request(lo, reply.handle);
+ req = nbd_find_request(lo, *(struct request **)reply.handle);
if (unlikely(IS_ERR(req))) {
result = PTR_ERR(req);
if (result != -ENOENT)
--
1.5.2.2
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] nbd: change a parameter's type to remove a memcpy call
Denis Cheng wrote:
this memcpy looks so strange, in fact it's merely a pointer dereference,
so I change the parameter's type to refer it more directly,
this could make the memcpy not needed anymore.
in the function nbd_read_stat where nbd_find_request is only once called,
the parameter served should be transformed accordingly.
This is really a matter of preference. The generated code ends up being
about the same, I think, while your patch makes the call to
nbd_find_request kind of obtuse. Also, the memcpy's are balanced between
send_req and find_request, so you can quickly see how the data is being
transferred (from req into handle, and then back again). Your patch
makes this less clear, at least to me.
--
Paul
Signed-off-by: Denis Cheng [EMAIL PROTECTED]
---
drivers/block/nbd.c |7 ++-
1 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 86639c0..a4d8508 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -235,14 +235,11 @@ error_out:
return 1;
}
-static struct request *nbd_find_request(struct nbd_device *lo, char *handle)
+static struct request *nbd_find_request(struct nbd_device *lo, struct request
*xreq)
{
struct request *req, *n;
- struct request *xreq;
int err;
- memcpy(xreq, handle, sizeof(xreq));
-
err = wait_event_interruptible(lo-active_wq, lo-active_req != xreq);
if (unlikely(err))
goto out;
@@ -297,7 +294,7 @@ static struct request *nbd_read_stat(struct nbd_device *lo)
goto harderror;
}
- req = nbd_find_request(lo, reply.handle);
+ req = nbd_find_request(lo, *(struct request **)reply.handle);
if (unlikely(IS_ERR(req))) {
result = PTR_ERR(req);
if (result != -ENOENT)
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] nbd: change a parameter's type to remove a memcpy call
On 7/20/07, Paul Clements [EMAIL PROTECTED] wrote: Denis Cheng wrote: this memcpy looks so strange, in fact it's merely a pointer dereference, so I change the parameter's type to refer it more directly, this could make the memcpy not needed anymore. in the function nbd_read_stat where nbd_find_request is only once called, the parameter served should be transformed accordingly. This is really a matter of preference. The generated code ends up being about the same, I think, while your patch makes the call to nbd_find_request kind of obtuse. Also, the memcpy's are balanced between send_req and find_request, so you can quickly see how the data is being transferred (from req into handle, and then back again). Your patch makes this less clear, at least to me. With one explicit memcpy stripped out, I think it's more clear to nbd_find_request. In nbd_read_stat, the cast to (struct request **) is not apparent, I must admit; but I think the best solution is declaring other few structs to make it clear, it's due to the lack of description of nbd client and server communication protocol. BTW, I think the nbd driver needs a clear documentation (its main site http://nbd.sourceforge.net/ does not give it): 1. When nbd_find_request is needed to call, the 8 byte memory of char handle[8] field in struct nbd_reply actually stores a pointer (struct request *), that pointer is received from the network. Since a pointer is only meaningful to the host, transfering it over the network will be unreliable, I don't think it's a good design, -- Denis Cheng Linux Application Developer One of my most productive days was throwing away 1000 lines of code. - Ken Thompson. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
