Re: [PATCH 1/3] qsd: Add pre-init argument parsing pass
On 24.01.22 10:23, Markus Armbruster wrote: Hanna Reitz writes: On 21.01.22 15:26, Markus Armbruster wrote: Hanna Reitz writes: On 21.01.22 11:27, Markus Armbruster wrote: Hanna Reitz writes: The problem I face is that currently there is no ergonomic way to wait until the QSD is up and running (besides looping until the PID file exists), and I don’t think a utility program that doesn’t know the QSD could provide this. (For example, it looks like daemonize(1) will have the parent exit immediately, regardless of whether the child is set up or not.) Why do you need to wait for QSD to be ready? I'm asking because with common daemons, I don't wait, I just connect to their socket and start talking. They'll reply only when ready. That only applies when you want to talk to a socket, which I often don’t do. Most of the time I use the storage daemon, I pass all --blockdev and --export options through the command line and don’t create any socket at all. When I use the QSD just to export some block device, I generally don’t need QMP. If you export via NBD, why can't you just connect to NBD socket? I’m not sure what exactly you mean by this, because the socket doesn’t exist before the QSD is launched. If I launch the QSD in the background and have it create an NBD server on a Unix socket, then this socket will not exist until the respective --nbd-server option is parsed. Trying to connect to it immediately after the QSD has been launched may work (if the QSD was quicker to parse the option and create the server than me trying to connect) or may yield ECONNREFUSED or ENOENT, depending on whether the socket file existed before or not. This is similar to "with common daemons, [...] I just connect to their socket and start talking." Also, outside of the iotests, I personally generally usually use FUSE exports instead of NBD exports. You could wait for the mount to appear with stat -f. As I’ve said from my very first reply on this thread, I could also simply use --pidfile and wait for the PID file to appear. I simply thought “Oh, that doesn’t feel nice, it would be very nice if I could simply have an option for QSD to launch it such that it would put itself in the background once its done.”
Re: [PATCH 1/3] qsd: Add pre-init argument parsing pass
Hanna Reitz writes: > On 21.01.22 15:26, Markus Armbruster wrote: >> Hanna Reitz writes: >> >>> On 21.01.22 11:27, Markus Armbruster wrote: Hanna Reitz writes: > The problem I face is that currently there is no ergonomic way to wait > until the QSD is up and running (besides looping until the PID file > exists), and I don’t think a utility program that doesn’t know the QSD > could provide this. (For example, it looks like daemonize(1) will > have the parent exit immediately, regardless of whether the child is > set up or not.) Why do you need to wait for QSD to be ready? I'm asking because with common daemons, I don't wait, I just connect to their socket and start talking. They'll reply only when ready. >>> That only applies when you want to talk to a socket, which I often >>> don’t do. Most of the time I use the storage daemon, I pass all >>> --blockdev and --export options through the command line and don’t >>> create any socket at all. When I use the QSD just to export some >>> block device, I generally don’t need QMP. >> >> If you export via NBD, why can't you just connect to NBD socket? > > I’m not sure what exactly you mean by this, because the socket doesn’t > exist before the QSD is launched. If I launch the QSD in the > background and have it create an NBD server on a Unix socket, then > this socket will not exist until the respective --nbd-server option is > parsed. Trying to connect to it immediately after the QSD has been > launched may work (if the QSD was quicker to parse the option and > create the server than me trying to connect) or may yield ECONNREFUSED > or ENOENT, depending on whether the socket file existed before or not. This is similar to "with common daemons, [...] I just connect to their socket and start talking." > Also, outside of the iotests, I personally generally usually use FUSE > exports instead of NBD exports. You could wait for the mount to appear with stat -f.
Re: [PATCH 1/3] qsd: Add pre-init argument parsing pass
On 21.01.22 15:26, Markus Armbruster wrote: Hanna Reitz writes: On 21.01.22 11:27, Markus Armbruster wrote: Hanna Reitz writes: The problem I face is that currently there is no ergonomic way to wait until the QSD is up and running (besides looping until the PID file exists), and I don’t think a utility program that doesn’t know the QSD could provide this. (For example, it looks like daemonize(1) will have the parent exit immediately, regardless of whether the child is set up or not.) Why do you need to wait for QSD to be ready? I'm asking because with common daemons, I don't wait, I just connect to their socket and start talking. They'll reply only when ready. That only applies when you want to talk to a socket, which I often don’t do. Most of the time I use the storage daemon, I pass all --blockdev and --export options through the command line and don’t create any socket at all. When I use the QSD just to export some block device, I generally don’t need QMP. If you export via NBD, why can't you just connect to NBD socket? I’m not sure what exactly you mean by this, because the socket doesn’t exist before the QSD is launched. If I launch the QSD in the background and have it create an NBD server on a Unix socket, then this socket will not exist until the respective --nbd-server option is parsed. Trying to connect to it immediately after the QSD has been launched may work (if the QSD was quicker to parse the option and create the server than me trying to connect) or may yield ECONNREFUSED or ENOENT, depending on whether the socket file existed before or not. Also, outside of the iotests, I personally generally usually use FUSE exports instead of NBD exports.
Re: [PATCH 1/3] qsd: Add pre-init argument parsing pass
Hanna Reitz writes: > On 21.01.22 11:27, Markus Armbruster wrote: >> Hanna Reitz writes: >>> The problem I face is that currently there is no ergonomic way to wait >>> until the QSD is up and running (besides looping until the PID file >>> exists), and I don’t think a utility program that doesn’t know the QSD >>> could provide this. (For example, it looks like daemonize(1) will >>> have the parent exit immediately, regardless of whether the child is >>> set up or not.) >> >> Why do you need to wait for QSD to be ready? >> >> I'm asking because with common daemons, I don't wait, I just connect to >> their socket and start talking. They'll reply only when ready. > > That only applies when you want to talk to a socket, which I often > don’t do. Most of the time I use the storage daemon, I pass all > --blockdev and --export options through the command line and don’t > create any socket at all. When I use the QSD just to export some > block device, I generally don’t need QMP. If you export via NBD, why can't you just connect to NBD socket? > Of course, I could just not do that, and instead only set up QMP and > then do all the configuration through that (where, as you say, QSD > will only reply once it can); but that’s much more complicated than > running a single command. > > (Or I could do a mix of both, which I described above, where I’d > create and have the QSD connect to a Unix socket just to see that > configuration is done and all exports are up. I’d prefer not to, > because it still means using an extra tool (ncat) to create the > socket.)
Re: [PATCH 1/3] qsd: Add pre-init argument parsing pass
On 21.01.22 11:27, Markus Armbruster wrote: Hanna Reitz writes: On 21.01.22 07:10, Markus Armbruster wrote: Hanna Reitz writes: On 20.01.22 17:00, Markus Armbruster wrote: Kevin Wolf writes: Am 19.01.2022 um 14:44 hat Hanna Reitz geschrieben: On 19.01.22 13:58, Markus Armbruster wrote: Hanna Reitz writes: We want to add a --daemonize argument to QSD's command line. Why? OK, s/we/I/. I find it useful, because without such an option, I need to have whoever invokes QSD loop until the PID file exists, before I can be sure that all exports are set up. I make use of it in the test cases added in patch 3. I suppose this could be worked around with a special character device, like so: ``` ncat --listen -U /tmp/qsd-done.sock I know duplicating this into every program that could server as a daemon is the Unix tradition. Doesn't make it good. Systemd[*] has tried to make it superfluous. Well. I have absolutely nothing against systemd. Still, I will not use it in an iotest, that’s for sure. My point isn't "use systemd in iotests". It's "consider doing it like systemd", i.e. do the daemonization work in a utility program. For what it's worth, Linux has daemonize(1). The problem I face is that currently there is no ergonomic way to wait until the QSD is up and running (besides looping until the PID file exists), and I don’t think a utility program that doesn’t know the QSD could provide this. (For example, it looks like daemonize(1) will have the parent exit immediately, regardless of whether the child is set up or not.) Why do you need to wait for QSD to be ready? I'm asking because with common daemons, I don't wait, I just connect to their socket and start talking. They'll reply only when ready. That only applies when you want to talk to a socket, which I often don’t do. Most of the time I use the storage daemon, I pass all --blockdev and --export options through the command line and don’t create any socket at all. When I use the QSD just to export some block device, I generally don’t need QMP. Of course, I could just not do that, and instead only set up QMP and then do all the configuration through that (where, as you say, QSD will only reply once it can); but that’s much more complicated than running a single command. (Or I could do a mix of both, which I described above, where I’d create and have the QSD connect to a Unix socket just to see that configuration is done and all exports are up. I’d prefer not to, because it still means using an extra tool (ncat) to create the socket.)
Re: [PATCH 1/3] qsd: Add pre-init argument parsing pass
Hanna Reitz writes: > On 21.01.22 07:10, Markus Armbruster wrote: >> Hanna Reitz writes: >> >>> On 20.01.22 17:00, Markus Armbruster wrote: Kevin Wolf writes: > Am 19.01.2022 um 14:44 hat Hanna Reitz geschrieben: >> On 19.01.22 13:58, Markus Armbruster wrote: >>> Hanna Reitz writes: >>> We want to add a --daemonize argument to QSD's command line. >>> Why? >> OK, s/we/I/. I find it useful, because without such an option, I need to >> have whoever invokes QSD loop until the PID file exists, before I can be >> sure that all exports are set up. I make use of it in the test cases >> added >> in patch 3. >> >> I suppose this could be worked around with a special character device, >> like >> so: >> >> ``` >> ncat --listen -U /tmp/qsd-done.sock > ncat_pid=$! >> >> qemu-storage-daemon \ >> ... \ >> --chardev socket,id=signal_done,path=/tmp/qsd-done.sock \ >> --monitor signal_done \ >> --pidfile /tmp/qsd.pid & >> >> wait $ncat_pid >> ``` >> >> But having to use an extra tool for this is unergonomic. I mean, if >> there’s >> no other way... I know duplicating this into every program that could server as a daemon is the Unix tradition. Doesn't make it good. Systemd[*] has tried to make it superfluous. >>> >>> Well. I have absolutely nothing against systemd. Still, I will not >>> use it in an iotest, that’s for sure. > >> My point isn't "use systemd in iotests". It's "consider doing it like >> systemd", i.e. do the daemonization work in a utility program. For what >> it's worth, Linux has daemonize(1). > > The problem I face is that currently there is no ergonomic way to wait > until the QSD is up and running (besides looping until the PID file > exists), and I don’t think a utility program that doesn’t know the QSD > could provide this. (For example, it looks like daemonize(1) will > have the parent exit immediately, regardless of whether the child is > set up or not.) Why do you need to wait for QSD to be ready? I'm asking because with common daemons, I don't wait, I just connect to their socket and start talking. They'll reply only when ready. >> [...] >> Care to put a brief version of the rationale for --daemonize and for forking early in the commit message? >>> >>> Well, my rationale for adding the feature doesn’t really extend beyond >>> “I want it, I find it useful, and so I assume others will, too”. >>> >> Don't pretend to be obtuse, it's not credible :) You mentioned iotests, >> which makes me guess your rationale is "I want this for iotests, and >> there may well be other uses." > > Oh, I also want it for other things, like the script I have to use the > QSD to make disk images accessible as raw files. Thing is, the stress > is on “want” in contrast to “need”. I can do without --daemonize, I > have already done so, even before there was --pidfile (I just queried > the block exports through QMP until they were all there). It’s just > that it’s kind of a pain. > > Same with the iotests, it’s absolutely possible to get away without > --daemonize. It’s just that I wrote the test, wanted to use some form > of --daemonize option, noticed there wasn’t any yet, and thought “Oh, > that’d be nice to have”. > > I would love a --daemonize option, but I can’t say it’s necessary. If > the way it’d need to be implemented isn’t acceptable, then I won’t > force it into the code. Rationale doesn't have to be "we must have this because". It can also be "I want this because". What it can't be is "I want this". [...]
Re: [PATCH 1/3] qsd: Add pre-init argument parsing pass
On 21.01.22 07:10, Markus Armbruster wrote: Hanna Reitz writes: On 20.01.22 17:00, Markus Armbruster wrote: Kevin Wolf writes: Am 19.01.2022 um 14:44 hat Hanna Reitz geschrieben: On 19.01.22 13:58, Markus Armbruster wrote: Hanna Reitz writes: We want to add a --daemonize argument to QSD's command line. Why? OK, s/we/I/. I find it useful, because without such an option, I need to have whoever invokes QSD loop until the PID file exists, before I can be sure that all exports are set up. I make use of it in the test cases added in patch 3. I suppose this could be worked around with a special character device, like so: ``` ncat --listen -U /tmp/qsd-done.sock I know duplicating this into every program that could server as a daemon is the Unix tradition. Doesn't make it good. Systemd[*] has tried to make it superfluous. Well. I have absolutely nothing against systemd. Still, I will not use it in an iotest, that’s for sure. My point isn't "use systemd in iotests". It's "consider doing it like systemd", i.e. do the daemonization work in a utility program. For what it's worth, Linux has daemonize(1). The problem I face is that currently there is no ergonomic way to wait until the QSD is up and running (besides looping until the PID file exists), and I don’t think a utility program that doesn’t know the QSD could provide this. (For example, it looks like daemonize(1) will have the parent exit immediately, regardless of whether the child is set up or not.) [...] Care to put a brief version of the rationale for --daemonize and for forking early in the commit message? Well, my rationale for adding the feature doesn’t really extend beyond “I want it, I find it useful, and so I assume others will, too”. Don't pretend to be obtuse, it's not credible :) You mentioned iotests, which makes me guess your rationale is "I want this for iotests, and there may well be other uses." Oh, I also want it for other things, like the script I have to use the QSD to make disk images accessible as raw files. Thing is, the stress is on “want” in contrast to “need”. I can do without --daemonize, I have already done so, even before there was --pidfile (I just queried the block exports through QMP until they were all there). It’s just that it’s kind of a pain. Same with the iotests, it’s absolutely possible to get away without --daemonize. It’s just that I wrote the test, wanted to use some form of --daemonize option, noticed there wasn’t any yet, and thought “Oh, that’d be nice to have”. I would love a --daemonize option, but I can’t say it’s necessary. If the way it’d need to be implemented isn’t acceptable, then I won’t force it into the code. I don’t really like putting “qemu-nbd has it” there, because... it was again me who implemented it for qemu-nbd. Because I found it useful. But I can of course do that, if it counts as a reason. Useful *what for*, and we have rationale. I can certainly (and understand the need to, and will) elaborate on the “This will require forking the process before we do any complex initialization steps” part. Thanks!
Re: [PATCH 1/3] qsd: Add pre-init argument parsing pass
Hanna Reitz writes: > On 20.01.22 17:00, Markus Armbruster wrote: >> Kevin Wolf writes: >> >>> Am 19.01.2022 um 14:44 hat Hanna Reitz geschrieben: On 19.01.22 13:58, Markus Armbruster wrote: > Hanna Reitz writes: > >> We want to add a --daemonize argument to QSD's command line. > Why? OK, s/we/I/. I find it useful, because without such an option, I need to have whoever invokes QSD loop until the PID file exists, before I can be sure that all exports are set up. I make use of it in the test cases added in patch 3. I suppose this could be worked around with a special character device, like so: ``` ncat --listen -U /tmp/qsd-done.sock >>> ncat_pid=$! qemu-storage-daemon \ ... \ --chardev socket,id=signal_done,path=/tmp/qsd-done.sock \ --monitor signal_done \ --pidfile /tmp/qsd.pid & wait $ncat_pid ``` But having to use an extra tool for this is unergonomic. I mean, if there’s no other way... >> >> I know duplicating this into every program that could server as a daemon >> is the Unix tradition. Doesn't make it good. Systemd[*] has tried to >> make it superfluous. > > Well. I have absolutely nothing against systemd. Still, I will not > use it in an iotest, that’s for sure. My point isn't "use systemd in iotests". It's "consider doing it like systemd", i.e. do the daemonization work in a utility program. For what it's worth, Linux has daemonize(1). [...] >> Care to put a brief version of the rationale for --daemonize and for >> forking early in the commit message? > > Well, my rationale for adding the feature doesn’t really extend beyond > “I want it, I find it useful, and so I assume others will, too”. Don't pretend to be obtuse, it's not credible :) You mentioned iotests, which makes me guess your rationale is "I want this for iotests, and there may well be other uses." > I don’t really like putting “qemu-nbd has it” there, because... it was > again me who implemented it for qemu-nbd. Because I found it useful. > But I can of course do that, if it counts as a reason. Useful *what for*, and we have rationale. > I can certainly (and understand the need to, and will) elaborate on > the “This will require forking the process before we do any complex > initialization steps” part. Thanks!
Re: [PATCH 1/3] qsd: Add pre-init argument parsing pass
On 20.01.22 17:00, Markus Armbruster wrote: Kevin Wolf writes: Am 19.01.2022 um 14:44 hat Hanna Reitz geschrieben: On 19.01.22 13:58, Markus Armbruster wrote: Hanna Reitz writes: We want to add a --daemonize argument to QSD's command line. Why? OK, s/we/I/. I find it useful, because without such an option, I need to have whoever invokes QSD loop until the PID file exists, before I can be sure that all exports are set up. I make use of it in the test cases added in patch 3. I suppose this could be worked around with a special character device, like so: ``` ncat --listen -U /tmp/qsd-done.sock I know duplicating this into every program that could server as a daemon is the Unix tradition. Doesn't make it good. Systemd[*] has tried to make it superfluous. Well. I have absolutely nothing against systemd. Still, I will not use it in an iotest, that’s for sure. The other point is that the system emulator has it, qemu-nbd has it, so certainly qsd should have it as well. Not the least because it should be able to replace qemu-nbd (at least for the purpose of exporting NBD. not necessarily for attaching it to the host). Point taken, but I think it's a somewhat weak one. qsd could certainly replace qemu-nbd even without --daemonize; we could use other means to run it in the background. This will require forking the process before we do any complex initialization steps, like setting up the block layer or QMP. Therefore, we must scan the command line for it long before our current process_options() call. Can you explain in a bit more detail why early forking is required? I have a strong dislike for parsing more than once... Because I don’t want to set up QMP and block devices, and then fork the process into two. That sounds like there’d be a lot of stuff to think about, which just isn’t necessary, because we don’t need to set up any of this in the parent. We must fork() before we create threads. Other resources are easy enough to hand over to the child. Still, having to think about less is good, I readily grant you that. The trouble is that forking early creates a new problem: any configuration errors detected in the child must be propagated to the parent somehow (output and exit status). I peeked at your PATCH 2, and I'm not convinced, but that's detail here. Here we can compare again: Both the system emulator and qemu-nbd behave the same, they fork before they do anything interesting. The difference is that they still parse the command line only once because they don't immediately create things, but just store the options and later process them in their own magic order. I'd much rather parse the command line twice than copy that behaviour. The part I hate is "own magic order". Without that, multiple passes are just fine with me. Parsing twice is a bit like having a two pass compiler run the first pass left to right, and then both passes intertwined left to right. The pedestrian way to do it is running the first pass left to right, then the second pass left to right. We're clearly talking taste here. Kevin For example, if I set up a monitor on a Unix socket (server=true), processing is delayed until the client connects. Say I put --daemonize afterwards. I connect to the waiting server socket, the child is forked off, and then... I’m not sure what happens, actually. Do I have a connection with both the parent and the child listening? I know that in practice, what happens is that once the parent exits, the connection is closed, and I get a “qemu: qemu_thread_join: Invalid argument” warning/error on the QSD side. There’s a lot of stuff to think about if you allow forking after other options, so it should be done first. We could just require the user to put --daemonize before all other options, and so have a single pass; but still, before options are even parsed, we have already for example called bdrv_init(), init_qmp_commands(), qemu_init_main_loop(). These are all things that the parent of a daemonizing process doesn’t need to do, and where I’d simply rather not think about what impact it has if we fork afterwards. Hanna Care to put a brief version of the rationale for --daemonize and for forking early in the commit message? Well, my rationale for adding the feature doesn’t really extend beyond “I want it, I find it useful, and so I assume others will, too”. I don’t really like putting “qemu-nbd has it” there, because... it was again me who implemented it for qemu-nbd. Because I found it useful. But I can of course do that, if it counts as a reason. I can certainly (and understand the need to, and will) elaborate on the “This will require forking the process before we do any complex initialization steps” part. Hanna
Re: [PATCH 1/3] qsd: Add pre-init argument parsing pass
Kevin Wolf writes: > Am 19.01.2022 um 14:44 hat Hanna Reitz geschrieben: >> On 19.01.22 13:58, Markus Armbruster wrote: >> > Hanna Reitz writes: >> > >> > > We want to add a --daemonize argument to QSD's command line. >> > >> > Why? >> >> OK, s/we/I/. I find it useful, because without such an option, I need to >> have whoever invokes QSD loop until the PID file exists, before I can be >> sure that all exports are set up. I make use of it in the test cases added >> in patch 3. >> >> I suppose this could be worked around with a special character device, like >> so: >> >> ``` >> ncat --listen -U /tmp/qsd-done.sock > ncat_pid=$! >> >> qemu-storage-daemon \ >> ... \ >> --chardev socket,id=signal_done,path=/tmp/qsd-done.sock \ >> --monitor signal_done \ >> --pidfile /tmp/qsd.pid & >> >> wait $ncat_pid >> ``` >> >> But having to use an extra tool for this is unergonomic. I mean, if there’s >> no other way... I know duplicating this into every program that could server as a daemon is the Unix tradition. Doesn't make it good. Systemd[*] has tried to make it superfluous. > The other point is that the system emulator has it, qemu-nbd has it, > so certainly qsd should have it as well. Not the least because it should > be able to replace qemu-nbd (at least for the purpose of exporting NBD. > not necessarily for attaching it to the host). Point taken, but I think it's a somewhat weak one. qsd could certainly replace qemu-nbd even without --daemonize; we could use other means to run it in the background. >> > >This will >> > > require forking the process before we do any complex initialization >> > > steps, like setting up the block layer or QMP. Therefore, we must scan >> > > the command line for it long before our current process_options() call. >> > >> > Can you explain in a bit more detail why early forking is required? >> > >> > I have a strong dislike for parsing more than once... >> >> Because I don’t want to set up QMP and block devices, and then fork the >> process into two. That sounds like there’d be a lot of stuff to think >> about, which just isn’t necessary, because we don’t need to set up any >> of this in the parent. We must fork() before we create threads. Other resources are easy enough to hand over to the child. Still, having to think about less is good, I readily grant you that. The trouble is that forking early creates a new problem: any configuration errors detected in the child must be propagated to the parent somehow (output and exit status). I peeked at your PATCH 2, and I'm not convinced, but that's detail here. > Here we can compare again: Both the system emulator and qemu-nbd behave > the same, they fork before they do anything interesting. > > The difference is that they still parse the command line only once > because they don't immediately create things, but just store the options > and later process them in their own magic order. I'd much rather parse > the command line twice than copy that behaviour. The part I hate is "own magic order". Without that, multiple passes are just fine with me. Parsing twice is a bit like having a two pass compiler run the first pass left to right, and then both passes intertwined left to right. The pedestrian way to do it is running the first pass left to right, then the second pass left to right. We're clearly talking taste here. > > Kevin > >> For example, if I set up a monitor on a Unix socket (server=true), >> processing is delayed until the client connects. Say I put --daemonize >> afterwards. I connect to the waiting server socket, the child is forked >> off, and then... I’m not sure what happens, actually. Do I have a >> connection with both the parent and the child listening? I know that in >> practice, what happens is that once the parent exits, the connection is >> closed, and I get a “qemu: qemu_thread_join: Invalid argument” warning/error >> on the QSD side. >> >> There’s a lot of stuff to think about if you allow forking after other >> options, so it should be done first. We could just require the user to put >> --daemonize before all other options, and so have a single pass; but still, >> before options are even parsed, we have already for example called >> bdrv_init(), init_qmp_commands(), qemu_init_main_loop(). These are all >> things that the parent of a daemonizing process doesn’t need to do, and >> where I’d simply rather not think about what impact it has if we fork >> afterwards. >> >> Hanna Care to put a brief version of the rationale for --daemonize and for forking early in the commit message? [...] [*] Not everything systemd does is bad. It's a big, mixed bag of ideas.
Re: [PATCH 1/3] qsd: Add pre-init argument parsing pass
Am 19.01.2022 um 14:44 hat Hanna Reitz geschrieben: > On 19.01.22 13:58, Markus Armbruster wrote: > > Hanna Reitz writes: > > > > > We want to add a --daemonize argument to QSD's command line. > > Why? > > OK, s/we/I/. I find it useful, because without such an option, I need to > have whoever invokes QSD loop until the PID file exists, before I can be > sure that all exports are set up. I make use of it in the test cases added > in patch 3. > > I suppose this could be worked around with a special character device, like > so: > > ``` > ncat --listen -U /tmp/qsd-done.sock ncat_pid=$! > > qemu-storage-daemon \ > ... \ > --chardev socket,id=signal_done,path=/tmp/qsd-done.sock \ > --monitor signal_done \ > --pidfile /tmp/qsd.pid & > > wait $ncat_pid > ``` > > But having to use an extra tool for this is unergonomic. I mean, if there’s > no other way... The other point is that the system emulator has it, qemu-nbd has it, so certainly qsd should have it as well. Not the least because it should be able to replace qemu-nbd (at least for the purpose of exporting NBD. not necessarily for attaching it to the host). > > >This will > > > require forking the process before we do any complex initialization > > > steps, like setting up the block layer or QMP. Therefore, we must scan > > > the command line for it long before our current process_options() call. > > Can you explain in a bit more detail why early forking is required? > > > > I have a strong dislike for parsing more than once... > > Because I don’t want to set up QMP and block devices, and then fork the > process into two. That sounds like there’d be a lot of stuff to think > about, which just isn’t necessary, because we don’t need to set up any > of this in the parent. Here we can compare again: Both the system emulator and qemu-nbd behave the same, they fork before they do anything interesting. The difference is that they still parse the command line only once because they don't immediately create things, but just store the options and later process them in their own magic order. I'd much rather parse the command line twice than copy that behaviour. Kevin > For example, if I set up a monitor on a Unix socket (server=true), > processing is delayed until the client connects. Say I put --daemonize > afterwards. I connect to the waiting server socket, the child is forked > off, and then... I’m not sure what happens, actually. Do I have a > connection with both the parent and the child listening? I know that in > practice, what happens is that once the parent exits, the connection is > closed, and I get a “qemu: qemu_thread_join: Invalid argument” warning/error > on the QSD side. > > There’s a lot of stuff to think about if you allow forking after other > options, so it should be done first. We could just require the user to put > --daemonize before all other options, and so have a single pass; but still, > before options are even parsed, we have already for example called > bdrv_init(), init_qmp_commands(), qemu_init_main_loop(). These are all > things that the parent of a daemonizing process doesn’t need to do, and > where I’d simply rather not think about what impact it has if we fork > afterwards. > > Hanna > > > > Instead of adding custom new code to do so, just reuse process_options() > > > and give it a @pre_init_pass argument to distinguish the two passes. I > > > believe there are some other switches but --daemonize that deserve > > > parsing in the first pass: > > > > > > - --help and --version are supposed to only print some text and then > > >immediately exit (so any initialization we do would be for naught). > > >This changes behavior, because now "--blockdev inv-drv --help" will > > >print a help text instead of complaining about the --blockdev > > >argument. > > >Note that this is similar in behavior to other tools, though: "--help" > > >is generally immediately acted upon when finding it in the argument > > >list, potentially before other arguments (even ones before it) are > > >acted on. For example, "ls /does-not-exist --help" prints a help text > > >and does not complain about ENOENT. > > > > > > - --pidfile does not need initialization, and is already exempted from > > >the sequential order that process_options() claims to strictly follow > > >(the PID file is only created after all arguments are processed, not > > >at the time the --pidfile argument appears), so it makes sense to > > >include it in the same category as --daemonize. > > > > > > - Invalid arguments should always be reported as soon as possible. (The > > >same caveat with --help applies: That means that "--blockdev inv-drv > > >--inv-arg" will now complain about --inv-arg, not inv-drv.) > > > > > > Note that we could decide to check only for --daemonize in the first > > > pass, and defer --help,
Re: [PATCH 1/3] qsd: Add pre-init argument parsing pass
On 19.01.22 13:58, Markus Armbruster wrote: Hanna Reitz writes: We want to add a --daemonize argument to QSD's command line. Why? OK, s/we/I/. I find it useful, because without such an option, I need to have whoever invokes QSD loop until the PID file exists, before I can be sure that all exports are set up. I make use of it in the test cases added in patch 3. I suppose this could be worked around with a special character device, like so: ``` ncat --listen -U /tmp/qsd-done.sock But having to use an extra tool for this is unergonomic. I mean, if there’s no other way... This will require forking the process before we do any complex initialization steps, like setting up the block layer or QMP. Therefore, we must scan the command line for it long before our current process_options() call. Can you explain in a bit more detail why early forking is required? I have a strong dislike for parsing more than once... Because I don’t want to set up QMP and block devices, and then fork the process into two. That sounds like there’d be a lot of stuff to think about, which just isn’t necessary, because we don’t need to set up any of this in the parent. For example, if I set up a monitor on a Unix socket (server=true), processing is delayed until the client connects. Say I put --daemonize afterwards. I connect to the waiting server socket, the child is forked off, and then... I’m not sure what happens, actually. Do I have a connection with both the parent and the child listening? I know that in practice, what happens is that once the parent exits, the connection is closed, and I get a “qemu: qemu_thread_join: Invalid argument” warning/error on the QSD side. There’s a lot of stuff to think about if you allow forking after other options, so it should be done first. We could just require the user to put --daemonize before all other options, and so have a single pass; but still, before options are even parsed, we have already for example called bdrv_init(), init_qmp_commands(), qemu_init_main_loop(). These are all things that the parent of a daemonizing process doesn’t need to do, and where I’d simply rather not think about what impact it has if we fork afterwards. Hanna Instead of adding custom new code to do so, just reuse process_options() and give it a @pre_init_pass argument to distinguish the two passes. I believe there are some other switches but --daemonize that deserve parsing in the first pass: - --help and --version are supposed to only print some text and then immediately exit (so any initialization we do would be for naught). This changes behavior, because now "--blockdev inv-drv --help" will print a help text instead of complaining about the --blockdev argument. Note that this is similar in behavior to other tools, though: "--help" is generally immediately acted upon when finding it in the argument list, potentially before other arguments (even ones before it) are acted on. For example, "ls /does-not-exist --help" prints a help text and does not complain about ENOENT. - --pidfile does not need initialization, and is already exempted from the sequential order that process_options() claims to strictly follow (the PID file is only created after all arguments are processed, not at the time the --pidfile argument appears), so it makes sense to include it in the same category as --daemonize. - Invalid arguments should always be reported as soon as possible. (The same caveat with --help applies: That means that "--blockdev inv-drv --inv-arg" will now complain about --inv-arg, not inv-drv.) Note that we could decide to check only for --daemonize in the first pass, and defer --help, --version, and checking for invalid arguments to the second one, thus largely keeping our current behavior. However, this would break "--help --daemonize": The child would print the help text to stdout, which is redirected to /dev/null, and so the text would disappear. We would need to have the text be printed to stderr instead, and this would then make the parent process exit with EXIT_FAILURE, which is probably not what we want for --help. This patch does make some references to --daemonize without having implemented it yet, but that will happen in the next patch. Signed-off-by: Hanna Reitz
Re: [PATCH 1/3] qsd: Add pre-init argument parsing pass
Hanna Reitz writes: > We want to add a --daemonize argument to QSD's command line. Why? > This will > require forking the process before we do any complex initialization > steps, like setting up the block layer or QMP. Therefore, we must scan > the command line for it long before our current process_options() call. Can you explain in a bit more detail why early forking is required? I have a strong dislike for parsing more than once... > Instead of adding custom new code to do so, just reuse process_options() > and give it a @pre_init_pass argument to distinguish the two passes. I > believe there are some other switches but --daemonize that deserve > parsing in the first pass: > > - --help and --version are supposed to only print some text and then > immediately exit (so any initialization we do would be for naught). > This changes behavior, because now "--blockdev inv-drv --help" will > print a help text instead of complaining about the --blockdev > argument. > Note that this is similar in behavior to other tools, though: "--help" > is generally immediately acted upon when finding it in the argument > list, potentially before other arguments (even ones before it) are > acted on. For example, "ls /does-not-exist --help" prints a help text > and does not complain about ENOENT. > > - --pidfile does not need initialization, and is already exempted from > the sequential order that process_options() claims to strictly follow > (the PID file is only created after all arguments are processed, not > at the time the --pidfile argument appears), so it makes sense to > include it in the same category as --daemonize. > > - Invalid arguments should always be reported as soon as possible. (The > same caveat with --help applies: That means that "--blockdev inv-drv > --inv-arg" will now complain about --inv-arg, not inv-drv.) > > Note that we could decide to check only for --daemonize in the first > pass, and defer --help, --version, and checking for invalid arguments to > the second one, thus largely keeping our current behavior. However, > this would break "--help --daemonize": The child would print the help > text to stdout, which is redirected to /dev/null, and so the text would > disappear. We would need to have the text be printed to stderr instead, > and this would then make the parent process exit with EXIT_FAILURE, > which is probably not what we want for --help. > > This patch does make some references to --daemonize without having > implemented it yet, but that will happen in the next patch. > > Signed-off-by: Hanna Reitz
Re: [PATCH 1/3] qsd: Add pre-init argument parsing pass
On 30.12.21 17:00, Vladimir Sementsov-Ogievskiy wrote:
22.12.2021 14:41, Hanna Reitz wrote:
We want to add a --daemonize argument to QSD's command line. This will
require forking the process before we do any complex initialization
steps, like setting up the block layer or QMP. Therefore, we must scan
the command line for it long before our current process_options() call.
Instead of adding custom new code to do so, just reuse process_options()
and give it a @pre_init_pass argument to distinguish the two passes. I
believe there are some other switches but --daemonize that deserve
parsing in the first pass:
- --help and --version are supposed to only print some text and then
immediately exit (so any initialization we do would be for naught).
This changes behavior, because now "--blockdev inv-drv --help" will
print a help text instead of complaining about the --blockdev
argument.
Note that this is similar in behavior to other tools, though:
"--help"
is generally immediately acted upon when finding it in the argument
list, potentially before other arguments (even ones before it) are
acted on. For example, "ls /does-not-exist --help" prints a help
text
and does not complain about ENOENT.
- --pidfile does not need initialization, and is already exempted from
the sequential order that process_options() claims to strictly follow
(the PID file is only created after all arguments are processed, not
at the time the --pidfile argument appears), so it makes sense to
include it in the same category as --daemonize.
- Invalid arguments should always be reported as soon as possible. (The
same caveat with --help applies: That means that "--blockdev inv-drv
--inv-arg" will now complain about --inv-arg, not inv-drv.)
Note that we could decide to check only for --daemonize in the first
pass, and defer --help, --version, and checking for invalid arguments to
the second one, thus largely keeping our current behavior. However,
this would break "--help --daemonize": The child would print the help
text to stdout, which is redirected to /dev/null, and so the text would
disappear. We would need to have the text be printed to stderr instead,
and this would then make the parent process exit with EXIT_FAILURE,
which is probably not what we want for --help.
This patch does make some references to --daemonize without having
implemented it yet, but that will happen in the next patch.
Signed-off-by: Hanna Reitz
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
storage-daemon/qemu-storage-daemon.c | 37 ++--
1 file changed, 35 insertions(+), 2 deletions(-)
diff --git a/storage-daemon/qemu-storage-daemon.c
b/storage-daemon/qemu-storage-daemon.c
index 52cf17e8ac..42a52d3b1c 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -164,7 +164,23 @@ static int getopt_set_loc(int argc, char **argv,
const char *optstring,
return c;
}
-static void process_options(int argc, char *argv[])
+/**
+ * Process QSD command-line arguments.
+ *
+ * This is done in two passes:
+ *
+ * First (@pre_init_pass is true), we do a pass where all global
+ * arguments pertaining to the QSD process (like --help or --daemonize)
+ * are processed. This pass is done before most of the QEMU-specific
+ * initialization steps (e.g. initializing the block layer or QMP), and
+ * so must only process arguments that are not really QEMU-specific.
+ *
+ * Second (@pre_init_pass is false), we (sequentially) process all
+ * QEMU/QSD-specific arguments. Many of these arguments are
effectively
+ * translated to QMP commands (like --blockdev for blockdev-add, or
+ * --export for block-export-add).
+ */
+static void process_options(int argc, char *argv[], bool pre_init_pass)
{
int c;
@@ -187,7 +203,22 @@ static void process_options(int argc, char
*argv[])
* they are given on the command lines. This means that things
must be
So, --pidfile already breaks a bit this comment. Still would be good
to adjust it now..
may be, s/options/QEMU-specific options/ or something like this.
Right, makes sense to make it match the comment above the function.
* defined first before they can be referenced in another option.
*/
+ optind = 1;
while ((c = getopt_set_loc(argc, argv, "-hT:V", long_options))
!= -1) {
+ bool handle_option_pre_init;
+
+ /* Should this argument be processed in the pre-init pass? */
+ handle_option_pre_init =
+ c == '?' ||
+ c == 'h' ||
+ c == 'V' ||
+ c == OPTION_PIDFILE;
+
+ /* Process every option only in its respective pass */
+ if (pre_init_pass != handle_option_pre_init) {
+ continue;
+ }
+
switch (c) {
case '?':
exit(EXIT_FAILURE);
@@ -321,6 +352,8 @@ int main(int argc, char *argv[])
qemu_init_exec_dir(argv[0]);
Re: [PATCH 1/3] qsd: Add pre-init argument parsing pass
22.12.2021 14:41, Hanna Reitz wrote:
We want to add a --daemonize argument to QSD's command line. This will
require forking the process before we do any complex initialization
steps, like setting up the block layer or QMP. Therefore, we must scan
the command line for it long before our current process_options() call.
Instead of adding custom new code to do so, just reuse process_options()
and give it a @pre_init_pass argument to distinguish the two passes. I
believe there are some other switches but --daemonize that deserve
parsing in the first pass:
- --help and --version are supposed to only print some text and then
immediately exit (so any initialization we do would be for naught).
This changes behavior, because now "--blockdev inv-drv --help" will
print a help text instead of complaining about the --blockdev
argument.
Note that this is similar in behavior to other tools, though: "--help"
is generally immediately acted upon when finding it in the argument
list, potentially before other arguments (even ones before it) are
acted on. For example, "ls /does-not-exist --help" prints a help text
and does not complain about ENOENT.
- --pidfile does not need initialization, and is already exempted from
the sequential order that process_options() claims to strictly follow
(the PID file is only created after all arguments are processed, not
at the time the --pidfile argument appears), so it makes sense to
include it in the same category as --daemonize.
- Invalid arguments should always be reported as soon as possible. (The
same caveat with --help applies: That means that "--blockdev inv-drv
--inv-arg" will now complain about --inv-arg, not inv-drv.)
Note that we could decide to check only for --daemonize in the first
pass, and defer --help, --version, and checking for invalid arguments to
the second one, thus largely keeping our current behavior. However,
this would break "--help --daemonize": The child would print the help
text to stdout, which is redirected to /dev/null, and so the text would
disappear. We would need to have the text be printed to stderr instead,
and this would then make the parent process exit with EXIT_FAILURE,
which is probably not what we want for --help.
This patch does make some references to --daemonize without having
implemented it yet, but that will happen in the next patch.
Signed-off-by: Hanna Reitz
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
storage-daemon/qemu-storage-daemon.c | 37 ++--
1 file changed, 35 insertions(+), 2 deletions(-)
diff --git a/storage-daemon/qemu-storage-daemon.c
b/storage-daemon/qemu-storage-daemon.c
index 52cf17e8ac..42a52d3b1c 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -164,7 +164,23 @@ static int getopt_set_loc(int argc, char **argv, const
char *optstring,
return c;
}
-static void process_options(int argc, char *argv[])
+/**
+ * Process QSD command-line arguments.
+ *
+ * This is done in two passes:
+ *
+ * First (@pre_init_pass is true), we do a pass where all global
+ * arguments pertaining to the QSD process (like --help or --daemonize)
+ * are processed. This pass is done before most of the QEMU-specific
+ * initialization steps (e.g. initializing the block layer or QMP), and
+ * so must only process arguments that are not really QEMU-specific.
+ *
+ * Second (@pre_init_pass is false), we (sequentially) process all
+ * QEMU/QSD-specific arguments. Many of these arguments are effectively
+ * translated to QMP commands (like --blockdev for blockdev-add, or
+ * --export for block-export-add).
+ */
+static void process_options(int argc, char *argv[], bool pre_init_pass)
{
int c;
@@ -187,7 +203,22 @@ static void process_options(int argc, char *argv[])
* they are given on the command lines. This means that things must be
So, --pidfile already breaks a bit this comment. Still would be good to adjust
it now..
may be, s/options/QEMU-specific options/ or something like this.
* defined first before they can be referenced in another option.
*/
+optind = 1;
while ((c = getopt_set_loc(argc, argv, "-hT:V", long_options)) != -1) {
+bool handle_option_pre_init;
+
+/* Should this argument be processed in the pre-init pass? */
+handle_option_pre_init =
+c == '?' ||
+c == 'h' ||
+c == 'V' ||
+c == OPTION_PIDFILE;
+
+/* Process every option only in its respective pass */
+if (pre_init_pass != handle_option_pre_init) {
+continue;
+}
+
switch (c) {
case '?':
exit(EXIT_FAILURE);
@@ -321,6 +352,8 @@ int main(int argc, char *argv[])
qemu_init_exec_dir(argv[0]);
os_setup_signal_handling();
+process_options(argc, argv, true);
+
module_call_init(MODULE_INIT_QOM);
[PATCH 1/3] qsd: Add pre-init argument parsing pass
We want to add a --daemonize argument to QSD's command line. This will
require forking the process before we do any complex initialization
steps, like setting up the block layer or QMP. Therefore, we must scan
the command line for it long before our current process_options() call.
Instead of adding custom new code to do so, just reuse process_options()
and give it a @pre_init_pass argument to distinguish the two passes. I
believe there are some other switches but --daemonize that deserve
parsing in the first pass:
- --help and --version are supposed to only print some text and then
immediately exit (so any initialization we do would be for naught).
This changes behavior, because now "--blockdev inv-drv --help" will
print a help text instead of complaining about the --blockdev
argument.
Note that this is similar in behavior to other tools, though: "--help"
is generally immediately acted upon when finding it in the argument
list, potentially before other arguments (even ones before it) are
acted on. For example, "ls /does-not-exist --help" prints a help text
and does not complain about ENOENT.
- --pidfile does not need initialization, and is already exempted from
the sequential order that process_options() claims to strictly follow
(the PID file is only created after all arguments are processed, not
at the time the --pidfile argument appears), so it makes sense to
include it in the same category as --daemonize.
- Invalid arguments should always be reported as soon as possible. (The
same caveat with --help applies: That means that "--blockdev inv-drv
--inv-arg" will now complain about --inv-arg, not inv-drv.)
Note that we could decide to check only for --daemonize in the first
pass, and defer --help, --version, and checking for invalid arguments to
the second one, thus largely keeping our current behavior. However,
this would break "--help --daemonize": The child would print the help
text to stdout, which is redirected to /dev/null, and so the text would
disappear. We would need to have the text be printed to stderr instead,
and this would then make the parent process exit with EXIT_FAILURE,
which is probably not what we want for --help.
This patch does make some references to --daemonize without having
implemented it yet, but that will happen in the next patch.
Signed-off-by: Hanna Reitz
---
storage-daemon/qemu-storage-daemon.c | 37 ++--
1 file changed, 35 insertions(+), 2 deletions(-)
diff --git a/storage-daemon/qemu-storage-daemon.c
b/storage-daemon/qemu-storage-daemon.c
index 52cf17e8ac..42a52d3b1c 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -164,7 +164,23 @@ static int getopt_set_loc(int argc, char **argv, const
char *optstring,
return c;
}
-static void process_options(int argc, char *argv[])
+/**
+ * Process QSD command-line arguments.
+ *
+ * This is done in two passes:
+ *
+ * First (@pre_init_pass is true), we do a pass where all global
+ * arguments pertaining to the QSD process (like --help or --daemonize)
+ * are processed. This pass is done before most of the QEMU-specific
+ * initialization steps (e.g. initializing the block layer or QMP), and
+ * so must only process arguments that are not really QEMU-specific.
+ *
+ * Second (@pre_init_pass is false), we (sequentially) process all
+ * QEMU/QSD-specific arguments. Many of these arguments are effectively
+ * translated to QMP commands (like --blockdev for blockdev-add, or
+ * --export for block-export-add).
+ */
+static void process_options(int argc, char *argv[], bool pre_init_pass)
{
int c;
@@ -187,7 +203,22 @@ static void process_options(int argc, char *argv[])
* they are given on the command lines. This means that things must be
* defined first before they can be referenced in another option.
*/
+optind = 1;
while ((c = getopt_set_loc(argc, argv, "-hT:V", long_options)) != -1) {
+bool handle_option_pre_init;
+
+/* Should this argument be processed in the pre-init pass? */
+handle_option_pre_init =
+c == '?' ||
+c == 'h' ||
+c == 'V' ||
+c == OPTION_PIDFILE;
+
+/* Process every option only in its respective pass */
+if (pre_init_pass != handle_option_pre_init) {
+continue;
+}
+
switch (c) {
case '?':
exit(EXIT_FAILURE);
@@ -321,6 +352,8 @@ int main(int argc, char *argv[])
qemu_init_exec_dir(argv[0]);
os_setup_signal_handling();
+process_options(argc, argv, true);
+
module_call_init(MODULE_INIT_QOM);
module_call_init(MODULE_INIT_TRACE);
qemu_add_opts(_trace_opts);
@@ -335,7 +368,7 @@ int main(int argc, char *argv[])
qemu_set_log(LOG_TRACE);
qemu_init_main_loop(_fatal);
-process_options(argc, argv);
+process_options(argc, argv, false);
/*
* Write the pid file after creating
