On Fri, Jul 03, 2015 at 03:33:31AM +0300, Dmitry V. Levin wrote:
> On Wed, Jul 01, 2015 at 02:52:45PM +0200, Patrik Jakobsson wrote:
> [...]
> > --- a/defs.h
> > +++ b/defs.h
> > @@ -266,6 +266,13 @@ struct tcb {
> > int u_error; /* Error code */
> > long scno; /* System call number */
> > long u_arg[MAX_ARGS]; /* System call arguments */
> > +
> > + /*
> > + * Private data for the decoding functions of the syscall. TCB core does
> > + * _not_ handle allocation / deallocation of this data.
> > + */
> > + void *priv_data;
> > +
>
> This will result to memory leaks if droptcb() is called before the
> syscall parser that allocated memory had a chance to deallocate it.
> As this data is no longer relevant after leaving trace_syscall_exiting(),
> I suggest to perform deallocation directly from trace_syscall_exiting.
>
> This API could be made more flexible by adding another pointer -
> the function to be called to deallocate memory, e.g.
> struct tcb {
> ...
> void *priv_data;
> void (*free_priv_data)(void *);
> ...
> };
> ...
> void
> free_priv_data(struct tcb *tcp)
> {
> if (tcp->priv_data) {
> if (tcp->free_priv_data) {
> tcp->free_priv_data(tcp->priv_data);
> tcp->free_priv_data = NULL;
> }
> tcp->priv_data = NULL;
> }
> }
> ...
> droptcb(struct tcb *tcp)
> {
> ...
> free_priv_data(tcp);
> ...
> }
> ...
> trace_syscall_exiting(struct tcb *tcp)
> {
> ...
> ret:
> free_priv_data(tcp);
> ...
> }
>
> [...]
> On Wed, Jul 01, 2015 at 02:52:46PM +0200, Patrik Jakobsson wrote:
Yes, that's more robust. I was afraid it would be too intrusive.
> > * Makefile.am: Add compilation of drm.c
> > * defs.h: Declarations of drm functions
> > * drm.c: Utility functions for drm driver detection
> > * io.c: Dispatch drm ioctls
> > * ioctl.c: Distpatch generic and driver specific ioctls
>
> This is not quite a GNU style changelog entry. Please have a look at
> http://www.gnu.org/prep/standards/html_node/Style-of-Change-Logs.html
> and examples in strace.git history.
>
> [...]
I'll get that sorted out.
> > +#include "defs.h"
> > +
> > +#include <drm.h>
> > +#include <linux/limits.h>
>
> Please include <sys/param.h> instead of <linux/limits.h>.
Yup
> > +#define DRM_MAX_NAME_LEN 128
> > +
> > +struct drm_ioctl_priv {
> > + char name[DRM_MAX_NAME_LEN];
> > +};
> > +
> > +inline int drm_is_priv(const unsigned int num)
> > +{
> > + return (_IOC_NR(num) >= DRM_COMMAND_BASE &&
> > + _IOC_NR(num) < DRM_COMMAND_END);
> > +}
> > +
> > +static int drm_get_driver_name(struct tcb *tcp, char *name, size_t bufsize)
> > +{
> > + char path[PATH_MAX];
> > + char link[PATH_MAX];
> > + int ret;
> > +
> > + ret = getfdpath(tcp, tcp->u_arg[0], path, PATH_MAX - 1);
> > + if (ret < 0)
> > + return ret;
> > +
> > + snprintf(link, PATH_MAX, "/sys/class/drm/%s/device/driver",
> > + basename(path));
> > +
> > + ret = readlink(link, path, PATH_MAX - 1);
> > + if (ret < 0)
> > + return ret;
> > +
> > + path[ret] = '\0';
> > + strncpy(name, basename(path), bufsize);
> > +
> > + return 0;
> > +}
>
> I think this is getting too complicated. This function could just return
> strdup(basename(path)) or NULL in case of any error:
>
> static char *
> drm_get_driver_name(struct tcb *tcp, const char *name)
> {
> char path[PATH_MAX];
> char link[PATH_MAX];
> int ret;
>
> if (getfdpath(tcp, tcp->u_arg[0], path, PATH_MAX - 1) < 0)
> return NULL;
>
> if (snprintf(link, PATH_MAX, "/sys/class/drm/%s/device/driver",
> basename(path)) >= PATH_MAX)
> return NULL;
>
> ret = readlink(link, path, PATH_MAX - 1);
> if (ret < 0)
> return NULL;
>
> path[ret] = '\0';
> return strdup(basename(path));
> }
>
That's nicer
> > +
> > +int drm_is_driver(struct tcb *tcp, const char *name)
> > +{
> > + struct drm_ioctl_priv *priv;
> > + int ret;
> > +
> > + /*
> > + * If no private data is allocated we are detecting the driver name for
> > + * the first time and must resolve it.
> > + */
> > + if (tcp->priv_data == NULL) {
> > + tcp->priv_data = xcalloc(1, sizeof(struct drm_ioctl_priv));
>
> xcalloc shouldn't be used if a potential memory allocation error is not
> fatal. In a parser that performs verbose syscall decoding no memory
> allocation error is fatal.
Ok
> > + priv = tcp->priv_data;
> > +
> > + ret = drm_get_driver_name(tcp, priv->name, DRM_MAX_NAME_LEN);
> > + if (ret)
> > + return 0;
> > + }
> > +
> > + priv = tcp->priv_data;
> > +
> > + return strncmp(name, priv->name, DRM_MAX_NAME_LEN) == 0;
>
> Then with priv_data+free_priv_data interface this would looks smth like
> ...
> if (!tcp->priv_data) {
> tcp->priv_data = drm_get_driver_name(tcp, name);
> if (tcp->priv_data) {
> tcp->free_priv_data = free;
> } else {
> tcp->priv_data = (void *) "";
> tcp->free_priv_data = NULL;
> }
> }
> return !strcmp(name, (char *) tcp->priv_data);
>
> > +}
> > +
> > +int drm_decode_number(struct tcb *tcp, unsigned int arg)
>
> This is an ioctl request code, let's consistently call it "code" to
> distinguish from its argument.
>
> [...]
> > --- a/ioctl.c
> > +++ b/ioctl.c
> > @@ -182,7 +182,7 @@ hiddev_decode_number(unsigned int arg)
> > }
> >
> > int
> > -ioctl_decode_command_number(unsigned int arg)
> > +ioctl_decode_command_number(struct tcb *tcp, unsigned int arg)
>
> I've already changed ioctl_decode_command_number's signature:
> struct tcb * is already there and "arg" is now called "code".
>
>
> --
> ldv
------------------------------------------------------------------------------
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/
_______________________________________________
Strace-devel mailing list
Strace-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/strace-devel
