[CRIU] [PATCH 12/17] proc_parse: Add parsing of fanotify entries

[Pavel Emelyanov]

On 12/24/2012 05:57 PM, Cyrill Gorcunov wrote:
> On Mon, Dec 24, 2012 at 05:28:04PM +0400, Pavel Emelyanov wrote:
>>
>> Why adding _new_ stuff to parse results in rewriting the _existing_ stuff parsing?
> 
> Because of the kernel format specifics -- I've to return the @type of the
> parsed entry. Previously we only were carried inotify objects, now I need
> to parse fanotify, and adding "call-back" parameters allowed me to unitfy
> the code.
> 
> I believe you don't like it because of the patch view itself. Here is how
> it looks in real
> 
> 
> +		if (fdinfo_field(str, "fanotify flags")) {
> +			struct fsnotify_params *p = arg;
> +
> +			if (type != FD_TYPES__FSNOTIFY)
> +				goto parse_err;
> +
> +			ret = sscanf(str, "fanotify flags:%x event-flags:%x",
> +				     &p->flags, &p->eflags);
> +			if (ret != 2)
> +				goto parse_err;
> +			entry_met = true;
> +			continue;
> +		}
> +		if (fdinfo_field(str, "inotify wd")	||
> +		    fdinfo_field(str, "fanotify ino")	||
> +		    fdinfo_field(str, "fanotify mnt_id")) {
>  			FhEntry f_handle = FH_ENTRY__INIT;
> +			struct fsnotify_params *p = arg;
>  			int hoff;
>  
>  			fsnotify_wd_entry__init(&entry.fsy);
>  			entry.fsy.f_handle = &f_handle;
>  			entry.fsy.has_type = true;
>  
>  			if (type != FD_TYPES__FSNOTIFY)

The "inotify wd" string results in parse error? Why?

>  				goto parse_err;
>  
> +			entry.fsy.has_type = true;
> +			switch (str[9]) {
> +			case 'd':
> +			case 'i':
> +				if (str[9] == 'd') {
> +					entry.fsy.type = FSNOTIFY_TYPE__INOTIFY;
> +					ret = sscanf(&str[11],
> +						     "%x ino:%lx sdev:%x "
> +						     "mask:%x ignored_mask:%x "
> +						     "fhandle-bytes:%x fhandle-type:%x "
> +						     "f_handle: %n",
> +						     &entry.fsy.wd, &entry.fsy.i_ino, &entry.fsy.s_dev,
> +						     &entry.fsy.mask, &entry.fsy.ignored_mask,
> +						     &entry.fsy.f_handle->bytes, &entry.fsy.f_handle->type,
> +						     &hoff);
> +					if (ret != 7)
> +						goto parse_err;
> +					hoff += 11;
> +				} else {
> +					entry.fsy.type = FSNOTIFY_TYPE__FANOTIFY;
> +					ret = sscanf(&str[13],
> +						     "%lx sdev:%x "
> +						     "mflags:%x mask:%x ignored_mask:%x "
> +						     "fhandle-bytes:%x fhandle-type:%x "
> +						     "f_handle: %n",
> +						     &entry.fsy.i_ino, &entry.fsy.s_dev,
> +						     &entry.fsy.wd, &entry.fsy.mask, &entry.fsy.ignored_mask,
> +						     &entry.fsy.f_handle->bytes, &entry.fsy.f_handle->type,
> +						     &hoff);
> +					if (ret != 7)
> +						goto parse_err;
> +					hoff += 13;
> +				}
>  
> +				f_handle.n_handle = FH_ENTRY_SIZES__min_entries;
> +				f_handle.handle = xmalloc(pb_repeated_size(&f_handle, handle));
> +				if (!f_handle.handle)
> +					return -1;
>  
> +				parse_fhandle_encoded(str + hoff, entry.fsy.f_handle);
> +				break;
> +			case 'm':
> +				/*
> +				 * In real it's not @s_dev parsed here but
> +				 * @mnt_id. Thus the caller must be ready
> +				 * for that and handle this trick accordingly.
> +				 */
> +				entry.fsy.type = FSNOTIFY_TYPE__FANOTIFY;
> +				ret = sscanf(&str[16],
> +					     "%x mflags:%x mask:%x ignored_mask:%x",
> +					     &entry.fsy.s_dev, &entry.fsy.wd,
> +					     &entry.fsy.mask, &entry.fsy.ignored_mask);
> +				if (ret != 4)
> +					goto parse_err;
> +				break;
> +			default:
> +				BUG();
> +				break;
> +			}
> +
> +			ret = cb(&entry, p);

This is ugly anyway. The resulting code looks like

if (field_is("xyzabc") || field_is("wvubcd") || field_is("wvucde")) {
	foo();
	switch (field[2]) { /* why field[2], not 3 or any other magic number? 
                             * because it's the first letter, that differs between
                             * these tree, isn't it obvious?! */
	'a':
		do_a();
		break;
	'b':
		do_b();
		break;
	'c':
		do_c();
		break;
	}

	continue;
}

instead of plain and readable

if (field_is("xyzabc")) {
	foo();
	do_a();
	continue;
}

if (field_is("wvubcd")) {
	foo();
	do_b();
	continue;
}

if (field_is("wvucde")) {
	foo();
	do_c();
	continue;
}

>  			xfree(f_handle.handle);
> 
> 	Cyrill
> .
>