RE: [PATCH v14 01/10] usbip: exporting devices: modifications to network header

2017-01-16 Thread fx IWATA NOBUO 
Hello,

> > This patch set implements operations to export and unexport device
> > using pre-defined but un-used export and un-export request and reply.
> 
> Get rid of the above from the changelog.

I will remove it.

> Please don't use file names, instead reference the structures.

I see.
I will not use file name to reference global structures.

> > They become empty struct. Other empty struct, 'op_devlist_request',
> > defined.
> 
> Does this go with this patch. Doesn't look like the above is accurate.
> Don't see op_devlist_request in this patch.

OK.

> As an example this change log would be lot clear as below:
> 
> "Add new status codes to network common header struct op_common.
>  Add corresponding user friendly string for each of the status
>  codes. Change dbg messages to use new status strings"
> 
> There is no need to describe code in the change log.

The revised change log is as below.
---
This patch adds new status codes which are needed for new operations of 
this series to network common header 'struct op_common'. Corresponding 
user friendly string for each of the status are added and a debug 
message is changed to use the new status strings.  
The 'returncode' of export and un-export reply are removed.
---

Best Regards,

Nobuo Iwata
//
> -Original Message-
> From: Shuah Khan [mailto:shua...@osg.samsung.com]
> Sent: Friday, January 13, 2017 2:44 AM
> To: fx IWATA NOBUO <nobuo.iw...@fujixerox.co.jp>;
> valentina.mane...@gmail.com; sh...@kernel.org;
> gre...@linuxfoundation.org; linux-usb@vger.kernel.org
> Cc: fx MICHIMURA TADAO <michimura.ta...@fujixerox.co.jp>; Shuah
> Khan <shua...@osg.samsung.com>; Shuah Khan <sh...@kernel.org>
> Subject: Re: [PATCH v14 01/10] usbip: exporting devices: modifications to
> network header
> 
> Hi Nobuo,
> 
> On 12/26/2016 12:08 AM, Nobuo Iwata wrote:
> > This patch set implements operations to export and unexport device
> > using pre-defined but un-used export and un-export request and reply.
> 
> Get rid of the above from the changelog.
> 
> >
> > This patch modifies export and un-export reply in
> > tools/usb/usbip/src/usbip_network.h. The 'returncode' of each
> response
> > is removed. The status is set in op_common.status.
> 
> Please don't use file names, instead reference the structures.
> 
> As an example this change log would be lot clear as below:
> 
> "Add new status codes to network common header struct op_common.
>  Add corresponding user friendly string for each of the status
>  codes. Change dbg messages to use new status strings"
> 
> There is no need to describe code in the change log.
> 
> >
> > Following status codes are added for this patch set.
> >   ST_NO_FREE_POR
> >   ST_NOT_FOUND
> >
> > The reason to use op_common.status:
> >   1) usbip_net_recv_op_common() hanles as error when the status is
> other
> >  than ST_OK.
> >   2) The status has a commnet "add more error code".
> >
> > They become empty struct. Other empty struct, 'op_devlist_request',
> > defined.
> 
> Does this go with this patch. Doesn't look like the above is accurate.
> Don't see op_devlist_request in this patch.
> 
> >
> > This patch also includes string translation of the status codes.
> >
> > Signed-off-by: Nobuo Iwata <nobuo.iw...@fujixerox.co.jp>
> > ---
> >  tools/usb/usbip/src/usbip_network.c | 26
> +-
> >  tools/usb/usbip/src/usbip_network.h |  8 
> >  2 files changed, 29 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/usb/usbip/src/usbip_network.c
> b/tools/usb/usbip/src/usbip_network.c
> > index b4c37e7..069ec54 100644
> > --- a/tools/usb/usbip/src/usbip_network.c
> > +++ b/tools/usb/usbip/src/usbip_network.c
> > @@ -163,6 +163,29 @@ int usbip_net_send_op_common(int sockfd,
> uint32_t code, uint32_t status)
> > return 0;
> >  }
> >
> > +struct op_common_error {
> > +   uint32_tstatus;
> > +   const char  *str;
> > +};
> > +
> > +struct op_common_error op_common_errors[] = {
> > +   {ST_NA, "not available"},
> > +   {ST_NO_FREE_PORT, "no free port"},
> > +   {ST_DEVICE_NOT_FOUND, "device not found"},
> > +   {0, NULL}
> > +};
> > +
> > +static const char *op_common_strerror(uint32_t status)
> > +{
> > +   struct op_common_error *err;
> > +
> > +   for (err = op_common_errors; err->str != NULL; err++) {
> > +   if (err->status == status)
> > +   return err->str;
> > +

Re: [PATCH v14 01/10] usbip: exporting devices: modifications to network header

2017-01-12 Thread Shuah Khan 
Hi Nobuo,

On 12/26/2016 12:08 AM, Nobuo Iwata wrote:
> This patch set implements operations to export and unexport device 
> using pre-defined but un-used export and un-export request and reply.

Get rid of the above from the changelog.

> 
> This patch modifies export and un-export reply in 
> tools/usb/usbip/src/usbip_network.h. The 'returncode' of each response 
> is removed. The status is set in op_common.status.

Please don't use file names, instead reference the structures.

As an example this change log would be lot clear as below:

"Add new status codes to network common header struct op_common.
 Add corresponding user friendly string for each of the status
 codes. Change dbg messages to use new status strings"

There is no need to describe code in the change log.

> 
> Following status codes are added for this patch set.
>   ST_NO_FREE_POR
>   ST_NOT_FOUND
> 
> The reason to use op_common.status:
>   1) usbip_net_recv_op_common() hanles as error when the status is other
>  than ST_OK.
>   2) The status has a commnet "add more error code".
> 
> They become empty struct. Other empty struct, 'op_devlist_request', 
> defined.

Does this go with this patch. Doesn't look like the above is accurate.
Don't see op_devlist_request in this patch.

> 
> This patch also includes string translation of the status codes.
> 
> Signed-off-by: Nobuo Iwata 
> ---
>  tools/usb/usbip/src/usbip_network.c | 26 +-
>  tools/usb/usbip/src/usbip_network.h |  8 
>  2 files changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/usb/usbip/src/usbip_network.c 
> b/tools/usb/usbip/src/usbip_network.c
> index b4c37e7..069ec54 100644
> --- a/tools/usb/usbip/src/usbip_network.c
> +++ b/tools/usb/usbip/src/usbip_network.c
> @@ -163,6 +163,29 @@ int usbip_net_send_op_common(int sockfd, uint32_t code, 
> uint32_t status)
>   return 0;
>  }
>  
> +struct op_common_error {
> + uint32_tstatus;
> + const char  *str;
> +};
> +
> +struct op_common_error op_common_errors[] = {
> + {ST_NA, "not available"},
> + {ST_NO_FREE_PORT, "no free port"},
> + {ST_DEVICE_NOT_FOUND, "device not found"},
> + {0, NULL}
> +};
> +
> +static const char *op_common_strerror(uint32_t status)
> +{
> + struct op_common_error *err;
> +
> + for (err = op_common_errors; err->str != NULL; err++) {
> + if (err->status == status)
> + return err->str;
> + }
> + return "unknown error";
> +}

This loop is unnecessary. Do a range check on the passed in status
value and index and return the string.

Move the op_common_errors[] and op_common_strerror() usbip_network.h
This can easily be a macro.

Change other places ST_OK handling is done. I see some in usbipd.c

> +
>  int usbip_net_recv_op_common(int sockfd, uint16_t *code)
>  {
>   struct op_common op_common;
> @@ -196,7 +219,8 @@ int usbip_net_recv_op_common(int sockfd, uint16_t *code)
>   }
>  
>   if (op_common.status != ST_OK) {
> - dbg("request failed at peer: %d", op_common.status);
> + dbg("request failed at peer: %s",
> + op_common_strerror(op_common.status));
>   goto err;
>   }
>  
> diff --git a/tools/usb/usbip/src/usbip_network.h 
> b/tools/usb/usbip/src/usbip_network.h
> index c1e875c..86c3ff0 100644
> --- a/tools/usb/usbip/src/usbip_network.h
> +++ b/tools/usb/usbip/src/usbip_network.h
> @@ -27,8 +27,10 @@ struct op_common {
>   uint16_t code;
>  
>   /* add more error code */

codes instead of code

> -#define ST_OK0x00
> -#define ST_NA0x01
> +#define ST_OK0x00
> +#define ST_NA0x01
> +#define ST_NO_FREE_PORT  0x02
> +#define ST_DEVICE_NOT_FOUND  0x03

Maintain the indentation for the last entry.

>   uint32_t status; /* op_code status (for reply) */
>  
>  } __attribute__((packed));
> @@ -93,7 +95,6 @@ struct op_export_request {
>  } __attribute__((packed));
>  
>  struct op_export_reply {
> - int returncode;
>  } __attribute__((packed));
>  

Remove the empty structure.

>  
> @@ -115,7 +116,6 @@ struct op_unexport_request {
>  } __attribute__((packed));
>  
>  struct op_unexport_reply {
> - int returncode;
>  } __attribute__((packed));

Remove the empty structure.

>  
>  #define PACK_OP_UNEXPORT_REQUEST(pack, request)  do {\
> 

thanks,
-- Shuah
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v14 01/10] usbip: exporting devices: modifications to network header

2017-01-12 Thread Krzysztof Opasiak 


On 12/26/2016 08:08 AM, Nobuo Iwata wrote:
> This patch set implements operations to export and unexport device 
> using pre-defined but un-used export and un-export request and reply.
> 
> This patch modifies export and un-export reply in 
> tools/usb/usbip/src/usbip_network.h. The 'returncode' of each response 
> is removed. The status is set in op_common.status.
> 
> Following status codes are added for this patch set.
>   ST_NO_FREE_POR
>   ST_NOT_FOUND
> 
> The reason to use op_common.status:
>   1) usbip_net_recv_op_common() hanles as error when the status is other
>  than ST_OK.
>   2) The status has a commnet "add more error code".
> 
> They become empty struct. Other empty struct, 'op_devlist_request', 
> defined. 
> 
> This patch also includes string translation of the status codes.
> 
> Signed-off-by: Nobuo Iwata 

Reviewied-by: Krzysztof Opasiak 

> ---
>  tools/usb/usbip/src/usbip_network.c | 26 +-
>  tools/usb/usbip/src/usbip_network.h |  8 
>  2 files changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/usb/usbip/src/usbip_network.c 
> b/tools/usb/usbip/src/usbip_network.c
> index b4c37e7..069ec54 100644
> --- a/tools/usb/usbip/src/usbip_network.c
> +++ b/tools/usb/usbip/src/usbip_network.c
> @@ -163,6 +163,29 @@ int usbip_net_send_op_common(int sockfd, uint32_t code, 
> uint32_t status)
>   return 0;
>  }
>  
> +struct op_common_error {
> + uint32_tstatus;
> + const char  *str;
> +};
> +
> +struct op_common_error op_common_errors[] = {
> + {ST_NA, "not available"},
> + {ST_NO_FREE_PORT, "no free port"},
> + {ST_DEVICE_NOT_FOUND, "device not found"},
> + {0, NULL}
> +};
> +
> +static const char *op_common_strerror(uint32_t status)
> +{
> + struct op_common_error *err;
> +
> + for (err = op_common_errors; err->str != NULL; err++) {
> + if (err->status == status)
> + return err->str;
> + }
> + return "unknown error";
> +}
> +
>  int usbip_net_recv_op_common(int sockfd, uint16_t *code)
>  {
>   struct op_common op_common;
> @@ -196,7 +219,8 @@ int usbip_net_recv_op_common(int sockfd, uint16_t *code)
>   }
>  
>   if (op_common.status != ST_OK) {
> - dbg("request failed at peer: %d", op_common.status);
> + dbg("request failed at peer: %s",
> + op_common_strerror(op_common.status));
>   goto err;
>   }
>  
> diff --git a/tools/usb/usbip/src/usbip_network.h 
> b/tools/usb/usbip/src/usbip_network.h
> index c1e875c..86c3ff0 100644
> --- a/tools/usb/usbip/src/usbip_network.h
> +++ b/tools/usb/usbip/src/usbip_network.h
> @@ -27,8 +27,10 @@ struct op_common {
>   uint16_t code;
>  
>   /* add more error code */
> -#define ST_OK0x00
> -#define ST_NA0x01
> +#define ST_OK0x00
> +#define ST_NA0x01
> +#define ST_NO_FREE_PORT  0x02
> +#define ST_DEVICE_NOT_FOUND  0x03
>   uint32_t status; /* op_code status (for reply) */
>  
>  } __attribute__((packed));
> @@ -93,7 +95,6 @@ struct op_export_request {
>  } __attribute__((packed));
>  
>  struct op_export_reply {
> - int returncode;
>  } __attribute__((packed));
>  
>  
> @@ -115,7 +116,6 @@ struct op_unexport_request {
>  } __attribute__((packed));
>  
>  struct op_unexport_reply {
> - int returncode;
>  } __attribute__((packed));
>  
>  #define PACK_OP_UNEXPORT_REQUEST(pack, request)  do {\
> 

-- 
Krzysztof Opasiak
Samsung R Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v14 01/10] usbip: exporting devices: modifications to network header

2016-12-25 Thread Nobuo Iwata 
This patch set implements operations to export and unexport device 
using pre-defined but un-used export and un-export request and reply.

This patch modifies export and un-export reply in 
tools/usb/usbip/src/usbip_network.h. The 'returncode' of each response 
is removed. The status is set in op_common.status.

Following status codes are added for this patch set.
  ST_NO_FREE_POR
  ST_NOT_FOUND

The reason to use op_common.status:
  1) usbip_net_recv_op_common() hanles as error when the status is other
 than ST_OK.
  2) The status has a commnet "add more error code".

They become empty struct. Other empty struct, 'op_devlist_request', 
defined. 

This patch also includes string translation of the status codes.

Signed-off-by: Nobuo Iwata 
---
 tools/usb/usbip/src/usbip_network.c | 26 +-
 tools/usb/usbip/src/usbip_network.h |  8 
 2 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/tools/usb/usbip/src/usbip_network.c 
b/tools/usb/usbip/src/usbip_network.c
index b4c37e7..069ec54 100644
--- a/tools/usb/usbip/src/usbip_network.c
+++ b/tools/usb/usbip/src/usbip_network.c
@@ -163,6 +163,29 @@ int usbip_net_send_op_common(int sockfd, uint32_t code, 
uint32_t status)
return 0;
 }
 
+struct op_common_error {
+   uint32_tstatus;
+   const char  *str;
+};
+
+struct op_common_error op_common_errors[] = {
+   {ST_NA, "not available"},
+   {ST_NO_FREE_PORT, "no free port"},
+   {ST_DEVICE_NOT_FOUND, "device not found"},
+   {0, NULL}
+};
+
+static const char *op_common_strerror(uint32_t status)
+{
+   struct op_common_error *err;
+
+   for (err = op_common_errors; err->str != NULL; err++) {
+   if (err->status == status)
+   return err->str;
+   }
+   return "unknown error";
+}
+
 int usbip_net_recv_op_common(int sockfd, uint16_t *code)
 {
struct op_common op_common;
@@ -196,7 +219,8 @@ int usbip_net_recv_op_common(int sockfd, uint16_t *code)
}
 
if (op_common.status != ST_OK) {
-   dbg("request failed at peer: %d", op_common.status);
+   dbg("request failed at peer: %s",
+   op_common_strerror(op_common.status));
goto err;
}
 
diff --git a/tools/usb/usbip/src/usbip_network.h 
b/tools/usb/usbip/src/usbip_network.h
index c1e875c..86c3ff0 100644
--- a/tools/usb/usbip/src/usbip_network.h
+++ b/tools/usb/usbip/src/usbip_network.h
@@ -27,8 +27,10 @@ struct op_common {
uint16_t code;
 
/* add more error code */
-#define ST_OK  0x00
-#define ST_NA  0x01
+#define ST_OK  0x00
+#define ST_NA  0x01
+#define ST_NO_FREE_PORT0x02
+#define ST_DEVICE_NOT_FOUND0x03
uint32_t status; /* op_code status (for reply) */
 
 } __attribute__((packed));
@@ -93,7 +95,6 @@ struct op_export_request {
 } __attribute__((packed));
 
 struct op_export_reply {
-   int returncode;
 } __attribute__((packed));
 
 
@@ -115,7 +116,6 @@ struct op_unexport_request {
 } __attribute__((packed));
 
 struct op_unexport_reply {
-   int returncode;
 } __attribute__((packed));
 
 #define PACK_OP_UNEXPORT_REQUEST(pack, request)  do {\
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html