LINUX.ORG.RU

есть ли ошибки в xfce плагине clor?

 , ,


0

1

Написал на днях программу, плагин. Это xfce4 плагин для панели. Может кто нибудь посмотреть код и сказать, есть ли там ошибки какие нибудь? Но вроде работает всё нормально. Есть правда момент, где выделяется память, когда нужно открыть свойства плагина и потом не удаляется. Если добавить событие, то удалять придётся все виджеты окна, так что пока ищу способ лучше.

click

☆

Последнее исправление: u0atgKIRznY5 (всего исправлений: 2)
		char *cookie = malloc ( 128 );
		memset ( cookie, 0, 128 );

calloc религия запрещает использовать? Почему константа? Хотите SIGSEGV на ровном месте из-за sscanf? Да и вообще:

sscanf ( scrf, "%s", cookie );
Я бы понял чтение инта (хоть и небезопасный, но метод), но блин, строку? Серьезно?

	if ( !strncmp ( response, "HTTP/1.1 200 OK", 15 ) ) {

Ну такое себе решение.

			if ( len == 16384 ) while ( ( len = SSL_read ( ssl, response, 16384 ) ) == 16384 );

Что это за магия? Где нормально названные константы или define'ы? Что тут вообще происходит?

		FILE *fp = fopen ( file, "r" );

Где проверка что он открылся? Выпадет же в sigsegv на fget

	pw = getpwuid ( getuid ( ) );
	char *file = g_strdup_printf (
			"/home/%s/.clor.conf",
			pw->pw_name
			);

Самый ненадежный способ. getenv(«HOME»)

		if ( request ) free ( request );
		if ( response ) free ( response );

Ну и зачем условия, если у вас malloc в самом начале а все остальные ветки в return?

Это то что в глаза бросается. Исправите - будем смотреть дальше.

PPP328 ★★★★★
()

Код особо не смотрел, но вот что бросилось в глаза: раздели пользовательский интерфейс и логику работы. Т. о. у тебя получится минимум два C-файла, в каждом только то, что относится к его области. В процессе разбивки попрактикуешься в тонкостях проектирования. Бонусом получишь меньший объём кода на файл, а если сделаешь разбивку как надо, то и число видимых функций сократишь, так проще просматривать и искать ошибки

XMs ★★★★★
()

Ещё вот на что наткнулся: авторизацию нужно вынести в отдельную функцию, которую уже будешь дёргать, когда надо. Посылку запросов (именно write() в сокет), возможно, тоже, чтоб в одном месте проверять на коды ошибок. Ещё, возможно, имеет смысл для каждого HTTP-запроса сделать свою функцию, которая заодно и ответ проверит, но я с HTTP не работал, поэтому не берусь утверждать, что это прям хорошее решение, тут уже пусть знающие подскажут

XMs ★★★★★
()

В функции talk() вызов SSL_shutdown, очистку памяти и вот это вот всё имеет смысл собрать в одном месте, куда потом делать goto. В ядре, например, это стандартная практика. Про магические константы, работу с памятью и прочее тебе уже написали, дублировать не стану

XMs ★★★★★
()
Ответ на: комментарий от XMs

собрать в одном месте, куда потом делать goto.

Да я тоже так иногда думал, чтобы не плодить один и тот же код.

u0atgKIRznY5 ☆
() автор топика

Ещё такая ошибка есть, что в i686 выключается после определённой работы, а 64 битной нормально работает. Даже и не знаю что может быть.

u0atgKIRznY5 ☆
() автор топика
Ответ на: комментарий от XMs

В ядре, например, это стандартная практика.

Я тоже такое видел не раз.

u0atgKIRznY5 ☆
() автор топика
Ответ на: комментарий от XMs

Я обдумывал как то, как можно создать отдельные функции отправки разных запросов, остановился на том, чтобы создать структуру, и её передавать как параметр.

Насчёт ошибки, может дело в этом, в том что лучше выделить в bss память?

https://github.com/xverizex/clor/blob/master/main.c#L178

u0atgKIRznY5 ☆
() автор топика
Ответ на: комментарий от deadNightTiger

HTTP вручную? Почему не libcurl?

Что-то не подумал даже о libcurl. Да и ладно. Так сойдёт. Незнаю чем бы отличалось, потому как libcurl не использовал ниразу, но так я понел как передавать данные в post запросе ( раньше может и знал ), и что храниться устанавливается в куки, как получать и как использовать эту информацию.

u0atgKIRznY5 ☆
() автор топика
Ответ на: комментарий от u0atgKIRznY5

остановился на том, чтобы создать структуру, и её передавать как параметр.

Гениально! Как до этого никто не додумался раньше, все всегда использовали для этого глобальные переменные. Срочно патентуй!

anonymous
()
Ответ на: комментарий от ox55ff

Я помню были ошибки, и слали их в dmesg. Но там нет детализованной информации. А так можно плагин сменить на gtk обычное приложение и высматривать, может так сбоить будет, но так не хочеться.

u0atgKIRznY5 ☆
() автор топика
Ответ на: комментарий от u0atgKIRznY5

А так можно плагин сменить на gtk обычное приложение и высматривать

Отлично. Так что запусти valgrind, он тебе скажет где проблема. Ну или под отладчиком посмотри.

ox55ff ★★★★★
()
Ответ на: комментарий от anonymous

зато в случае суперглобалов экономится память и предикторы работают стопроцентно!

надо корочи выделить штук двадцать интовых переменных, и ещё парочку void* буферов константной длины, считать их «регистрами», и потом аккуратно в них всё хоронить между вызовами методов

вот это будет настоящее Ъ, автор станет героем

stevejobs ★★★★☆
()
Ответ на: комментарий от stevejobs

Общение с ассемблером не прошло для тебя бесследно.

ox55ff ★★★★★
()
Ответ на: комментарий от XMs

Разделил на функции. Щас остаётся ждать и смотреть, будет ли ошибка. Мне что не нравиться делить на файлы, так это то, что если в header я объявлю SSL *ssl, и в одном файле инициализирую, то будет ли без проблем работать эта же ssl в другом файле, или в другом файле будет объявлен новый ssl?

u0atgKIRznY5 ☆
() автор топика
Ответ на: комментарий от bormant

А точно. Вроде бы пока плагин на i686 не выключается.

u0atgKIRznY5 ☆
() автор топика
Ответ на: комментарий от XMs

Ты кстати пользуешься моей программой? Так удобно. Браузер закрыт. Иногда нужно только поглядывать на панель, есть ли новые сообщения.

u0atgKIRznY5 ☆
() автор топика

Щас проверил, число нужное получает, но видимо проблема в

gtk_label_set_text ( (GtkLabel *) cl->title, sp );
Она почему то не срабатывает.

u0atgKIRznY5 ☆
() автор топика
Ответ на: комментарий от u0atgKIRznY5

Это не срабатывает на i686, на 64 всё нормально.

u0atgKIRznY5 ☆
() автор топика
Ответ на: комментарий от u0atgKIRznY5

По сути у тебя теперь браузер не закрыт, а запускается каждые n раз. Браузер уведомлений. Лол.

anonymous
()
Ответ на: комментарий от u0atgKIRznY5

Ты кстати пользуешься моей программой?

Разумеется, нет, я же кедовод, к тому же очень сильно не люблю GTK.

Вопросы, на которые уже был дан ответ от других товарищей, пропущу, чтобы не дублировать

XMs ★★★★★
()

CFLAGS += -Wall -pedantic -std=c89
Юзай нормальные отступы, юзай что нибудь для http, не юзай sscanf. replace end/cleanup. Раздели же ты файл на части. Еще всякие #define и объявления в начало файла лучше.

linuhs_user ★
()
Ответ на: комментарий от linuhs_user

calloc было предложено использовать вместо malloc + memset (0. Зачем автор зануляет память для fgets - уже другой вопрос.

PPP328 ★★★★★
()
Ответ на: комментарий от PPP328

Зачем автор зануляет память для fgets - уже другой вопрос.

Я думаю что fgets получает буфер без завершающего нуля.

u0atgKIRznY5 ☆
() автор топика
Ответ на: комментарий от PPP328

calloc религия запрещает использовать?

Я так привык уже делать. Как то плотно изучал как работает программа reaver, что как делается, и там почему то было так.

malloc
memset
Потом я тоже начал так делать.

u0atgKIRznY5 ☆
() автор топика
Ответ на: комментарий от linuhs_user

а зачем тогда тут calloc нужен?

Я не знаю, зачем он делает memset сразу после malloc, но если делает, значит дальнейший код расчитывает на то, что там будут нули. А раз так, вместо вызова malloc+memset проще вызвать calloc. Потенциально, это может быть даже быстрее (чу-у-уточку), потому что как ты уже упоминал, calloc не всегда зануляет память. Но при чтении выделенного участка будут читаться нули, это гарантируется.

i-rinat ★★★★★
()
Ответ на: комментарий от PPP328

Ну так оно и не зануляет иногда.) Это вопрос к i-rinat был уже, к его логике)

linuhs_user ★
()
Ответ на: комментарий от i-rinat

вместо вызова malloc+memset проще вызвать calloc

Ну это как то странно... Первое дает понятный результат, malloc будет быстрее если убрать memset итд)

linuhs_user ★
()
Ответ на: комментарий от linuhs_user

malloc будет быстрее если убрать memset

В принципе зачем там memset не очень понятно. Разве что стат-анализаторы ругаются, что «память неинициализирована». Ладно вы ещё читал в буфер n-1, чтоб завершающий 0 стоял, так вроде такого не видно. sscanf ЕМНИП сам завершающий 0 ставит, а для пустой строки, вроде как, достаточно и просто в начало буфера 0 воткнуть.

ЗЫ. хотя, в принципе, там не громадный кусок memset-ить. Зануляет и пёс бы с ним, лишним не будет.

SkyMaverick ★★★★★
()
Ответ на: комментарий от u0atgKIRznY5

Я думаю что fgets получает буфер без завершающего нуля.

Надо не думать, надо читать мануалы:

A terminating null byte ('\0') is stored after the last character in the buffer.

PPP328 ★★★★★
()
Ответ на: комментарий от PPP328

Я к тому, что он там не делает, чего-то, что требует скорости и экономии на вызовах (условно говоря, fftw обогнать не пытается). Ну занулил и занулил (к тому же «копеечный» блок). Лишнее, ну и пёс бы с ним, надежней будет.

SkyMaverick ★★★★★
()
Ответ на: комментарий от SkyMaverick

Лишняя, вызываемая каждую секунду несколько раз функция. Это не инициализационный блок, это блок который будет вызываться каждую секунду в составе индикатора на панели.

PPP328 ★★★★★
()
Ответ на: комментарий от PPP328

Ну ок, пусть просто нуль втыкает, сэкономим лишние вызовы :)
Что на фоне ранее стоящих fopen/fclose и sleep-ов будет как-то не так, чтобы и сильно заметно.
ЗЫ. Я не утверждаю, что говоришь не правильно: memset тут действительно нафиг не впёрся, я просто заметил, что по сравнению с остальным рядом стоящим оверхедом memset-ы тут дают копейки (хотя тоже что-то да стоят, не без этого).

SkyMaverick ★★★★★
()
Ответ на: комментарий от SkyMaverick

Ну положим файл надо перечитывать, да, ибо не панель же перезапускать при изменениях. Но тогда надо его не раз в секунду читать, а вешать хук на нотайс. Sleep ресурсы не жрет.

PPP328 ★★★★★
()
Ответ на: комментарий от PPP328

Sleep-то не жрёт, но пресловутые memset-ы и всё иже с ними крутится реже, чем могло бы. А так согласен, да.

SkyMaverick ★★★★★
()
Ответ на: комментарий от PPP328

Это не инициализационный блок, это блок который будет вызываться каждую секунду в составе индикатора на панели.

Каждую секунду тащить главную с ЛОР? Наркоман?

crutch_master ★★★★★
()
Ответ на: комментарий от linuhs_user

Первое дает понятный результат

А второе — нет?

i-rinat ★★★★★
()
Вы не можете добавлять комментарии в эту тему: топик перемещен в архив.