| Message ID | 20200415150409.27938-2-email@uajain.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, On 15/04/2020 16:04, Umang Jain wrote: > Pointed out by Coverity DefectId=279052 > > Signed-off-by: Umang Jain <email@uajain.com> > --- > test/ipc/unixsocket.cpp | 19 ++++++++++++------- > 1 file changed, 12 insertions(+), 7 deletions(-) > > diff --git a/test/ipc/unixsocket.cpp b/test/ipc/unixsocket.cpp > index 5348f35..7bc6a89 100644 > --- a/test/ipc/unixsocket.cpp > +++ b/test/ipc/unixsocket.cpp > @@ -16,6 +16,7 @@ > #include <unistd.h> > > #include <libcamera/event_dispatcher.h> > +#include <libcamera/file_descriptor.h> > #include <libcamera/timer.h> > > #include "ipc_unixsocket.h" > @@ -467,18 +468,22 @@ private: > if (fd < 0) > return fd; > > + FileDescriptor file_desc = FileDescriptor(fd); > + close(fd); Since you created this patch, the FileDescriptor has been updated to take move semantics to avoid having to close an FD directly after instantiating. I think updating this patch to use that new API needs to happen in this patch, could you update please? I'm not 100% sure of the syntax, but I think it's something like as simple as: file_desc = FileDescriptor(std::move(fd)); > + if (file_desc.fd() < 0) > + return file_desc.fd(); > + > int size = 0; > for (unsigned int i = 0; i < num; i++) { > - int clone = dup(fd); > - if (clone < 0) > - return clone; > + FileDescriptor *clone = new FileDescriptor(file_desc.fd()); > + int clone_int = clone->fd(); I wouldn't use a local var here,, just reference clone->fd() below instead. > + if (clone_int < 0) > + return clone_int; > > - size += calculateLength(clone); > - message->fds.push_back(clone); > + size += calculateLength(clone_int); > + message->fds.push_back(clone_int); > } > > - close(fd); > - > return size; > } > >
Hi Umang, On Thu, Jun 04, 2020 at 10:53:16AM +0100, Kieran Bingham wrote: > On 15/04/2020 16:04, Umang Jain wrote: > > Pointed out by Coverity DefectId=279052 > > > > Signed-off-by: Umang Jain <email@uajain.com> > > --- > > test/ipc/unixsocket.cpp | 19 ++++++++++++------- > > 1 file changed, 12 insertions(+), 7 deletions(-) > > > > diff --git a/test/ipc/unixsocket.cpp b/test/ipc/unixsocket.cpp > > index 5348f35..7bc6a89 100644 > > --- a/test/ipc/unixsocket.cpp > > +++ b/test/ipc/unixsocket.cpp > > @@ -16,6 +16,7 @@ > > #include <unistd.h> > > > > #include <libcamera/event_dispatcher.h> > > +#include <libcamera/file_descriptor.h> > > #include <libcamera/timer.h> > > > > #include "ipc_unixsocket.h" > > @@ -467,18 +468,22 @@ private: > > if (fd < 0) > > return fd; > > > > + FileDescriptor file_desc = FileDescriptor(fd); > > + close(fd); > > Since you created this patch, the FileDescriptor has been updated to > take move semantics to avoid having to close an FD directly after > instantiating. > > I think updating this patch to use that new API needs to happen in this > patch, could you update please? > > I'm not 100% sure of the syntax, but I think it's something like as > simple as: > > file_desc = FileDescriptor(std::move(fd)); This creates temporary file descriptor object and then uses copy assignment. The compiler will optimize it out, but it's best to write FileDescriptor file_desc{ std::move(fd) }; > > + if (file_desc.fd() < 0) > > + return file_desc.fd(); > > + > > int size = 0; > > for (unsigned int i = 0; i < num; i++) { > > - int clone = dup(fd); > > - if (clone < 0) > > - return clone; > > + FileDescriptor *clone = new FileDescriptor(file_desc.fd()); > > + int clone_int = clone->fd(); > > I wouldn't use a local var here,, just reference clone->fd() below instead. I wouldn't use FileDescriptor here actually. The reason why new is used above is (I assume) to avoid closing the clone when the FileDescriptor instance is destroyed, but that ends up leaking that new FileDescriptor instance. FileDescriptor is fine for file_desc (which should be named fileDesc as we use camelCase), but here you can just keep the int clone variable. > > + if (clone_int < 0) > > + return clone_int; > > > > - size += calculateLength(clone); > > - message->fds.push_back(clone); > > + size += calculateLength(clone_int); > > + message->fds.push_back(clone_int); > > } > > > > - close(fd); > > - > > return size; > > } > >
diff --git a/test/ipc/unixsocket.cpp b/test/ipc/unixsocket.cpp index 5348f35..7bc6a89 100644 --- a/test/ipc/unixsocket.cpp +++ b/test/ipc/unixsocket.cpp @@ -16,6 +16,7 @@ #include <unistd.h> #include <libcamera/event_dispatcher.h> +#include <libcamera/file_descriptor.h> #include <libcamera/timer.h> #include "ipc_unixsocket.h" @@ -467,18 +468,22 @@ private: if (fd < 0) return fd; + FileDescriptor file_desc = FileDescriptor(fd); + close(fd); + if (file_desc.fd() < 0) + return file_desc.fd(); + int size = 0; for (unsigned int i = 0; i < num; i++) { - int clone = dup(fd); - if (clone < 0) - return clone; + FileDescriptor *clone = new FileDescriptor(file_desc.fd()); + int clone_int = clone->fd(); + if (clone_int < 0) + return clone_int; - size += calculateLength(clone); - message->fds.push_back(clone); + size += calculateLength(clone_int); + message->fds.push_back(clone_int); } - close(fd); - return size; }
Pointed out by Coverity DefectId=279052 Signed-off-by: Umang Jain <email@uajain.com> --- test/ipc/unixsocket.cpp | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)