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 ★★★★★
()
Вы не можете добавлять комментарии в эту тему. Тема перемещена в архив.