Re: [Tarantool-patches] [PATCH v4 4/4] crash: report crash data to the feedback server

[Serge Petrenko <sergepetrenko@tarantool.org>]

10.12.2020 19:18, Cyrill Gorcunov пишет:
> We have a feedback server which gathers information about a running instnace.
> While general info is enough for now we may loose a precious information about
> crashes (such as call backtrace which caused the issue, type of build and etc).
>
> In the commit we add support of sending this kind of infomation to the feedback
> server. Internally we gather the reason of failure, pack it into base64 form
> and then run another tarantool instance which sends it out.
>
> A typical report might look like
>
>   | {
>   |   "crashdump": {
>   |     "version": "1",
>   |     "data": "eyJ1bmFtZSI6eyJzeXNuYW1lIjoiTGludXgiLCJyZWxlYXNlIjoiNS"
>   |             "45LjExLTEwMC5mYzMyLng4Nl82NCIsInZlcnNpb24iOiIjMSBTTVAg"
>   |             "VHVlIE5vdiAyNCAxOToxNjo1MyBVVEMgMjAyMCIsIm1hY2hpbmUiOi"
>   |             "J4ODZfNjQifSwiYnVpbGQiOnsidmVyc2lvbiI6IjIuNy4wLTgzLWc5"
>   |             "ZTg1MDM4ZmIiLCJjbWFrZV90eXBlIjoiTGludXgteDg2XzY0LURlYn"
>   |             "VnIn0sInNpZ25hbCI6eyJzaWdubyI6MTEsInNpX2NvZGUiOjAsInNp"
>   |             "X2FkZHIiOiIweDNlODAwMDVjOGE5IiwiYmFja3RyYWNlIjoiSXpBZ0"
>   |             "lEQjROak14WkRBeklHbHVJR055WVhOb1gyTnZiR3hsWTNRclltWUtJ"
>   |             "ekVnSURCNE5qTXlaV1JoSUdsdUlHTnlZWE5vWDNOcFoyNWhiRjlqWW"
>   |             "lzMU1Rb2pNaUFnTUhnM1pqRmtORGt5TXpsaE9UQWdhVzRnWm5WdWJH"
>   |             "OWphMlpwYkdVck5qQUtJek1nSURCNE4yWXhaRFE0WkdReFl6VmxJR2"
>   |             "x1SUdWd2IyeHNYM2RoYVhRck5XVUtJelFnSURCNE9ETTNOekV6SUds"
>   |             "dUlHVndiMnhzWDNCdmJHd3JPVGtLSXpVZ0lEQjRPRE5oWkRRNElHbH"
>   |             "VJR1YyWDNKMWJpczBPRGNLSXpZZ0lEQjROakJtTUdGbElHbHVJSFJo"
>   |             "Y21GdWRHOXZiRjlzZFdGZmNuVnVYM05qY21sd2RDc3hNemdLSXpjZ0"
>   |             "lEQjRORFEyTldZMUlHbHVJRzFoYVc0ck5XRmpDaU00SUNBd2VEZG1N"
>   |             "V1EwT0dObU56QTBNaUJwYmlCZlgyeHBZbU5mYzNSaGNuUmZiV0ZwYm"
>   |             "l0bU1nb2pPU0FnTUhnME5EUmhNR1VnYVc0Z1gzTjBZWEowS3pKbENn"
>   |             "PT0iLCJ0aW1lc3RhbXAiOiIyMDIwLTEyLTEwIDE3OjA4OjQyIE1TSyJ9fQ=="
>   |   }
>   | }
>
> The `data` value is a single string I wrapped it for commit message only.
> When `data` is decoded it consists of
>
>   | {
>   |   "uname": {
>   |     "sysname": "Linux",
>   |     "release": "5.9.11-100.fc32.x86_64",
>   |     "version": "#1 SMP Tue Nov 24 19:16:53 UTC 2020",
>   |     "machine": "x86_64"
>   |   },
>   |   "build": {
>   |     "version": "2.7.0-83-g9e85038fb",
>   |     "cmake_type": "Linux-x86_64-Debug"
>   |   },
>   |   "signal": {
>   |     "signo": 11,
>   |     "si_code": 0,
>   |     "si_addr": "0x3e80005c8a9",
>   |     "backtrace": "IzAgIDB4NjMxZDAzIGluIGNyYXNoX2NvbGxlY3QrYmYKIzEgID"
>   |                  "B4NjMyZWRhIGluIGNyYXNoX3NpZ25hbF9jYis1MQojMiAgMHg3ZjFkNDkyMzlhO"
>   |                  "TAgaW4gZnVubG9ja2ZpbGUrNjAKIzMgIDB4N2YxZDQ4ZGQxYzVlIGluIGVwb2xs"
>   |                  "X3dhaXQrNWUKIzQgIDB4ODM3NzEzIGluIGVwb2xsX3BvbGwrOTkKIzUgIDB4ODN"
>   |                  "hZDQ4IGluIGV2X3J1bis0ODcKIzYgIDB4NjBmMGFlIGluIHRhcmFudG9vbF9sdW"
>   |                  "FfcnVuX3NjcmlwdCsxMzgKIzcgIDB4NDQ2NWY1IGluIG1haW4rNWFjCiM4ICAwe"
>   |                  "DdmMWQ0OGNmNzA0MiBpbiBfX2xpYmNfc3RhcnRfbWFpbitmMgojOSAgMHg0NDRh"
>   |                  "MGUgaW4gX3N0YXJ0KzJlCg==",
>   |     "timestamp": "2020-12-10 17:08:42 MSK"
>   |   }
>   | }


Thanks for the patch!

Please find one comment below.


> @@ -126,6 +214,179 @@ crash_collect(int signo, siginfo_t *siginfo, void *ucontext)
>   	return cinfo;
>   }
>   
> +/**
> + * Report crash information to the feedback daemon
> + * (ie send it to feedback daemon).
> + */
> +static void
> +crash_report_feedback_daemon(struct crash_info *cinfo)
> +{
> +	/*
> +	 * Update to a new number if format get changed.
> +	 */
> +	static const int crashinfo_version = 1;
> +
> +	char *p = static_alloc(SMALL_STATIC_SIZE);
> +	char *e = &p[SMALL_STATIC_SIZE - 1];
> +	char *head = p;
> +	char *tail = &p[SMALL_STATIC_SIZE - 8];
> +
> +	/*
> +	 * Note that while we encode the report we
> +	 * intensively use a tail of the allocated
> +	 * buffer as a temporary store.
> +	 */
> +
> +#define snprintf_safe(fmt, ...)					\
> +	do {							\
> +		p += snprintf(p, e - p, fmt, ##__VA_ARGS__);	\
> +		if (p >= e)					\
> +			goto out;				\
> +	} while (0)
> +
> +	/*
> +	 * On linux there is new_utsname structure which
> +	 * encodes each field to __NEW_UTS_LEN + 1 => 64 + 1 = 65.
> +	 *
> +	 * So lets just reserve more data in advance: 5 fields
> +	 * 128 bytes each => 640 bytes.
> +	 */
> +	static_assert(sizeof(struct utsname) <= 640,
> +		      "utsname is bigger than expected");


This static assert fails on my mac.
Looks like `struct utsname` is 1280 bytes in size there.


> +
> +	/*
> +	 * Lets reuse tail of the buffer as a temp space.
> +	 */
> +	struct utsname *uname_ptr = (void *)&tail[-640];
> +	if (p > (char *)(void *)uname_ptr)
> +		goto out;
> +
> +	if (uname(uname_ptr) != 0) {
> +		pr_syserr("uname call failed, ignore");
> +		memset(uname_ptr, 0, 640);
> +	}
> +

-- 
Serge Petrenko