On Thu, Jan 15, 2009 at 10:19:58AM +0000, Daniel P. Berrange wrote:
> > +#ifdef __sun
> > + {
> > + ucred_t *ucred = NULL;
> > + const priv_set_t *privs;
> > +
> > + if (getpeerucred (fd, &ucred) == -1 ||
> > + (privs = ucred_getprivset (ucred, PRIV_EFFECTIVE)) == NULL) {
> > + if (ucred != NULL)
> > + ucred_free (ucred);
> > + close (fd);
> > + return -1;
> > + }
> > +
> > + if (!priv_ismember (privs, PRIV_VIRT_MANAGE)) {
> > + ucred_free (ucred);
> > + close (fd);
> > + return -1;
> > + }
> > +
> > + ucred_free (ucred);
>
> Can move the ucred_free up before priv_ismember() call and thus
> avoid the need for the call in the cleanup path.
Nope, privs points into the ucred structure.
> Is the chmod of the socket really required for solaris ? We already
I'll double check these.
> Also, if this libvirtd is running as UID 60, so the chown really
> needed ? We also setgid before creating the socket so that it
> gets desired group ownership at time of creation, rather than
> having to change it post-create.
At this point in the code (I think) we're still root.
> This would appear to make it try to change the socket ownership
> and permissions, before we've actually created the socket, which
> is much later in the main() method where we call NetworkInit
Hmm, let me re-check.
> > +#ifdef __sun
> > + /*
> > + * On Solaris, all clients are forced to go via virtd. As a result,
> > + * virtd must indicate it really does want to connect to the
> > + * hypervisor.
> > + */
> > + name = "xen:///";
> > +#endif
>
> This should not be neccessary if the client end + drivers are
> correctly written.
Can you explain a bit more? Why don't we need to rewrite the URI as xen?
> If you want Xen to always go via the demon, the only change that should
> be required, is to make xenUnifiedOpen() return VIR_DRV_OPEN_DECLINED.
Hmm, yes you might be right. Let me experiment.
> > if (err == 0) {
> > - error (in_open ? NULL : conn,
> > - VIR_ERR_RPC, _("socket closed unexpectedly"));
> > + DEBUG("conn %p: socket closed unexpectedly", conn);
> > return -1;
> > }
> > }
>
> These two I/O methods here have been completely re-written in my
> thread patches. Why is removing the error messages required ?
If we try to connect w/o privilege, then the socket is closed straight
after accept() - so it's not longer an RPC error for this to happen.
> > + /*
> > + * If the connection over a socket failed abruptly, it's probably
> > + * due to not having the right privileges.
> > + */
> > + if (sigpipe)
> > + vshError(ctl, TRUE, _("failed to connect (insufficient privileges?)"));
> > +
>
> It will also be seen if the daemon drops the connection due to an
> OOM condition, or the max-clients limit being exceeded, so perhaps
> a little more detailed message.
Suggestions?
thanks
john
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list