From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Dmitry V. Levin" Subject: Re: [PATCH v3 2/5] drm: Add private data field to trace control block Date: Fri, 3 Jul 2015 03:33:31 +0300 Message-ID: <20150703003331.GA29080@altlinux.org> References: <1435755168-16207-4-git-send-email-patrik.jakobsson@linux.intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2015583611==" Return-path: Received: from pegasus3.altlinux.org (pegasus3.altlinux.org [194.107.17.103]) by gabe.freedesktop.org (Postfix) with ESMTP id 6E10A6E1F9 for ; Thu, 2 Jul 2015 17:33:34 -0700 (PDT) In-Reply-To: <1435755168-16207-4-git-send-email-patrik.jakobsson@linux.intel.com> <1435755168-16207-3-git-send-email-patrik.jakobsson@linux.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Patrik Jakobsson Cc: intel-gfx@lists.freedesktop.org, strace-devel@lists.sourceforge.net List-Id: intel-gfx@lists.freedesktop.org --===============2015583611== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="oyUTqETQ0mS9luUI" Content-Disposition: inline --oyUTqETQ0mS9luUI Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 *); ... }; =2E.. 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 =3D NULL; } tcp->priv_data =3D NULL; } } =2E.. droptcb(struct tcb *tcp) { ... free_priv_data(tcp); ... } =2E.. trace_syscall_exiting(struct tcb *tcp) { ... ret: free_priv_data(tcp); ... } [...] On Wed, Jul 01, 2015 at 02:52:46PM +0200, Patrik Jakobsson wrote: > * 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. [...] > +#include "defs.h" > + > +#include > +#include Please include instead of . > +#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) >=3D DRM_COMMAND_BASE && > + _IOC_NR(num) < DRM_COMMAND_END); > +} > + > +static int drm_get_driver_name(struct tcb *tcp, char *name, size_t bufsi= ze) > +{ > + char path[PATH_MAX]; > + char link[PATH_MAX]; > + int ret; > + > + ret =3D 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 =3D readlink(link, path, PATH_MAX - 1); > + if (ret < 0) > + return ret; > + > + path[ret] =3D '\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)) >=3D PATH_MAX) return NULL; ret =3D readlink(link, path, PATH_MAX - 1); if (ret < 0) return NULL; path[ret] =3D '\0'; return strdup(basename(path)); } > + > +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 =3D=3D NULL) { > + tcp->priv_data =3D 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. > + priv =3D tcp->priv_data; > + > + ret =3D drm_get_driver_name(tcp, priv->name, DRM_MAX_NAME_LEN); > + if (ret) > + return 0; > + } > + > + priv =3D tcp->priv_data; > + > + return strncmp(name, priv->name, DRM_MAX_NAME_LEN) =3D=3D 0; Then with priv_data+free_priv_data interface this would looks smth like ... if (!tcp->priv_data) { tcp->priv_data =3D drm_get_driver_name(tcp, name); if (tcp->priv_data) { tcp->free_priv_data =3D free; } else { tcp->priv_data =3D (void *) ""; tcp->free_priv_data =3D 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) > } > =20 > 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". --=20 ldv --oyUTqETQ0mS9luUI Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iEYEARECAAYFAlWV2FsACgkQfKvmrJ41Nh5DiwCcCgq5wmiJwzpyv6ZAq26JzLEk aG0An31UU89RPhPWZZPy5jYNo4lqZx5S =JfWn -----END PGP SIGNATURE----- --oyUTqETQ0mS9luUI-- --===============2015583611== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9pbnRlbC1nZngK --===============2015583611==--