On Sep 2, 2011, at 2:04 PM, Dave Miner wrote: > On 09/02/11 13:34, Harold Shaw wrote: >> Dave, >> The code here is attempting to create the ISC DHCP subnet stanza from >> the existing Solaris DHCP configuration. It may be possible to get a >> range of addresses from 'pntadn -P <network>' of >> 192.168.12.240-192.168.13.32. Instead of generating a single range I >> chose to generate it as 2 ranges 192.168.12.240 - 192.168.12.254 and >> 192.168.13.0 - 192.168.13.32. It was easier to do and functionally >> equivalent. I can make the change necessary to generate this as a single >> range if desired. >> > > So the generated configuration has multiple ranges rather than a single one? > That seems sub-optimal to me. I at least understand why the code is correct, > anyway.
FWIW, I would tend to agree here that we should have one entry for the range. While functionally equivalent, it just seems a bit confusing (and wrong-seeming) to work with the tuples as meaningful network/client delineation bounds. That said, I also see that it will indeed be functionally equivalent, so if this is the best way to do it for now, I'm not entirely opposed to it. But, like I said yesterday, it may lead to confusion. The comment certainly adds clarity, though, so I'm Ok with it. /jb > > Dave > >> Harold >> >> On 09/02/11 10:26, Dave Miner wrote: >>> Harold, I don't follow how "treating addresses as Class C" is at all >>> correct here. Can you elaborate on why that's the right thing to do >>> when networks of that particular size are increasingly uncommon? >>> >>> Dave >>> >>> On 09/01/11 17:14, Harold Shaw wrote: >>>> I have made the changes discussed below and reposted the webrev: >>>> >>>> https://cr.opensolaris.org/action/browse/caiman/hshaw/7082285/webrev/ >>>> >>>> Thanks, >>>> Harold >>>> >>>> On 09/01/11 02:24, Darren Kenny wrote: >>>>> Hi Harold, >>>>> >>>>> It took me a few goes to understand what exactly this code was doing - >>>>> which >>>>> makes me think that it needs some comments to explain it a little more. >>>>> >>>>> Looking at the code, it looks like there is a possible hole in the >>>>> logic if >>>>> somehow you got addresses like: >>>>> >>>>> 192.168.1.1 >>>>> 192.168.1.2 >>>>> 192.168.2.3<- Note the subnet also changes. >>>>> >>>>> This wouldn't see this as a new range, but would think it's part of the >>>>> currently processed range since the expression: >>>>> >>>>> int(client_ip_tuple) != int(last_ip_tuple) + 1: >>>>> >>>>> would evaluate to False. >>>>> >>>>> Is this possible to occur in reality? >>>> I believe that it can. I have made the changes necessary to account for >>>> this. >>>>> If so, maybe you need two groups in the RE - one for the subnet and >>>>> one for the >>>>> last digit (why do you call this a tuple - it's confusing I think) - >>>>> and then >>>>> compare the subnets too in the expression at line 164. >>>> I have refactored the code and renamed the variables so that it is, >>>> hopefully more understandable. Let me know if it addresses your >>>> concerns. >>>>> Thanks, >>>>> >>>>> Darren. >>>>> >>>>> On 31/08/2011 19:16, Harold Shaw wrote: >>>>>> Can I get another reviewer for this? >>>>>> >>>>>> Thanks, >>>>>> Harold >>>>>> >>>>>> On 08/31/11 08:33, Harold Shaw wrote: >>>>>>> Thanks, William. >>>>>>> >>>>>>> Harold >>>>>>> >>>>>>> On 08/31/11 07:08, William Schumann wrote: >>>>>>>> Harold, >>>>>>>> This fix looks fine to me. >>>>>>>> William >>>>>>>> >>>>>>>> On 8/30/2011 11:36 PM, Harold Shaw wrote: >>>>>>>>> Can I get a couple of reviews of the changes for: >>>>>>>>> 7082285<http://monaco.us.oracle.com/detail.jsf?cr=7082285> >>>>>>>>> /usr/sbin/installadm-convert dumps traceback with IndexError code >>>>>>>>> >>>>>>>>> The webrev is located at: >>>>>>>>> >>>>>>>>> https://cr.opensolaris.org/action/browse/caiman/hshaw/7082285/webrev/ >>>>>>>>> >>>>>>>>> >>>>>>>>> To verify the fix the following DHCP configurations were run: >>>>>>>>> - Single IP address >>>>>>>>> - Single IP range >>>>>>>>> - Mixture of single IP addresses and IP ranges >>>>>>>>> >>>>>>>>> Thanks, >>>>>>>>> Harold >>>>>>>>> >>>>>>>>> >>>>>>>>> _______________________________________________ >>>>>>>>> caiman-discuss mailing list >>>>>>>>> [email protected] >>>>>>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>>>>> >>>>>>> _______________________________________________ >>>>>>> caiman-discuss mailing list >>>>>>> [email protected] >>>>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>>>> >>>>>> >>>>>> _______________________________________________ >>>>>> caiman-discuss mailing list >>>>>> [email protected] >>>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>> >>>> _______________________________________________ >>>> caiman-discuss mailing list >>>> [email protected] >>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>> >> > > _______________________________________________ > caiman-discuss mailing list > [email protected] > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss _______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
