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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
