RE: [PATCH v13 01/10] usbip: exporting devices: modifications to network header
Hello, > + if (!error) > + reply.returncode = 0; > + else > + reply.returncode = -1; > > It looks a little bit ugly to me that we change the value to be unsigned > and then assign a signed number to it. In addition your compiler is going > to complain if you use -Wconversion flag. My mistake. > > struct op_unexport_reply { > > - int returncode; > > + uint32_t returncode; > > } __attribute__((packed)); *** I will remove 'returncode'. It's enough by using op_common.status(ST_OK|ST_NA). There 2 error case for each operation. Import : not-found, export-error Export: busy(no free), attach-error Unexport : not-found, export-error Existing, import, doesn't carry the reason. It only uses op_common.status. Currently, usbip's exit-status doesn't have the reason. There's a president with no field: op_devlist_request. Thank you for reviewing, n.iwata // > -Original Message- > From: Krzysztof Opasiak [mailto:k.opas...@samsung.com] > Sent: Friday, December 02, 2016 4:11 AM > To: fx IWATA NOBUO; valentina.mane...@gmail.com; shuah...@samsung.com; > gre...@linuxfoundation.org; linux-usb@vger.kernel.org > Cc: fx MICHIMURA TADAO > Subject: Re: [PATCH v13 01/10] usbip: exporting devices: modifications to > network header > > Hi, > > On 11/22/2016 07:48 AM, Nobuo Iwata wrote: > > Modification to export and un-export response in > > tools/usb/usbip/src/usbip_network.h. It just changes return code type > > from int to uint32_t as same as other responses. > > > > Signed-off-by: Nobuo Iwata <nobuo.iw...@fujixerox.co.jp> > > --- > > tools/usb/usbip/src/usbip_network.h | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/tools/usb/usbip/src/usbip_network.h > > b/tools/usb/usbip/src/usbip_network.h > > index c1e875c..e1ca86a 100644 > > --- a/tools/usb/usbip/src/usbip_network.h > > +++ b/tools/usb/usbip/src/usbip_network.h > > @@ -93,7 +93,7 @@ struct op_export_request { } > > __attribute__((packed)); > > > > struct op_export_reply { > > - int returncode; > > + uint32_t returncode; > > } __attribute__((packed)); > > > > > > @@ -115,7 +115,7 @@ struct op_unexport_request { } > > __attribute__((packed)); > > > > struct op_unexport_reply { > > - int returncode; > > + uint32_t returncode; > > } __attribute__((packed)); > > > > #define PACK_OP_UNEXPORT_REQUEST(pack, request) do {\ > > > > The field name is returncode but we have no suitable defines with "return > codes". I ran through USBIP documentation but didn't find any list of allowed > return codes. Is there any? > > You change the value type to unsigned but later you use: > > + if (!error) > + reply.returncode = 0; > + else > + reply.returncode = -1; > > It looks a little bit ugly to me that we change the value to be unsigned > and then assign a signed number to it. In addition your compiler is going > to complain if you use -Wconversion flag. > > In my opinion we should have suitable defines for error codes and those > code should be documented in protocol documentation. > > Best regards > -- > 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
Re: [PATCH v13 01/10] usbip: exporting devices: modifications to network header
Hi, On 11/22/2016 07:48 AM, Nobuo Iwata wrote: > Modification to export and un-export response in > tools/usb/usbip/src/usbip_network.h. It just changes return code type > from int to uint32_t as same as other responses. > > Signed-off-by: Nobuo Iwata> --- > tools/usb/usbip/src/usbip_network.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tools/usb/usbip/src/usbip_network.h > b/tools/usb/usbip/src/usbip_network.h > index c1e875c..e1ca86a 100644 > --- a/tools/usb/usbip/src/usbip_network.h > +++ b/tools/usb/usbip/src/usbip_network.h > @@ -93,7 +93,7 @@ struct op_export_request { > } __attribute__((packed)); > > struct op_export_reply { > - int returncode; > + uint32_t returncode; > } __attribute__((packed)); > > > @@ -115,7 +115,7 @@ struct op_unexport_request { > } __attribute__((packed)); > > struct op_unexport_reply { > - int returncode; > + uint32_t returncode; > } __attribute__((packed)); > > #define PACK_OP_UNEXPORT_REQUEST(pack, request) do {\ > The field name is returncode but we have no suitable defines with "return codes". I ran through USBIP documentation but didn't find any list of allowed return codes. Is there any? You change the value type to unsigned but later you use: + if (!error) + reply.returncode = 0; + else + reply.returncode = -1; It looks a little bit ugly to me that we change the value to be unsigned and then assign a signed number to it. In addition your compiler is going to complain if you use -Wconversion flag. In my opinion we should have suitable defines for error codes and those code should be documented in protocol documentation. Best regards -- 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
Re: [PATCH v13 01/10] usbip: exporting devices: modifications to network header
On 11/21/2016 11:48 PM, Nobuo Iwata wrote: > Modification to export and un-export response in > tools/usb/usbip/src/usbip_network.h. It just changes return code type > from int to uint32_t as same as other responses. > > Signed-off-by: Nobuo IwataLooks fine to me. Acked-by: Shuah Khan > --- > tools/usb/usbip/src/usbip_network.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tools/usb/usbip/src/usbip_network.h > b/tools/usb/usbip/src/usbip_network.h > index c1e875c..e1ca86a 100644 > --- a/tools/usb/usbip/src/usbip_network.h > +++ b/tools/usb/usbip/src/usbip_network.h > @@ -93,7 +93,7 @@ struct op_export_request { > } __attribute__((packed)); > > struct op_export_reply { > - int returncode; > + uint32_t returncode; > } __attribute__((packed)); > > > @@ -115,7 +115,7 @@ struct op_unexport_request { > } __attribute__((packed)); > > struct op_unexport_reply { > - int returncode; > + uint32_t returncode; > } __attribute__((packed)); > > #define PACK_OP_UNEXPORT_REQUEST(pack, request) do {\ > -- Shuah Khan Sr. Linux Kernel Developer Open Source Innovation Group Samsung Research America(Silicon Valley) shuah...@samsung.com -- 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 v13 01/10] usbip: exporting devices: modifications to network header
Modification to export and un-export response in tools/usb/usbip/src/usbip_network.h. It just changes return code type from int to uint32_t as same as other responses. Signed-off-by: Nobuo Iwata--- tools/usb/usbip/src/usbip_network.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/usb/usbip/src/usbip_network.h b/tools/usb/usbip/src/usbip_network.h index c1e875c..e1ca86a 100644 --- a/tools/usb/usbip/src/usbip_network.h +++ b/tools/usb/usbip/src/usbip_network.h @@ -93,7 +93,7 @@ struct op_export_request { } __attribute__((packed)); struct op_export_reply { - int returncode; + uint32_t returncode; } __attribute__((packed)); @@ -115,7 +115,7 @@ struct op_unexport_request { } __attribute__((packed)); struct op_unexport_reply { - int returncode; + uint32_t 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