On 15.01.2012 21:31, Paolo Bonzini wrote: > On 01/15/2012 05:44 PM, Michael Tokarev wrote: >>>>>> + * stdout (temporarily) to the pipe to parent, >>>>> >>>>> This is a bit of a hack. >>>> >>>> There's another way -- to keep the writing pipe end in some >>>> local variable and use that one instead of STDOUT_FILENO. >>>> I can do it that way for sure, just thought it's already >>>> using too much local variables. >>> >>> Yes, that would be better. >> >> Done in a v2 version I sent you. > > Please stay on the list.
Sorry? I sent it to you and to the list, here's the command line from my .bash_history: git format-patch --subject-prefix="PATCH v2" --stdout --to 'qemu-devel@nongnu.org' --cc "Paolo Bonzini <pbonz...@redhat.com>" --cc m...@tls.msk.ru HEAD^ | /usr/sbin/sendmail -t -i On which list I shoult stay? [] >> Um, I missed that "half of this" part. Indeed, nbd_client_thread() >> does dup2(STDOUT_FILENO, STDERR_FILENO) which should go away, but >> it is harmless for now, and can be addressed in a separate patch. > > Again, _the client thread_ is the right place to do this! See below. [] >> We're doomed anyway, and it is even good >> we've a small remote chance for our error message to >> be seen. Currently it just goes to /dev/null. > > No, currently it is sent from the daemon to the parent through the pipe, the > parent prints it and exits with status code 1. With your patch, if the dup2 > wins the race you exit with status code 0; if the client thread wins the race > it is the same as master. Aha. I finally see what you mean. I still disagree, -- all the operations done in the client thread can be done before forking a new thread, syncronously, and _that_ will be the easiest and cleanest solution here. >> That's not a bad intention. I'm fixing existing logic without >> introducing new logical changes. If you want to fix other >> stuff, it is better be done in a separate commit/change. > > AFAIK the only known bug (besides the devfd/sockfd mixup) is the missing > chdir, and that should be fixed first. It all looked so ugly to me that I didn't even want to think about just adding a chdir() instead of getting rid of daemon(). But ok, I can go that ugly route too. Thanks, /mjt
