Re: [Libvir] Remote patch, 2007-02-28

Mark McLoughlin wrote: Morning Mark, thanks for taking the time to look at the patch in detail. Do you have some actual concrete problems with SunRPC? For me it solves the problem of marshalling complicated data structures, including all the error infrastructure, over the wire (see src/remote_rpc.x). It is very trim and efficient. It demonstrably allows us to run over TLS, Unix domain sockets, and ssh. It handles versioning for us. On the other hand, coding with it is grotty to say the least. But we definitely shouldn't publish the SunRPC interface or allow others to interoperate with it, so that we can keep our options open in future. Some more comments and questions below. Because SunRPC code has a pleasant tendancy to write messages to stderr. It's required because the SunRPC client needs semi-random IDs for outgoing messages. See src/sunrpc/create_xid.c for the details. Yes, you're right. I'll fix this in the next iteration. Ah indeed. I _used_ to use it, but then removed that code. I'll kill this in the next iteration, although IIRC you'll be wanting /dev/urandom for something? I'm quite sure that git could do this, but my level of git knowledge is pretty small at the moment, so I'm just using it as a kind of "local CVS". I did supply some patches separately yesterday, so if those go in to the main libvirt tree then they will disappear from future releases of the remote patch. That's the way SunRPC (in glibc at least) works. Yes, you can be sure it's never NULL in this instance. Indeed. Unless the -d (debug) option is passed on the command line in which case it goes to stderr. Again, that's the way SunRPC servers work. There is no concurrency problem here, because SunRPC servers don't run concurrently (at least not the ones you can create using glibc). If we want concurrency, then we prefork. This is a bug. ATTRIBUTE_UNUSED isn't in the collection of headers I'm using, but needs to be added. I'll get this fixed in the next rev. Yes, not sure how to do this right. Since the maxids comes from an external process, but leads to an array being allocated inside libvirtd, there is a very definite route to a DoS attack if the maxids parameter is allowed to be very large. But how large is large? Um .. yes, that'll be a bug then. Well spotted. A check is needed here - this is a bug. As an aside, I only ever claimed we shouldn't check for malloc when the only good thing to do is to fail. In long running daemons or any other security-related context that's not the case because it's a DoS attack. In any case I really need to find out what the right way to allocate these structures is (hence XXX comment, etc.) I'll try and get this right in the next iter. Supposedly clnt_freeres (called by the stub code in SunRPC) should free the allocation, but that's what I need to work out. Can you explain more? Ditto. There's now a 1-1 relationship between virConnectPtr and the underlying TCP connection. Is that what you meant? This comes out of Dan's email about cookies (cookies have been removed completely from the code in the meantime). http://www.faqs.org/faqs/C-faq/faq/ (section 5.2) '0' in a pointer context is converted to the representation of NULL (even if the representation of NULL on some wierd architecture is not zero bits). Or do you mean stylistically you prefer NULL? [realloc] I'll take a look at both and use the one which is easier to understand/shorter. (God, I really hate reimplementing fundamentals :-) See next iteration. Yes, that's a bug. I think my thinking was that there was no place to log the error, but in fact that was wrong. This really is a "should never happen" place. If it does happen, I want to know about it. What's the right way to report that? I worry that logging an ordinary error will mean it's just ignored. Yup, bug, will investigate and fix. This is during start-up. One thing I _really_ dislike about bind9 is that errors in the zone files don't cause the thing to fail to start up. Instead you have to manually check /var/log/messages to see if it found any problems. If you don't check then your website disappears off the map 12 hours later. Thus, I like daemons that fail when the configuration file is broken. I wasn't planning on it. Note that svc_run never returns, so libvirtd can only ever exit on a signal. If my OS is leaking memory allocations, then I've got a really big problem that free isn't going to solve. In this case, the previous value is always a static string. In nfsd, if /etc/exports is empty, then nfsd doesn't start up. So the parallel is that if /etc/libvirtd.conf contains directives disabling every service, then it is not a bug to just exit. I was imagining that the wrapper script would clean up pid files if the process doesn't start up correctly. I wouldn't rely on the process to do this - what happens if it segfaults for instance? In the filesystem, yes. I'll take a look at this. There are actually other places where the code could die on SIGPIPE -- possibly on client going away unexpectedly. Cut'n'paste job from the gnutls example code. There's a Steve McConnell study where he shows that function length doesn't affect code comprehension. I'll dig it up. As an opaque data pointer? Yes, that's much better abstracted. I'll fix that in the next release. Thanks again for looking at this. Rich.