On 11.04.2013 15:33, harryxiyou wrote: Maybe you've pasted wrong code snippet, but the one you've pasted is correct. If virCommandRun() fails and returns -1; we return that value to the caller. And if the command doesn't fail, we call virStorageBackendSheepdogParseNodeInfo to parse output of the command. Michal

On 04/11/2013 07:48 AM, harryxiyou wrote: Not necessary. virCommand is DESIGNED for streamlined usage, so that it is much easier to read how the command is constructed without being distracted by error checking in the caller every step of the way. As a virCommandPtr has no semantic impact until it is run, it is sufficient to delay error checking until the caller is actually ready to run the command. Therefore, we wrote virCommandRun() to specifically check for NULL, and report an error at that time, so that the caller need not worry about virCommandNew* returning NULL. No bug here. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On Thu, Apr 11, 2013 at 10:48:05PM +0800, harryxiyou wrote: The question of efficiency is irrelevant in this context. We are explicitly choosing to prioritize reliability and readability of the code here. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 04/11/2013 08:48 AM, harryxiyou wrote: No need. You are talking about the error path, the one that only hits on the RARE case of OOM. If we are OOM, we are already hosed - what does it matter if we take a few extra calls to a few more virCommand*() functions before finally reporting the error to the user? All that matters is that we are hosed gracefully (ie. that we DO report the error, instead of crashing). On the common case, virCommandNew*() never fails, so every step of the way is important and there is nothing that can be shaved. It boils down to whether you check for NULL at every call site (a maintenance burden) or inside every virCommand*() API - either way, a NULL check has to be done to remain robust, but we intentionally chose the design that results in the fewest lines of code. There is NOTHING wrong here. Quit trying to make a bug out of something that's working. One more point: libvirt is generally not the bottleneck. Remember, libvirt is the management, the thing that kicks off the long-running tasks. But the long running tasks are separate processes. Libvirt itself is not doing the long-running task. Therefore, shaving a few instructions off of libvirt is generally going to have a negligible effect on the overall performance of your usage of the virt stack. Focus on features, not micro-optimizations, unless you can first post benchmarks proving that libvirt is indeed the cause of a bottleneck you are trying to optimize. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 11.04.2013 15:48, harryxiyou wrote: And yet again - the code is bugfree. If virCommandNewArgList returns NULL, this is passed to virStorageBackendSheepdogAddHostArg() which operates then with virCommand* API. This API is perfectly comfortable with working over NULL. That is, so we can have: cmd = virCommmandNew*(blah, blah, ....); virCommand*(cmd, ...); /* repeated multiple times */ if (virCommandRun(cmd, NULL) < 0) { printf("Ouch!"); goto cleanup; } instead of: cmd = virCommandNew*(blah, blah, ...); if (!cmd) goto cleanup; if (virCommand*(cmd, ...) < 0) goto cleanup; if (virCommand*(cmd, ...) < 0) goto cleanup; if (virCommand*(cmd, ...) < 0) goto cleanup; if (virCommandRun(cmd, ...) < 0) { printf("Ouch!"); goto cleanup; } If you want to dig into libvirt code I'd recommend going though our HACKING: http://libvirt.org/hacking.html and generating tags for the code (ctags -R .) so whenever you open a file in your editor you can easily jump to a function you cursor is at. That is: in this case, if you run with cursor over the line number 123 and press CTRL + ] in vim (I bet there are plenty of emacs users who will contribute with their hotkey for this) vim takes you to the function definition so you can see what it actually does. And if you apply this process twice it will get you to the virCommand* implementation where you'll see NULL check at the beginning of each API (if not, *that's* a real bug). Happy hacking! Michal