Threads: Ist der Code so in Ordnung?

Tentone

Hallo und guten Abend an alle Hacker

Man lernt immer mehr aber nie aus :).
Ich habe mich jetzt in das Thema Threads usw. eingelesen und probiert ein erstes eigen geschriebenes Exemplar zu vertigen. Mir ist es zwar etwas peinlich den Code darzustellen ,
aber was solls nur so kann man hilfe bekommen.

Ich will eigentlich nur wissen:
Ob dieser Code in ordnung ist, oder ob das ein "Zufallsprodukt" ist. Wenn er nicht gut sein sollte, dann bitte kurze Anmerkungen, damit ich mich verbessern kann.

Das Programm besteht aus zwei Threads "Lesen" und "Schreiben". Diese Threads werden über Flags synchronisiert.

#include <stdio.h>
#include <stdlib.h>
#include <time.h>
#include <iostream>
#include <windows.h>
#include <winsock.h>
#include <time.h>
#include "flag.h"

#define BLOCK 60000

typedef struct FLAG
{
	FILE *file_w, *file_r;
	int flag;
	int data_read;
	int data_write;
	int buffer[BLOCK];
	int data[2];
};

using namespace std;
CRITICAL_SECTION cs; 

DWORD WINAPI Read_File(FLAG *check_flag)
{
	int daten;
	for(;;)
	{
		EnterCriticalSection(&cs);
		if(check_flag->flag!=UNDEFINED&&check_flag->data_read==0&&check_flag->flag!=FINISHED)
		{

			if (check_flag->flag!=BLOCK_READ&&check_flag->flag!=FINISHED)
			{
				daten=fgetc(check_flag->file_r);
				check_flag->data_read=0;
				check_flag->data[0]=check_flag->data[0]+1;
				check_flag->buffer[check_flag->data[0]]=daten;
			}
			if (daten==EOF)
			{
				check_flag->flag=FINISHED;
			}

			if (check_flag->data[0]>=BLOCK||check_flag->flag==FINISHED)
			{
				if(check_flag->flag!=FINISHED)
				{
					check_flag->flag=BLOCK_READ;
				}
					check_flag->data[1]=0;
					check_flag->data_read=1;
					check_flag->data_write=0;

			}
		}

		LeaveCriticalSection(&cs);
	}

}

DWORD WINAPI Write_File(FLAG *check_flag)
{
	int daten=0;

	for(;;)
	{
		EnterCriticalSection(&cs);
		if(check_flag->flag!=UNDEFINED&&check_flag->data_write==0)
		{

			if (check_flag->flag!=BLOCK_WRITE)
			{
				daten=fputc(check_flag->buffer[check_flag->data[1]],check_flag->file_w);
				check_flag->data[1]=check_flag->data[1]+1;
				check_flag->data_write=0;
			}
			if (check_flag->data[1]==BLOCK||check_flag->buffer[check_flag->data[1]]==EOF)
			{
				if (check_flag->flag!=FINISHED)
					check_flag->flag=BLOCK_WRITE;
				check_flag->data[0]=0;
				check_flag->data_write=1;
				check_flag->data_read=0;
			}

		}
		LeaveCriticalSection(&cs);

	}

}

int main()
{
	FLAG *error_flag = (FLAG*)malloc(sizeof(FLAG)); 
	DWORD   ThreadIdArray[2];
	HANDLE  hThreadArray[2]; 
	error_flag->data[0]=0;
	error_flag->data[1]=0;
	error_flag->data_read=0;
	error_flag->data_write=0;

	InitializeCriticalSection(&cs);
	hThreadArray[0]=CreateThread(NULL, 0, (LPTHREAD_START_ROUTINE)Read_File, error_flag, 0, &ThreadIdArray[0]);
	hThreadArray[1]=CreateThread(NULL, 0, (LPTHREAD_START_ROUTINE)Write_File, error_flag, 0, &ThreadIdArray[1]);
	error_flag->flag=UNDEFINED;
	error_flag->file_r=fopen("test.txt","r");
	error_flag->file_w=fopen("ziel.txt","w+");
	printf("Hier die Read_ID: %d und Write_ID: %d\n", (int)hThreadArray[0], (int)hThreadArray[1]);
	error_flag->flag=BLOCK_WRITE;
	for(;;)
	{
		EnterCriticalSection(&cs);
		if(error_flag->flag==FINISHED && error_flag->data_write==1)
		{
			error_flag->data[0]=0;
			error_flag->data[1]=0;
			exit(-1);
		}
		/* printf("Die Flagge:%d\n", error_flag->flag); */

		LeaveCriticalSection(&cs);
	}
	return 1;
}

Danke für euer Bemühen und grüße aus dem abendlichen Bayerb
Tentone

Was soll das sein? Willst du mit einem Thread ein File auslesen und mit dem nächsten die Daten in ein anderes File schreiben? Da sind Threads ziemlich sinnlos, weil da ein CPU schon 1000 mal schneller ist als deine Platte.

Und wieso inkludierst du <iostream> in ein C-Programm?

BorisDieKlinge

Was soll das sein? Willst du mit einem Thread ein File auslesen und mit dem nächsten die Daten in ein anderes File schreiben? Da sind Threads ziemlich sinnlos, weil da ein CPU schon 1000 mal schneller ist als deine Platte.

naja vll. wollte der OP nur bischen thread syncronisierung üben.. wenn nicht, sollte er die thread für diesen problemfall weg lassen;)

und er sollte sich mal Events bzw. WaitForSingleObjects.. etc.

Tentone

Hallo Danke für eure Antworten,

und ich wollte nur mit Threads üben. Nur wenn ich was von vornherein falsch mache wird das größere Projekt nichts.
Das kleine (sinnlose) Programm ist noch übersichtlich und man kann noch ohen Aufwand fehler und verbesserungen machen und erkennen.
Das wird bei meinem größeren Projekt nicht mehr der Fall sein.
Da laufen schon drei Threads im Hintergrund:
Ein thread horcht auf einen Port ob ein File über Ethernet geschickt wurde oder nicht.
Sobald das File geschcitk wird, wird es abgespeichert dann läuft (sobald Daten angekommen sind) ein Viterbidekodierer und schickt die Daten weiter.

Bin auch kein Informatiker sondern Ingenieur, deswegen brauch ich ja eure Hilfe ... Nur ohne Programmierung geht heute leider nichts mehr.

Danke und Grüße
Tentone

BorisDieKlinge

wie gesagt, schau dir mal SetEvent, ResetEvent, WaitforSingleObject etc. an.. damit solltest du syncronisieren..

pumuckl

Wie schon oben angedeutet wurde solltest du dich vor allem erstmal entscheiden, ob du C oder C++ programmieren willst. Du hast da ein #include <iostream> und ein using namespace std; was beides C++ ist - nur benutzt du im ganzen Code nicht aus dem iostream header und auch ncihts aus dem namespace std. auf der anderen Seite bindest du header ein, die in C++ veraltet sind - stdio.h, stdlib.h und time.h (letztere hast du übrigens zweimal eingebunden, warum?) heißen in C++ <cstdio>, <cstdlib> und <ctime>, wobei stdio wiederum die C-IO-Routinen anbietet, was sich mit <iostream> erst recht beißt.

Ich weiß, in erster Linie gehts dir nur um die Threads, aber imho ist gerade für Threads ein sauberes Programmieren wichtig, und ein lustiger Mix aus C und C++ mit unmotivierten Includes und Using-Direktiven ist nicht wirklich sauber, konsistent und logisch. Deshalb halte ich es für wichtig, solche Dinge anzugehen bevor man sich um die Feinheiten bei Threads kümmert.