GSoC 2017 introduction

Dmitry V. Levin ldv at altlinux.org
Fri Mar 10 01:36:18 UTC 2017


On Fri, Mar 10, 2017 at 01:55:13AM +0300, Victor Krapivensky wrote:
> From e8754de2791ddb6c79fd51a49c83582c9d9a01d7 Mon Sep 17 00:00:00 2001
> From: Victor Krapivensky <krapivenskiy.va at phystech.edu>
> Date: Thu, 9 Mar 2017 20:26:14 +0300
> Subject: [PATCH v2] Add support for statx syscall

I've given a very cursory look at this patch, see my comments below.

[...]
> --- a/linux/32/syscallent.h
> +++ b/linux/32/syscallent.h
> @@ -281,6 +281,7 @@
>  [288] = { 4,	TM|SI,		SEN(pkey_mprotect),		"pkey_mprotect"		},
>  [289] = { 2,	0,		SEN(pkey_alloc),		"pkey_alloc"		},
>  [290] = { 1,	0,		SEN(pkey_free),			"pkey_free"		},
> +[383] = { 5,	TD|TF,		SEN(statx),			"statx"			},

This cannot be correct.  AFAICT, statx is wired up only for x86
architectures yet, and m68k is pending
(https://marc.info/?l=linux-kernel&m=148879574112095).  I suppose more
architectures will follow soon, and this generic 32-bit is going to be
assigned the next free number, most likely 291.

> --- /dev/null
> +++ b/statx.c
> @@ -0,0 +1,158 @@
> +/*
> + * Copyright (c) 2017 Victor Krapivensky <krapivenskiy.va at phystech.edu>
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + * 3. The name of the author may not be used to endorse or promote products
> + *    derived from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
> + * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
> + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
> + * IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
> + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
> + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include "defs.h"
> +
> +#include <sys/stat.h>
> +#include <linux/fcntl.h>
> +
> +#include "xlat/statx_masks.h"
> +#include "xlat/statx_attrs.h"
> +#include "xlat/at_statx_sync_types.h"
> +
> +struct struct_statx_timestamp {
> +	int64_t sec;
> +	int32_t nsec;
> +	int32_t reserved;
> +};

This "struct struct" is awkward.  Possible solutions:
- rename struct_statx_timestamp to strace_statx_timestamp;
- define an anon struct and use a typedef, e.g.
  typedef struct { ... } struct_statx_timestamp;

I prefer the shorter variant.

> +struct struct_statx {
> +	uint32_t stx_mask; /* What results were written [uncond] */
> +	uint32_t stx_blksize; /* Preferred general I/O size [uncond] */
> +	uint64_t stx_attributes; /* Flags conveying information about the file
> +	                            [uncond] */
> +
> +	uint32_t stx_nlink; /* Number of hard links */
> +	uint32_t stx_uid; /* User ID of owner */
> +	uint32_t stx_gid; /* Group ID of owner */
> +	uint16_t stx_mode; /* File mode */
> +	uint16_t intpa_tre0[1];
> +
> +	uint64_t stx_ino; /* Inode number */
> +	uint64_t stx_size; /* File size */
> +	uint64_t stx_blocks; /* Number of 512-byte blocks allocated */
> +	uint64_t intpa_tre1[1];
> +
> +	struct struct_statx_timestamp stx_atime; /* Last access time */
> +	struct struct_statx_timestamp stx_btime; /* File creation time */
> +	struct struct_statx_timestamp stx_ctime; /* Last attribute change
> +	                                            time */
> +	struct struct_statx_timestamp stx_mtime; /* Last data modification
> +	                                            time */
> +
> +	uint32_t stx_rdev_major; /* Device ID of special file [if bdev/cdev] */
> +	uint32_t stx_rdev_minor;
> +	uint32_t stx_dev_major; /* ID of device containing file [uncond] */
> +	uint32_t stx_dev_minor;
> +
> +	uint64_t intpa_tre2[16]; /* Spare space for future expansion */
> +};

Likewise.

btw, what is intpa_treN?  Aren't they just pads or reserved space?

> +SYS_FUNC(statx)
> +{
> +	if (entering(tcp)) {
> +		print_dirfd(tcp, tcp->u_arg[0]);
> +		printpath(tcp, tcp->u_arg[1]);
> +		tprints(", ");
> +		if (printflags(at_flags, tcp->u_arg[2] & ~AT_STATX_SYNC_TYPE,
> +		               NULL))
> +		{
> +			tprints("|");
> +		}
> +		const char *name = xlookup(at_statx_sync_types,
> +		                           tcp->u_arg[2] & AT_STATX_SYNC_TYPE);
> +		if (name) {
> +			tprints(name);
> +		} else {
> +			tprintf("%#llx /* AT_STATX_SYNC_TYPE_??? */",
> +			        (unsigned long long) tcp->u_arg[2]);
> +		}

This is a bug: tcp->u_arg[2] & ~AT_STATX_SYNC_TYPE has already been
printed a few lines earlier.

I think you can use printxval here.

> +		tprints(", ");
> +		printflags(statx_masks, tcp->u_arg[3], "STATX_???");
> +		tprints(", ");
> +	} else {
> +#define PRINT_FIELD_U(field) \
> +	tprintf(", %s=%llu", #field, (unsigned long long) stx.field)
> +
> +#define ABS(x) ((x) < 0 ? -(x) : (x))
> +
> +#define PRINT_FIELD_TIME(field)						\
> +	do {								\
> +		tprints(", " #field "=");				\
> +		tprints(sprinttime(stx.field.sec));			\
> +		tprintf(".%09lld", (long long) ABS(stx.field.nsec));	\

This is wrong, the type of nsec is int32_t.  I have no idea why it is
defined as signed, I see no reason for that.  Anyway, you shouldn't just
print an absolute value, it wouldn't be correct.  I suggest printing it
using %09u format.


-- 
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20170310/fe768aa6/attachment.bin>


More information about the Strace-devel mailing list