opensand team mailing list archive

[Didier Barvaux <didier@xxxxxxxxxxx>]

Andy,

> Thanks for yesterday feedback. Lately I was working again on 32 bit
> edition of ubuntu 12.04 (this time desktop).
> I manage to perform source compilation for this platform (up to
> manager so far) and thanks to your previous suggestions I haven't
> crash my fresh VM once again. I'll describe all steps which are
> different to ubuntu 10.04 installation in my report.
> 
> Of course they are some warnings to report (please check attached log
> -> opensand-corep-wornings) and I've manage to reproduce scenario with
> $OPENSAND_BASE_DIR failure (please check ManagerCompilationError) but
> at the moment starting deamon is most important in my opinion.

Please provide a patch that fixes those errors.

To help you, I'll give some information about the first error (the one
you encounter in your attached log file):

  | sat_emulator_err.cpp: In function 'int SE_init_error_generator_from_file(char*)':
  | sat_emulator_err.cpp:139:8: error: variable 'lp_char' set but not used [-Werror=unused-but-set-variable]
  | cc1plus: all warnings being treated as errors
  | ...
  | in line 139 there is declaration of variable
  | it is in use in line 166 and 171 in the same fuction.
  | I'm a bit lost with that error/worning massage.

The lp_char variable is declared at line 139. It is affected a value at
lines 166 and 171. "affected" does not mean "used" for the compiler.

Think that "affected" = "write a value to the variable" and "used" =
"read the value stored in the variable".

So, we have the lp_char variable that is declared, then affected a
value twice, but never used (the values are never read). So, either
a/ the variable is useless and can be dropped, either b/ we forget to
use the variable to do something smart.

In this specific case, the variable is affected with the return values
of functions:
 | ...
 | lp_char = fgets((char *) &(lp_error_generator->name), 120, lp_f);
 | ...
 | lp_char = fgets((char *) &(lp_error_generator->desc), 498, lp_f);
 | ...

In general, not checking for the return values of functions is considered as
a bad behaviour. You may miss some processing error.

To be sure that we should check the return value of the fgets()
function, let's read the man page (especially its 'RETURN VALUE'
section):
 $ man fgets
 ...
 SYNOPSIS
 ...
       char *fgets(char *s, int size, FILE *stream);
 ...
 RETURN VALUE
 ...
       gets()  and  fgets() return s on success, and NULL on error or
       when end of file occurs while no characters have been read.
 ...

The man page reads that the function will return NULL in case of error
or end of file. So the current code is wrong by not checking for the
return value of the function. A failure will not be detected and make
the program breaks (segfault, strange runtime behaviour...). So the
compiler makes us a favor by reporting that error.

In conclusion the correction is to check the value:
  | ...
  | lp_char = fgets((char *) &(lp_error_generator->name), 120, lp_f);
  | if(lp_char == NULL)
  | {
  |    UTI_ERROR("%s failed to retrieve error generator name: "
  |              "malformed configuration file '%s'\n", FUNCNAME, ip_filename);
  |    return 0;
  | }
(similar thing for the 2nd call to fgets()).


As a side note, the SE_init_error_generator_from_file() function is a
collection of bad behaviours :(
 - fgets() is not recommended nowadays
 - several global variables are used
 - the return values of several called functions are not checked
 - the return value of the function itself is not checked by callers
 - UTI_INFO is used instead of UTI_ERROR
 - the code is written in C, not C++
However it is not your current job, so please fix only the errors
reported by the compiler.


If you need some help for particular errors, please ask us.


> Problem which I encounter this time is similar to that one which is
> present for ubuntu 10.04 (please check attached log - DaemonError).
> 
> I simply can't run properly network and daemon like I should
> according to installation manual.
> With ubuntu 10.04 I was able to run daemon through packages and
> reconfigure command.
> I presume that I can't do it with ubuntu 12.04 because packages are
> not compatible.
>
> I checked all bug reports (all 28) and I haven't found any bug
> description and its patch which might help. If I'm wrong, please
> correct me.
> 
> I'll be grateful for suggestions.


The big problem is that the sand-network service was written with DEB
packages in mind. It retrieves its config from package database. That's
wrong for portability reasons. I reported the bug some time ago:
https://bugs.launchpad.net/opensand/+bug/1042641 

If you configured the network manually, you do not need it. So,
creating the /var/lock/opensand-network file is a good workaround. The
real fix is however to rewrite the opensand-network init script.

You encounter a second problem with the opensand-daemon service. It
seems to fail at startup. By looking at the service script, you may
encounter several problems:
 - no directory for PID file,
 - bad permissions for PID file,
 - daemon stops/crashes at startup.

Please check that the /var/run/sand-daemon/ directory exists. Please
check that it is owned by the "opensand" user. If not, check that the
"opensand" user exists on your system. If everything's fine, check for
any traces that the daemon could have written in the log
files (/var/log/). An explanation of the failure should be present in
the logs.


Regards,
Didier

Attachment: signature.asc
Description: PGP signature