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