Sockets, envio e recebimento de arquivos

Eai galera? Beleza? Já faz um tempo que não apareço por aqui, tava meio parado com tudo mesmo. Então, meu problema é, estou dando uma estudada em sockets e estou no meio de um programa que tem as funções de enviar e receber arquivos implementadas. O código até funciona mais ou menos, mas perfeitamente só em algumas situações, em arquivos pequenos. Realmente busco por uma solução para "qualquer" tamanho de arquivo que funcione tanto nos posix quando no windows, que apesar de terem a mesma API (freebsd) em se tratando dos sockets, podem ter algumas peculiaridades no caso de arquivos.
Estive buscando a solução mais simples mesmo e aqui vai o código das duas funções que criei:

void upload(char *s, int sockfd){

	FILE *f;

	int n;

	f=fopen(s,"rb");

	char buff[1024];

	

	if(f==NULL){

	  perror("Read error");

	  send(sockfd, buff, sizeof buff, 0);

	  return;

	}

	printf("Uploading...    \n");

	do{

		memset(buff,0,sizeof buff);

		n=fread(buff, sizeof(char), 1024, f);

		if(send(sockfd, buff, n, 0)<0){

			perror("send error\n");

			return;

		}

		//recv(sockfd, buff, sizeof buff, 0);

		printf("|");

	  

	}while(n==1024*sizeof(char));

	fclose(f);

}



void download(char *s, int sockfd){

	FILE *f;

	f=fopen(s,"wb");

	int n;

	char buff[1024];

	

	if(f==NULL){

	  perror("write error");

	  return;

	}

	printf("Downloading file....\n");

	do{

	  memset(buff,0,sizeof buff);

	  if((n=recv(sockfd, buff, sizeof buff, 0))<0){

		  perror("read error\n");

		  return;

	  }

	  fwrite(buff, n, 1, f);

	  //send(sockfd, "go", sizeof "go", 0);

	  printf("|");

	}while(n>0);

	fclose(f);

  

}

 

Tem algumas coisas comentadas inúteis ai fiquei com preguiça de tirar :p. De qualquer forma, gostaria que alguém comentasse algumas implementações para que isso passasse a funcionar para arquivos grandes também. A ideia é que quanto a função download executa de um lado, a upload é executada do outro.(servidor-cliente), ou vice versa.

Afinal, o que tem a dizer?

ps: Conheço a biblioteca sendfile.h, mas não quero usá-la já que pretendo que o código seja compilável para windows.

Os problemas são análogos nas funções de upload e download, então vou comentar uma vez só, com comentários que podem se aplicar a diversos lugares:

    - Funções como essas, que se assemelham a funções de biblioteca, que serão usadas por outros programas, geralmente não devem imprimir mensagens de erro por conta própria, pois seus eventuais usuários podem preferir outro tipo de interface com o usuário (por exemplo, interface gráfica ou sinalização por código de terminação do programa). Possivelmente seria melhor você mudar o tipo de retorno das funções de void para int (ou bool), com um valor de retorno para indicar erro, e outro para indicar sucesso. O tipo de erro poderia ser informado através da variável errno (até porque você usa internamente funções que podem ser causadoras de erro, e tais funções já usam errno para informar sobre seus erros).

    - Em vários lugares você usou a expressão “sizeof(char)”. Isso não chega a ser um erro, mas acaba sendo um esforço desnecessário, porque, por definição, sizeof(char) é sempre igual a 1.

    - Por outro lado, junto a algumas das ocorrências de “sizeof(char)”, você usa uma constante inteira 1024, que é o número de elementos do array buff. O problema de espalhar essas constantes pelo código é que dificulta a manutenção. Digamos que você tenha chegado à conclusão de que um buffer de 8192 bytes aumenta a eficiência da comunicação. Quantos pontos do programa você terá de alterar? Será que todas as ocorrências da constante podem ser trocadas ao mesmo tempo, ou só algumas se aplicam à variável buff? Não seria interessante achar um meio de concentrar essas alterações num ponto só do programa (preferencialmente na declaração da variável)?

    - Os dois problemas anteriores são devidamente tratados do seguinte modo: em todo lugar em que for importante saber o tamanho de cada elemento (por exemplo, no segundo argumento de fread() e fwrite()), você pode usar a expressão “sizeof buff[0]”. E sempre que você precisar saber o número de elementos num array (como no terceiro argumento de fread() e fwrite()), basta dividir o tamanho total do array pelo tamanho de cada elemento, ou seja: “sizeof buff/sizeof buff[0]”. Essa estratégia funciona para qualquer array, com qualquer tipo de elemento e qualquer dimensão.

    - No caso particular em que você garanta que o tipo do elemento é char, a expressão “sizeof buff[0]” pode ser substituída, com segurança, pelo valor 1.

    - Por outro lado, recv() e send() trabalham com tamanho total em bytes. Se você algum dia optar por arrays com tipos de elementso que não sejam char, não pode esquecer de multiplicar o número de elementos pelo tamanho de cada elemento.

    - Você coloca algumas declarações de variáveis após comandos (em ambas as funções, há declarações depois do comando usado para abrir o arquivo). Em C++ ou no C após o padrão de 1999, isso não é errado. Contudo, isso era considerado um erro no padrão do C de 1989/1990, que ainda é usado por default por muitos compiladores. Então, considere colocar primeiro todas as declarações, e depois disso você começa a colocar os comandos.

    - Além do mais, logo após abrir os arquivos, a operação seguinte é testar se a abertura foi bem sucedida. Por que interromper visualmente essa sequência lógica com declarações de outras variáveis, que nada têm a ver com a abertura do arquivo?

    - Quando você testa o erro de abertura e constata que a abertura realmente falhou, as mensagens de erro que você imprime não refletem o problema que de fato ocorreu. Você indica erro de leitura (ou escrita), quando, na verdade, o erro foi de obtenção de recurso (abertura do arquivo). As operações de leitura/escrita podem falhar, sim, mas só depois de o arquivo já ter sido aberto com sucesso. (De todo modo, considere não imprimir nada; como dito no primeiro comentário, sinalize o erro para quem chamou a função, e deixe que essa pessoa escolha o tratamento a ser dado.)

    - Em todas as vezes que você usou memset(), fê-lo inutilmente, pois imediatamente sobrescreve a memória que acabou de limpar com outro conteúdo. Simplesmente remova tais chamadas.

    - Nos dois laços de repetição, eu usaria while em lugar de do-while pois, em caso de arquivo de leitura/recepção vazios (ou com erro), não provocaria uma tentativa de envio/gravação com tamanho zero. Não que isso constitua erro, mas pode trazer um pequeno ganho de eficiência.

while((n=fread(buff, 1, sizeof buff, f))>0){

  /* Envia conteúdo lido. */

}

if(ferror(f)){

  /*

    Loop foi interrompido por erro, não pelo fim do arquivo.

    É bom sinalizar isso.

  */

} 

while((n=recv(sockfd, buff, sizeof buff, 0))>0){

  /* Grava conteúdo recebido. */

}

if(n==-1){

  /*

    Loop foi interrompido por erro de recepção, não pelo fim normal dos dados.

    É bom sinalizar isso.

  */

} 

    - Você não trata o possível caso de erro de gravação dos dados recebidos da rede no arquivo local. Provavelmente deveria fazê-lo.

    - Aparentemente você trocou a ordem do segundo e do terceiro argumentos na chamada a fwrite().

    - Considere (note que eu não estou recomendando que use, mas que analise para ver se é conveniente) usar MSG_WAITALL como opção (quarto argumento) de recv().

    - Suprima o código comentado que você mesmo disse que é inútil. Ele pode acabar confundindo quem lê seu programa.

Sei como é, também fiquei um tempo parado (sem estudar C), mas ai é só voltar a estudar que você se interessa e aprende de novo :)
Engraçado ontem eu fui dormir pensando em uma função parecida... HEHE

Leia atentamente a resposta do paulo1205, pois é muito bem detalhada e comentada sobre alguns aspectos estranhos na escrita do teu code.

Eu consideraria postar o code inteiro, incluindo a função main() e outras (se houver), para uma melhor resposta. Mas lendo essas funções, eu reparei nisso:

	if(f==NULL){ //"if((f=fopen(s,"rb"))==NULL)"

	  perror("Read error"); //"fprintf(stderr,"%s\n",strerror(errno));" "#include <errno.h>

	  send(sockfd, buff, sizeof buff, 0);

	  return; //"return EXIT_FAILURE;" ou "exit(1);"

	}

	else{...} 

Aí "se nulo" aborta ou da exit e nem continua a execução do restante da função, ao invés de apenas infomar o erro com "perror()"

Na manipulação de arquivos eu prefiro informar os erros com fprintf() imprimindo em stderr o valor de errno setado com strerror().
Enquanto "perror()" eu uso para detalhar erros diretamente, como nas funções de sockets.

Enquanto ao upload/download eu optaria por checar os arquivos e tratar os possíveis erros antes de, de fato continuar com a função de upload/download.
Eu também consideraria usar apenas while() em recv() e send() como o paulo1205 sugeriu.