Re: sasc re-write

["Simon 'corecode' Schubert" <corecode@xxxxxxxxxxxx>]

hey, just a few comments

On 16.01.2005, at 19:09, Liam J Foy wrote:

> @@ -27,137 +28,165 @@
>   * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>   * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE 
> USE OF
>   * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> - *
> + *

trailing whitespace addition

>   * $FreeBSD: src/usr.bin/sasc/sasc.c,v 1.7.2.1 2000/06/30 09:47:52 ps 
> Exp $
>   * $DragonFly: src/usr.bin/sasc/sasc.c,v 1.4 2005/01/05 00:34:36 
> cpressey Exp $
> + *

same here

>   */
>
> +#include <sys/file.h>
> +#include <sys/ioctl.h>
> +#include <sys/types.h>
> +
>  #include <err.h>
> -#include <stdlib.h>
> +#include <errno.h>
>  #include <stdio.h>
> +#include <stdlib.h>
>  #include <unistd.h>
> -#include <sys/file.h>
> -#include <sys/ioctl.h>
> +
>  #include <machine/asc_ioctl.h>

i'd move this up after sys/*

> -int
> -main(int argc, char **argv)
> +/* Check given numerical arguments */
> +static int
> +getnum(const char *str)
> +{
> +        long val;
> +        char *ep;
> +
> +        val = strtol(str, &ep, 10);
> +        if (errno)
> +                err(1, "strtol failed: %s", str);

you don't reset errno to 0, so this might be not working. reading the 
man page it might be needed :/

> +        if (str == ep || *ep != '\0')
> +                errx(1, "invalid value: %s", str);
> +
> +        return((int)val);
> +}
> +
> +static void
> +asc_set(int fd, u_long asc_setting, int asc_value)
>  {
> -  char c;
> -  int fd;
> +	if (ioctl(fd, asc_setting, &asc_value) < 0)
> +		err(1, "ioctl failed setting %d", asc_value);
> +
> +	printf("Successfully set\n");

you don't tell *what* got set :) this would be nice, but not directly 
neccessary.

how about this:

#define	asc_set(fd, key, val)		_asc_set(fd, key, val, #key)
static void
_asc_set(int fd, u_long asc_key, int asc_value, const char *asc_keyname)
{ ...

> +static int
> +asc_get(int fd, u_long asc_setting)
> +{
> +	int asc_value;
> +
> +	if (ioctl(fd, asc_setting, &asc_value) < 0)
> +		err(1, "ioctl failed", asc_value);

same here

cheers
  simon

--
/"\
\ /
 \     ASCII Ribbon Campaign
/ \  Against HTML Mail and News

Attachment: PGP.sig
Description: This is a digitally signed message part