Re: sasc re-write

[Liam J Foy <liamfoy@xxxxxxxxxxxxx>]

On Mon(17)/Jan/05 - , Simon 'corecode' Schubert wrote:
> 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

Fixed

> 
> >  * $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

Fixed

> 
> >  */
> >
> >+#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/*

Done

> 
> >-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 :/

Agreed. I believe we had a discussion ;) - fixed

> 
> >+        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

Well, I think the fact we show what value is trying to be
set is enough. The user would know what he/she was trying to
set via what value they put it.

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



-- 
- Liam J. Foy
<liamfoy@xxxxxxxxxxxxx>