Brian Cameron wrote: > > Robert: > >>> Thanks for the patch. Could you explain a few things about this patch? >>> You said you created a bug for this in bugzilla.gnome.org in the gdm >>> category, but I don't see a bug there. Would you mind adding a bug >>> and attaching the patch? > > >> I already submitted a bug report on opensolaris.org but apparently >> they don't show up immediately. I've learned from other Sun folks >> that the bug id # is 6626891. I didn't see any way to attach the >> patch to the bug report when I submitted it. > > I was hoping you could also file a bug at bugzilla.gnome.org in the > "gdm" category. Since this is a general GDM issue, not a Sun specific > one. > While I agree that the patch makes the code "more correct" in the general sense, the problem only occurs on Solaris not on Linux. The glibc versions of getnameinfo(), getaddrinfo() and sendto (called by XdmcpFlush) only check that the sockaddr is >= to the expected length. Solaris on the other hand checks to see that the length is == to the expected length.
So in a sense this is a Solaris specific problem. >>> First, could you explain what specific error(s) you were seeing that >>> this patch fixes? >>> >> The error is that xdmcp doesn't work. > > I gathered, but I was hoping you could explain how the failure manifests > itself. Do you click on a client in the chooser and it just fails to > connect? Do you see any errors? This sort of information is useful > so that if others report the same sort of problem, then I will have > a better idea if it really is the same problem. > The calls to XdmcpFlush fail so no packets are sent (locally or remotely). This means that xdmcp doesn't work at all. The host doesn't appear in any chooser, remote or local. >>> The change to better calculate the address size makes sense to me. >>> >>> I notice in misc.c that you are no longer setting hints.ai_family to >>> AF_INET or AF_INET6. Why is this change needed? >>> >> The getaddrinfo() api was failing (I don't have ipv6 configured on >> the interface). The code made no sense since ai_family is not a >> bitmask. > > Okay, this makes sense to me. Thanks for explaining. > >>> Also, I notice you removed NI_NUMERICHOST | NI_NUMERICSERV from >>> getnameinfo in common/gdm-common.c. I assume this change is needed >>> so we allow DNS/NIS to be used? Is that correct? >>> >> NI_NUMERICHOST | NI_NUMERICSERV forces the result to be returned in >> numeric form which defeats the whole point of what the function is >> supposed to return based on its name. If that is really what was >> intended then inet_ntop() should be called directly. Without those >> flags the hostname is returned unless it is unavailable in which case >> the numeric representation is returned. This routine is primarily >> used to obtain a human-friendly name for logging purposes. > > I'm still a bit uncomfortable with this change. Looking at how > gdm_address_peek_local_list is used, I don't see it is only used for > logging purposes. I see it is used in the following ways: > You are confusing the change to misc.c with the one to gdm-common.c. The removal of NI_NUMERICHOST and NI_NUMERICSERV is in the routine gdm_address_get_info() not gdm_address_peek_local_list(). However, in reviewing all the calls to gdm_address_get_info() there are 2 places out of 31 where the numeric version is actually wanted. I updated the patch on the ftp site to remove that change. I originally made the change to simplify debugging the xdmcp problem. Since it is primarily cosmetic, I'll redo the change and fix the other locations that really did want the numeric address and submit it directly to gnome.org. Thanks for the feedback. > 1) In auth.c in get_local_auths to add auth entries for each address > that corresponds to the local machine. > 2) In gdm-xdmcp-manager.c to find the loopback address. > 3) In misc.c in gdm_address_is_local to see if two addresses are equal. > > So, it doesn't seem that this is only used for logging purposes, unless > I'm misreading the code. And some of these use cases seem to be wanting > to know things like "is this the local machine". I'd think you would > really want the numeric representation here since the same IP address > can have different DNS entries, for example. > > Is this change really needed to fix your xdmcp not working problem? If > not, I think I would prefer to leave this bit of code alone. Or perhaps > to rewrite the function to just call inet_ntop() rather than doing the > existing code (if you have the interest). > > By the way, I am the GDM maintainer, so if it seems like I'm being a bit > picky, I just want to get these changes right since I'd like to get > these fixes upstream. > Your feedback has been helpful and not the least bit pedantic. > Thoughts? > > >>> I'll work to get this change upstream. Thanks for the help. >>> >>> Thanks, >>> >>> Brian >>> >>>> I found the problem(s) with gdm and xdmcp. >>>> >>>> I uploaded a patch to ftp://ftp.the-nelsons.org/pub. >>>> >>>> If you don't want to rebuild SUNWgnome-display-mgr then you can >>>> download the prebuilt package from there as well. >>>> >>>> >>>> This message posted from opensolaris.org >>>> _______________________________________________ >>>> desktop-discuss mailing list >>>> desktop-discuss at opensolaris.org >>> >> >> >
