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:shuahkh@xxxxxxxxxxxxxxx] > Sent: Friday, January 13, 2017 2:44 AM > To: fx IWATA NOBUO <Nobuo.Iwata@xxxxxxxxxxxxxxx>; > valentina.manea.m@xxxxxxxxx; shuah@xxxxxxxxxx; > gregkh@xxxxxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx > Cc: fx MICHIMURA TADAO <MICHIMURA.Tadao@xxxxxxxxxxxxxxx>; Shuah > Khan <shuahkh@xxxxxxxxxxxxxxx>; Shuah Khan <shuah@xxxxxxxxxx> > 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.iwata@xxxxxxxxxxxxxxx> > > --- > > 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_t status; > > + 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_OK 0x00 > > -#define ST_NA 0x01 > > +#define ST_OK 0x00 > > +#define ST_NA 0x01 > > +#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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
