Re: [libvirt] [PATCH v2] Add helper program to create custom leases

On 01/27/2014 01:47 AM, Nehal J Wani wrote: In JSON, this is probably better represented as an int rather than string. JSON is pretty flexible as a key/value lookup; omitting a key rather than setting it to a sentinel value of '*' actually makes the parser easier (it's easier to ask "is the key present" than it is "lookup the key, now strcmp with "*" to see if the lease file didn't have anything to tell us). Everything else as string makes sense. We can drop the endif/if on the same condition, and just make the existing block longer. Then again, I'm trying to figure out when the leases helper should be build. Is it always built for exactly the same conditions that libvirtd is built? I think we are pretty safe to say that only libvirtd will be registering this helper binary with dnsmasq. On the other hand, I think it is possible to build libvirtd without qemu support, or more generically, in a setup where we will not be using dnsmasq. Thinking aloud, maybe the real condition should be: if WITH_LIBVIRTD existing iohelper stuff if WITH_DNSMASQ leasehelper stuff end WITH_DNSMASQ end WITH_LIBVIRTD which also would require a configure.ac setting of an automake conditional that says whether we are compiling with dnsmasq support. Looking at existing configure.ac, we merely check for a path to dnsmasq, and assume that we either learn the absolute path to use at configure time, or that it will be found on PATH at runtime, without needing to configure it, so on that front, your use of WITH_LIBVIRTD may be sufficient. It's okay to omit this if we don't have anything to list. s/cutom/custom/ So this hooks up the helper, but nothing is parsing the result yet? That's still okay for an incremental patch, but it would be nice to also see the consumer code. It's now 2014, and while attributing Red Hat copyright for the portions of this file that you copied from elsewhere, you're also entitled to claim copyright for your new file if you'd like. I like spelling this (2 * 1024 * 1024), as that's a bit more obvious that it is 2M. Why not reuse our existing NULLSTR macro? Style: leading { of a function body on its own line. Why not also reject argc > 4? I am also a strong believer that EVERY app should support --help and --version (even if only to say that the app is useless on its own). iohelper.c at least does: if (argc > 1 && STREQ(argv[1], "--help")) usage(EXIT_SUCCESS); but doesn't support --version, so you can argue that libvirt isn't consistent on always doing this, but it's still nicer to not propagate the bad examples. -1 gets truncated to a return value of 255, but that is generally a bad idea. I'd much rather see 'return EXIT_FAILURE' or 'exit(EXIT_FAILURE)'. Also, your error message is not translated. I guess we can argue that since the helper app is only ever useful internally, then hard-coding it to the C locale doesn't hurt. But even iohelper.c took care to call setlocale()/bindtextdomain()/textdomain() before printing any error messages, which were marked for translation. Although we require a C99 compiler, and therefore allow declarations after statements, we tend to avoid them; you should swap this block to occur before your check for proper argc. By storing "*" in iaid now, you've lost whether it was not provided in the environment. I'd rather see the output modified to omit keys that were not provided, but for that to work, you want to leave iaid NULL if it was not found in the environment. Uninitialized... Ah, you DO set up translations. You'll need to sink your argc check below here, and mark that string for translation. You'll also need to separate the declaration of ip from the actual assigning it from argv[3], so that you aren't dereferencing beyond array bounds. ...and on error, the cleanup label unconditionally frees lease_new and friends. That's a crash waiting to happen (the chance of being out of memory this early after startup is remote, but it will probably trigger Coverity warnings if you don't fix it). Is it worth writing a diagnostic to stderr before cleaning up? I know it is dnsmasq and not libvirt that calls this app, so I don't know whether error messages printed here will be propagated back to libvirt, but it's probably worth trying to be verbose about any error exit. For that matter, you may want to copy what tools/virt-login-shell.c does: virSetErrorFunc(NULL, NULL); virSetErrorLogPriorityFunc(NULL); that way, errors printed by libvirt functions such as virAsprintf are SURE to go to stderr. This feels wrong. The file should already be created by libvirtd before it spawned dnsmasq. Again, I'd rather omit keys for values that were not provided by dnsmasq. Shouldn't you error out if 'action' was not one of the three strings you know how to handle? That way, if future dnsmasq adds capabilities, we get logging that we didn't know what to do with it. The else is redundant, because the if called continue. Again. And dropping the else and the indentation will fix your style issue (if either side of if/else used {}, then both sides should use it). You should append the integer value of expirytime_tmp rather than a string. Changing your return value to success prior to writing is fishy. I'd defer this to the very end, when you know you succeeded. Also, EXIT_SUCCESS is nicer than hard-coded 0. Writing an empty string when we encountered an error feels fishy. We should instead report failure if leases_str is NULL. Hmmm. This is not atomic. If your program gets killed mid-write, it will leave the half-written new stuff at the front and the old data at the back. You want to use virFileRewrite() instead, because that interface guarantees via rename() that the official file name will be either the old contents or the new contents. Again, -1 is not a good return value. EXIT_FAILURE is better. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org