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...@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...@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
pgpzEjuPfGrJy.pgp
Description: PGP signature
------------------------------------------------------------------------------ Announcing the Oxford Dictionaries API! The API offers world-renowned dictionary content that is easy and intuitive to access. Sign up for an account today to start using our lexical data to power your apps and projects. Get started today and enter our developer competition. http://sdm.link/oxford
_______________________________________________ Strace-devel mailing list Strace-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/strace-devel
