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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.opasiak@xxxxxxxxxxx]
> Sent: Friday, December 02, 2016 4:11 AM
> To: fx IWATA NOBUO; valentina.manea.m@xxxxxxxxx; shuah.kh@xxxxxxxxxxx;
> gregkh@xxxxxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx
> 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.iwata@xxxxxxxxxxxxxxx>
> > ---
> >  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&D Institute Poland
> Samsung Electronics
--
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




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux