[libvirt] [RFC] virCommandRun return error number
Hi all, I copy portions of libvirt/src/storage/storage_backend_sheepdog.c [...] 125 ret = virCommandRun(cmd, NULL); 126 if (ret == 0) 127 ret = virStorageBackendSheepdogParseNodeInfo(pool->def, output); [...] I found virCommandRun may return '-1', which hinds something error happens. However, Sheepdog hasn't check here. Maybe i have come up with this bug. I cannot remember clearly. Could anyone please give me some suggestions? If ture, i will give a patch for this bug ;-). -- Thanks Harry Wei
On 11.04.2013 15:33, harryxiyou wrote:
Hi all,
I copy portions of libvirt/src/storage/storage_backend_sheepdog.c [...] 125 ret = virCommandRun(cmd, NULL); 126 if (ret == 0) 127 ret = virStorageBackendSheepdogParseNodeInfo(pool->def, output); [...]
I found virCommandRun may return '-1', which hinds something error happens. However, Sheepdog hasn't check here. Maybe i have come up with this bug. I cannot remember clearly. Could anyone please give me some suggestions? If ture, i will give a patch for this bug ;-).
-- Thanks Harry Wei
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 Thu, Apr 11, 2013 at 9:38 PM, Michal Privoznik <mprivozn@redhat.com> 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.
Sorry, i have asked for a stupid bug. Let us check following one. [...] 114 static int 115 virStorageBackendSheepdogRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, 116 virStoragePoolObjPtr pool) 117 { 118 int ret; 119 char *output = NULL; 120 virCommandPtr cmd; 121 122 cmd = virCommandNewArgList(COLLIE, "node", "info", "-r", NULL); 123 virStorageBackendSheepdogAddHostArg(cmd, pool); [...] virCommandNewArgList may return ‘NULL’, so i think we should check this condition, which we need not do following stuffs, right? -- Thanks Harry Wei
On 04/11/2013 07:48 AM, harryxiyou wrote:
On Thu, Apr 11, 2013 at 9:38 PM, Michal Privoznik <mprivozn@redhat.com> 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.
Sorry, i have asked for a stupid bug. Let us check following one.
[...] 114 static int 115 virStorageBackendSheepdogRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, 116 virStoragePoolObjPtr pool) 117 { 118 int ret; 119 char *output = NULL; 120 virCommandPtr cmd; 121 122 cmd = virCommandNewArgList(COLLIE, "node", "info", "-r", NULL); 123 virStorageBackendSheepdogAddHostArg(cmd, pool); [...]
virCommandNewArgList may return ‘NULL’, so i think we should check this condition, which we need not do following stuffs, right?
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:26 PM, Eric Blake <eblake@redhat.com> wrote: Hi Eric, [...]
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.
ACK. However, we really need not do the following stuffs. It may affect efficiencies. Maybe i have thought more about this matter. -- Thanks Harry Wei
On Thu, Apr 11, 2013 at 10:48:05PM +0800, harryxiyou wrote:
On Thu, Apr 11, 2013 at 10:26 PM, Eric Blake <eblake@redhat.com> wrote:
Hi Eric,
[...]
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.
ACK. However, we really need not do the following stuffs. It may affect efficiencies. Maybe i have thought more about this matter.
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:
On Thu, Apr 11, 2013 at 10:26 PM, Eric Blake <eblake@redhat.com> wrote:
Hi Eric,
[...]
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.
ACK. However, we really need not do the following stuffs. It may affect efficiencies. Maybe i have thought more about this matter.
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 Thu, Apr 11, 2013 at 11:03 PM, Eric Blake <eblake@redhat.com> 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).
ACK.
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.
Hmmm..., i see. Let me focus on our Libvirt features, thanks. -- Thanks Harry Wei
On 11.04.2013 15:48, harryxiyou wrote:
On Thu, Apr 11, 2013 at 9:38 PM, Michal Privoznik <mprivozn@redhat.com> 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.
Sorry, i have asked for a stupid bug. Let us check following one.
[...] 114 static int 115 virStorageBackendSheepdogRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, 116 virStoragePoolObjPtr pool) 117 { 118 int ret; 119 char *output = NULL; 120 virCommandPtr cmd; 121 122 cmd = virCommandNewArgList(COLLIE, "node", "info", "-r", NULL); 123 virStorageBackendSheepdogAddHostArg(cmd, pool); [...]
virCommandNewArgList may return ‘NULL’, so i think we should check this condition, which we need not do following stuffs, right?
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
participants (4)
-
Daniel P. Berrange -
Eric Blake -
harryxiyou -
Michal Privoznik