On 11/13/17 4:05 PM, Nikolay Borisov wrote:
>
>
> On 13.11.2017 23:44, Dave Chinner wrote:
>> On Mon, Nov 13, 2017 at 05:47:53PM +0200, Nikolay Borisov wrote:
>>> Currently the fiemap implementation of xfs_io doesn't support making ranged
>>> queries. This patch implements the '-r' parameter, taking up to 2 arguments -
>>> the starting offset and the length of the region to be queried. This also
>>> requires changing of the final hole range is calculated. Namely, it's now being
>>> done as [last_logical, logical addres of next extent] rather than being
>>> statically determined by [last_logical, filesize].
>>>
>>> Signed-off-by: Nikolay Borisov <nborisov@xxxxxxxx>
>>> ---
>>> V3:
>>> * Fixed a bug where incorrect extent index was shown if we didn't print a
>>> hole. This was due to statically returning 2 at the end of print_(plain|verbose)
>>> Now, the actual number of printed extents inside the 2 functions is used.
>>> This bug is visible only with the -r parameter
>>>
>>> * Fixed a bug where 1 additional extent would be printed. This was a result of
>>> the aforementioned bug fix, since now printing function can return 1 and still
>>> have printed an extent and no hole. This can happen when you use -r parameter,
>>> this is now fixed and a comment explaining it is put.
>>>
>>> * Reworked the handling of the new params by making them arguments to the
>>> -r parameter.
>>>
>>> V2:
>>> * Incorporated Daricks feedback - removed variables which weren't introduced
>>> until the next patch as a result the build with this patch was broken. This is
>>> fixed now
>> .....
>>
>>> @@ -256,9 +269,12 @@ fiemap_f(
>>> int tot_w = 5; /* 5 since its just one number */
>>> int flg_w = 5;
>>> __u64 last_logical = 0;
>>> + size_t fsblocksize, fssectsize;
>>> struct stat st;
>>>
>>> - while ((c = getopt(argc, argv, "aln:v")) != EOF) {
>>> + init_cvtnum(&fsblocksize, &fssectsize);
>>> +
>>> + while ((c = getopt(argc, argv, "aln:vr")) != EOF) {
>>
>> Ok, you're not telling gotopt that "-r" has parameters, so....
>>
>>> switch (c) {
>>> case 'a':
>>> fiemap_flags |= FIEMAP_FLAG_XATTR;
>>> @@ -272,6 +288,50 @@ fiemap_f(
>>> case 'v':
>>> vflag++;
>>> break;
>>> + case 'r':
>>> + /* Parse the first option which is mandatory */
>>> + if (optind < argc && argv[optind][0] != '-') {
>>> + off64_t start_offset = cvtnum(fsblocksize,
>>> + fssectsize,
>>> + argv[optind]);
>>> + if (start_offset < 0) {
>>> + printf("non-numeric offset argument -- "
>>> + "%s\n", argv[optind]);
>>> + return 0;
>>> + }
>>> + last_logical = start_offset;
>>> + optind++;
>>> + } else {
>>> + fprintf(stderr, _("Invalid offset argument for"
>>> + " -r\n"));
>>> + exitcode = 1;
>>> + return 0;
>>> + }
>>> +
>>> + if (optind < argc) {
>>> + /* first check if what follows doesn't begin
>>> + * with '-' which means it would be a param and
>>> + * not an argument
>>> + */
>>> + if (argv[optind][0] == '-') {
>>> + optind--;
>>> + break;
>>> +
>>> + }
>>> +
>>> + off64_t length = cvtnum(fsblocksize,
>>> + fssectsize,
>>> + argv[optind]);
>>> + if (length < 0) {
>>> + printf("non-numeric len argument --"
>>> + " %s\n", argv[optind]);
>>> + return 0;
>>> + }
>>> + len = length;
>>> + range_limit = true;
>>> + }
>>> + break;
>>
>> .... this is pretty nasty because you're having to check if the next
>> option should be parsed by the main loop or not. This assumes that
>> getopt is giving us the options in the order they were presented on
>> the command line, which is not a good assumption to make as glibc
>> can mutate the order as it parses the listi of know options and
>> arguments.
>>
>> Given that "-r" is the only option that has parameters, then this
>> can be massively simplified just by noting we've seen the rflag, and
>> leaving the non-arg parameter parsing to after the end of the loop.
>> i.e.:
>
>
> You are right this is a bit ugly, but it seems more consistend to me,
> rather than something like
>
> xfs_io -c "fiemap -r -n 10 -v 4k 10k" for example. And the reason why
> I'm using this hackish way and not declaring r as r: in getopt is
> because getopt doesn't recognise when a parameter takes more than 1
> argument.
Sorry for chiming in so late after all the go-rounds, but:
Why not just drop -r entirely, and make fiemap go into ranged mode iff a
range is specified at the end of the command, i.e.:
fiemap [ -alv ] [ -n nx ] [ offset length ]
and if no offset/length is specified, map the whole file. You can parse
the optional last 2 arguments just like pread, write, sync_range, sendfile,
or a host of other commands already do (though some are not optional.)
There are /many/ other xfs_io commands that take offset & length at the
end, so this usage should be very familiar to users.
-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
