| Message ID | 1337631598-30639-3-git-send-email-coreyb@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 05/21/2012 02:19 PM, Corey Bryant wrote: > This patch provides support for the getfd_file monitor command. > This command will allow passing of a filename and its corresponding > file descriptor to a guest via the monitor. This command could be > followed, for example, by a drive_add command to hot attach a disk > drive. > > Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com> Is the only difference between 'getfd' and 'getfd_file' the fact that 'getfd' introduces an abstract namespace usable only by the fd: protocol, while the 'getfd_file' introduces a name identical to the absolute naming of the file system and usable by the file: protocol? What happens if I pass 'getfd_file' a relative file name? Must the filename passed to 'getfd_file' be in canonical form, or may it contain symlinks, .., and other non-canonical constructs? Can the 'closefd' command be used to close the fd originally given to qemu via 'getfd_file'?
On Mon, May 21, 2012 at 9:19 PM, Corey Bryant <coreyb@linux.vnet.ibm.com> wrote: I think Eric has raised the main questions about duplicating getfd and rules regarding canonical file names (QEMU mashes filenames together if the backing filename is relative!). > + if (qemu_isdigit(filename[0])) { > + qerror_report(QERR_INVALID_PARAMETER_VALUE, "filename", > + "a name not starting with a digit"); > + return -1; > + } What is the reason for this filename restriction? > diff --git a/qmp-commands.hx b/qmp-commands.hx > index db980fa..1825a91 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -891,6 +891,36 @@ Example: > EQMP > > { > + .name = "getfd_file", > + .args_type = "filename:s", > + .params = "getfd_file filename", > + .help = "receive a file descriptor via SCM rights and assign it a filename", > + .user_print = monitor_user_noop, > + .mhandler.cmd_new = do_getfd_file, > + }, > + > + > +SQMP > + > +getfd_file > +-------------- > + > +Receive a file descriptor via SCM rights and assign it a filename. > + > +Arguments: > + > +- "filename": filename (json-string) > + > +Example: > + > +-> { "execute": "getfd_file", > + "arguments": { "filename": "/var/lib/libvirt/images/tst.img" } } > +<- { "return": {} } > + > + > +EQMP QMP commands should be added to qapi-schema.json as described in docs/writing-qmp-commands.txt. Stefan
On 05/21/2012 05:48 PM, Eric Blake wrote: > On 05/21/2012 02:19 PM, Corey Bryant wrote: >> This patch provides support for the getfd_file monitor command. >> This command will allow passing of a filename and its corresponding >> file descriptor to a guest via the monitor. This command could be >> followed, for example, by a drive_add command to hot attach a disk >> drive. >> >> Signed-off-by: Corey Bryant<coreyb@linux.vnet.ibm.com> > > Is the only difference between 'getfd' and 'getfd_file' the fact that > 'getfd' introduces an abstract namespace usable only by the fd: > protocol, while the 'getfd_file' introduces a name identical to the > absolute naming of the file system and usable by the file: protocol? The only difference is that getfd passes an fdname to associate to the fd, and getfd_file passes a filename to associate to the fd. These name/fd pairs are stored separately so there won't be any conflicts (ie. fdname == filename). > What happens if I pass 'getfd_file' a relative file name? Must the > filename passed to 'getfd_file' be in canonical form, or may it contain > symlinks, .., and other non-canonical constructs? As the code is now, the 'getfd_file' filename has to be the same as the 'drive_add' filename, for example. And the same goes for the '-drive' filename and the '-filfd' filename. I didn't introduce any special handling to canonicalize the filenames, but I think it is necessary. Either in QEMU or libvirt, but it probably makes more sense to canonicalize in QEMU. > > Can the 'closefd' command be used to close the fd originally given to > qemu via 'getfd_file'? > No, 'closefd' won't close an fd passed in by 'getfd_file'. I was thinking I should probably add a 'closefd_file' that could do this.
On 05/22/2012 05:18 AM, Stefan Hajnoczi wrote: > On Mon, May 21, 2012 at 9:19 PM, Corey Bryant<coreyb@linux.vnet.ibm.com> wrote: > I think Eric has raised the main questions about duplicating getfd and > rules regarding canonical file names (QEMU mashes filenames together > if the backing filename is relative!). > Ok, good so it sounds like we this covered in the other threads then. >> + if (qemu_isdigit(filename[0])) { >> + qerror_report(QERR_INVALID_PARAMETER_VALUE, "filename", >> + "a name not starting with a digit"); >> + return -1; >> + } > > What is the reason for this filename restriction? > The reason is that I copied this from 'getfd'. :) I'll remove it. >> diff --git a/qmp-commands.hx b/qmp-commands.hx >> index db980fa..1825a91 100644 >> --- a/qmp-commands.hx >> +++ b/qmp-commands.hx >> @@ -891,6 +891,36 @@ Example: >> EQMP >> >> { >> + .name = "getfd_file", >> + .args_type = "filename:s", >> + .params = "getfd_file filename", >> + .help = "receive a file descriptor via SCM rights and assign it a filename", >> + .user_print = monitor_user_noop, >> + .mhandler.cmd_new = do_getfd_file, >> + }, >> + >> + >> +SQMP >> + >> +getfd_file >> +-------------- >> + >> +Receive a file descriptor via SCM rights and assign it a filename. >> + >> +Arguments: >> + >> +- "filename": filename (json-string) >> + >> +Example: >> + >> +-> { "execute": "getfd_file", >> + "arguments": { "filename": "/var/lib/libvirt/images/tst.img" } } >> +<- { "return": {} } >> + >> + >> +EQMP > > QMP commands should be added to qapi-schema.json as described in > docs/writing-qmp-commands.txt. > > Stefan > Ok thanks!
On Tue, 22 May 2012 10:18:22 +0100 Stefan Hajnoczi <stefanha@gmail.com> wrote: > QMP commands should be added to qapi-schema.json as described in > docs/writing-qmp-commands.txt. Looks like there's consensus on dropping this patch and enhancing getfd to return the fd number. This would require to first convert getfd from plain HMP to the QAPI, which is basically to say more or less the same thing as Stefan said above (you could also look for examples searching for "qapi: convert" in the git log). But there's a small problem. Today getfd commands are closely tied to the Monitor. In Anthony's development tree, the getfd commands are tied to the new QMP server's session support. Asking you to integrate the new QMP server only to have the getfd command returning a simple integer would be too much, but at the same time I think you'll have to at least to break it from the monitor. This means moving its data structure away from the Monitor object and probably reworking the internal API used to get fds (ie. monitor_get_fd()). Shouldn't be hard, but you should be careful not to break external users.
On 05/22/2012 03:06 PM, Luiz Capitulino wrote: > On Tue, 22 May 2012 10:18:22 +0100 > Stefan Hajnoczi<stefanha@gmail.com> wrote: > >> QMP commands should be added to qapi-schema.json as described in >> docs/writing-qmp-commands.txt. > > Looks like there's consensus on dropping this patch and enhancing getfd > to return the fd number. This would require to first convert getfd from > plain HMP to the QAPI, which is basically to say more or less the same > thing as Stefan said above (you could also look for examples searching > for "qapi: convert" in the git log). Yep, ok thanks. > > But there's a small problem. Today getfd commands are closely tied to the > Monitor. In Anthony's development tree, the getfd commands are tied to the > new QMP server's session support. > > Asking you to integrate the new QMP server only to have the getfd command > returning a simple integer would be too much, but at the same time I think > you'll have to at least to break it from the monitor. This means moving its > data structure away from the Monitor object and probably reworking the > internal API used to get fds (ie. monitor_get_fd()). > > Shouldn't be hard, but you should be careful not to break external users. > Just to verify, are you talking about moving the "fds" off the Monitor struct? --> QLIST_HEAD(,mon_fd_t) fds; Was this already moved away from the Monitor struct in Anthony's development tree? If not do you have a recommendation on where to move it? I think this would make more sense to me if I took a look at the getfd code in Anthony's development tree. Is this the correct tree? I had some issues cloning it. https://github.com/aliguori/qemu-next.git
On Tue, 22 May 2012 16:02:19 -0400 Corey Bryant <coreyb@linux.vnet.ibm.com> wrote: > > But there's a small problem. Today getfd commands are closely tied to the > > Monitor. In Anthony's development tree, the getfd commands are tied to the > > new QMP server's session support. > > > > Asking you to integrate the new QMP server only to have the getfd command > > returning a simple integer would be too much, but at the same time I think > > you'll have to at least to break it from the monitor. This means moving its > > data structure away from the Monitor object and probably reworking the > > internal API used to get fds (ie. monitor_get_fd()). > > > > Shouldn't be hard, but you should be careful not to break external users. > > > > Just to verify, are you talking about moving the "fds" off the Monitor > struct? --> QLIST_HEAD(,mon_fd_t) fds; Yes. > Was this already moved away from the Monitor struct in Anthony's > development tree? If not do you have a recommendation on where to move it? Yes, iirc it moved inside the new QMP server session support in Anthony's tree. > I think this would make more sense to me if I took a look at the getfd > code in Anthony's development tree. Is this the correct tree? I had > some issues cloning it. https://github.com/aliguori/qemu-next.git The 'development' tree I'm referring to is the old glib branch in git://repo.or.cz/qemu/aliguori.git.
On 05/22/2012 04:26 PM, Luiz Capitulino wrote: > On Tue, 22 May 2012 16:02:19 -0400 > Corey Bryant<coreyb@linux.vnet.ibm.com> wrote: > >>> But there's a small problem. Today getfd commands are closely tied to the >>> Monitor. In Anthony's development tree, the getfd commands are tied to the >>> new QMP server's session support. >>> >>> Asking you to integrate the new QMP server only to have the getfd command >>> returning a simple integer would be too much, but at the same time I think >>> you'll have to at least to break it from the monitor. This means moving its >>> data structure away from the Monitor object and probably reworking the >>> internal API used to get fds (ie. monitor_get_fd()). >>> >>> Shouldn't be hard, but you should be careful not to break external users. >>> >> >> Just to verify, are you talking about moving the "fds" off the Monitor >> struct? --> QLIST_HEAD(,mon_fd_t) fds; > > Yes. > >> Was this already moved away from the Monitor struct in Anthony's >> development tree? If not do you have a recommendation on where to move it? > > Yes, iirc it moved inside the new QMP server session support in Anthony's tree. > >> I think this would make more sense to me if I took a look at the getfd >> code in Anthony's development tree. Is this the correct tree? I had >> some issues cloning it. https://github.com/aliguori/qemu-next.git > > The 'development' tree I'm referring to is the old glib branch in > git://repo.or.cz/qemu/aliguori.git. > Hmm, it looks like fds is still on the Monitor struct in that branch. I'll do some more searching later unless you have anything else you can point me to.
On Tue, 22 May 2012 18:34:19 -0400 Corey Bryant <coreyb@linux.vnet.ibm.com> wrote: > On 05/22/2012 04:26 PM, Luiz Capitulino wrote: > > On Tue, 22 May 2012 16:02:19 -0400 > > Corey Bryant<coreyb@linux.vnet.ibm.com> wrote: > > > >>> But there's a small problem. Today getfd commands are closely tied to the > >>> Monitor. In Anthony's development tree, the getfd commands are tied to the > >>> new QMP server's session support. > >>> > >>> Asking you to integrate the new QMP server only to have the getfd command > >>> returning a simple integer would be too much, but at the same time I think > >>> you'll have to at least to break it from the monitor. This means moving its > >>> data structure away from the Monitor object and probably reworking the > >>> internal API used to get fds (ie. monitor_get_fd()). > >>> > >>> Shouldn't be hard, but you should be careful not to break external users. > >>> > >> > >> Just to verify, are you talking about moving the "fds" off the Monitor > >> struct? --> QLIST_HEAD(,mon_fd_t) fds; > > > > Yes. > > > >> Was this already moved away from the Monitor struct in Anthony's > >> development tree? If not do you have a recommendation on where to move it? > > > > Yes, iirc it moved inside the new QMP server session support in Anthony's tree. > > > >> I think this would make more sense to me if I took a look at the getfd > >> code in Anthony's development tree. Is this the correct tree? I had > >> some issues cloning it. https://github.com/aliguori/qemu-next.git > > > > The 'development' tree I'm referring to is the old glib branch in > > git://repo.or.cz/qemu/aliguori.git. > > > > Hmm, it looks like fds is still on the Monitor struct in that branch. Oh, you're right. That code is unfinished. It seems that I kept the finished version in my mind. Well, I don't think that moving the fd array to another object will buy us much, so you can keep it this way. Note that you still have to convert do_getfd() to the QAPI as pointed out by Stefan and that the monitor object (*mon pointer) won't be passed to it. You'll have to use cur_mon in qmp_getfd() (as Anthony's version does).
On 05/23/2012 09:33 AM, Luiz Capitulino wrote: > On Tue, 22 May 2012 18:34:19 -0400 > Corey Bryant<coreyb@linux.vnet.ibm.com> wrote: > >> On 05/22/2012 04:26 PM, Luiz Capitulino wrote: >>> On Tue, 22 May 2012 16:02:19 -0400 >>> Corey Bryant<coreyb@linux.vnet.ibm.com> wrote: >>> >>>>> But there's a small problem. Today getfd commands are closely tied to the >>>>> Monitor. In Anthony's development tree, the getfd commands are tied to the >>>>> new QMP server's session support. >>>>> >>>>> Asking you to integrate the new QMP server only to have the getfd command >>>>> returning a simple integer would be too much, but at the same time I think >>>>> you'll have to at least to break it from the monitor. This means moving its >>>>> data structure away from the Monitor object and probably reworking the >>>>> internal API used to get fds (ie. monitor_get_fd()). >>>>> >>>>> Shouldn't be hard, but you should be careful not to break external users. >>>>> >>>> >>>> Just to verify, are you talking about moving the "fds" off the Monitor >>>> struct? --> QLIST_HEAD(,mon_fd_t) fds; >>> >>> Yes. >>> >>>> Was this already moved away from the Monitor struct in Anthony's >>>> development tree? If not do you have a recommendation on where to move it? >>> >>> Yes, iirc it moved inside the new QMP server session support in Anthony's tree. >>> >>>> I think this would make more sense to me if I took a look at the getfd >>>> code in Anthony's development tree. Is this the correct tree? I had >>>> some issues cloning it. https://github.com/aliguori/qemu-next.git >>> >>> The 'development' tree I'm referring to is the old glib branch in >>> git://repo.or.cz/qemu/aliguori.git. >>> >> >> Hmm, it looks like fds is still on the Monitor struct in that branch. > > Oh, you're right. That code is unfinished. It seems that I kept the finished > version in my mind. Oh how I wish I could git clone some people's brains. :) > > Well, I don't think that moving the fd array to another object will buy us > much, so you can keep it this way. Note that you still have to convert do_getfd() > to the QAPI as pointed out by Stefan and that the monitor object (*mon pointer) > won't be passed to it. You'll have to use cur_mon in qmp_getfd() (as Anthony's > version does). > Alright, thanks for the info.
diff --git a/hmp-commands.hx b/hmp-commands.hx index 18cb415..9cd5ed1 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -1240,6 +1240,23 @@ used by another monitor command. ETEXI { + .name = "getfd_file", + .args_type = "filename:s", + .params = "getfd_file filename", + .help = "receive a file descriptor via SCM rights and assign it a filename", + .user_print = monitor_user_noop, + .mhandler.cmd_new = do_getfd_file, + }, + +STEXI +@item getfd_file @var{filename} +@findex getfd_file +If a file descriptor is passed alongside this command using the SCM_RIGHTS +mechanism on unix sockets, it is stored using the name @var{filename} for +later use by other monitor commands. +ETEXI + + { .name = "block_passwd", .args_type = "device:B,password:s", .params = "block_passwd device password", diff --git a/monitor.c b/monitor.c index 12a6fe2..bdf4dd8 100644 --- a/monitor.c +++ b/monitor.c @@ -163,6 +163,7 @@ struct Monitor { #endif QError *error; QLIST_HEAD(,mon_fd_t) fds; + QLIST_HEAD(,mon_fd_t) file_fds; QLIST_ENTRY(Monitor) entry; }; @@ -2256,6 +2257,42 @@ static int do_closefd(Monitor *mon, const QDict *qdict, QObject **ret_data) return -1; } +static int do_getfd_file(Monitor *mon, const QDict *qdict, QObject **ret_data) +{ + const char *filename = qdict_get_str(qdict, "filename"); + mon_fd_t *monfd; + int fd; + + fd = qemu_chr_fe_get_msgfd(mon->chr); + if (fd == -1) { + qerror_report(QERR_FD_NOT_SUPPLIED); + return -1; + } + + if (qemu_isdigit(filename[0])) { + qerror_report(QERR_INVALID_PARAMETER_VALUE, "filename", + "a name not starting with a digit"); + return -1; + } + + QLIST_FOREACH(monfd, &mon->file_fds, next) { + if (strcmp(monfd->name, filename) != 0) { + continue; + } + + close(monfd->fd); + monfd->fd = fd; + return 0; + } + + monfd = g_malloc0(sizeof(mon_fd_t)); + monfd->name = g_strdup(filename); + monfd->fd = fd; + + QLIST_INSERT_HEAD(&mon->file_fds, monfd, next); + return 0; +} + static void do_loadvm(Monitor *mon, const QDict *qdict) { int saved_vm_running = runstate_is_running(); @@ -2292,6 +2329,39 @@ int monitor_get_fd(Monitor *mon, const char *fdname) return -1; } +int monitor_get_fd_file(Monitor *mon, const char *filename, + bool take_ownership) +{ + mon_fd_t *monfd; + + QLIST_FOREACH(monfd, &mon->file_fds, next) { + int fd; + + if (strcmp(monfd->name, filename) != 0) { + continue; + } + + fd = monfd->fd; + + if (take_ownership) { + /* caller takes ownership of fd */ + QLIST_REMOVE(monfd, next); + g_free(monfd->name); + g_free(monfd); + } + + return fd; + } + + return -1; +} + +int qemu_get_fd_file(const char *filename, bool take_ownership) +{ + return cur_mon ? + monitor_get_fd_file(cur_mon, filename, take_ownership) : -1; +} + /* mon_cmds and info_cmds would be sorted at runtime */ static mon_cmd_t mon_cmds[] = { #include "hmp-commands.h" diff --git a/monitor.h b/monitor.h index 0d49800..529d8a7 100644 --- a/monitor.h +++ b/monitor.h @@ -60,6 +60,9 @@ int monitor_read_block_device_key(Monitor *mon, const char *device, void *opaque); int monitor_get_fd(Monitor *mon, const char *fdname); +int monitor_get_fd_file(Monitor *mon, const char *filename, + bool take_ownership); +int qemu_get_fd_file(const char *filename, bool take_ownership); void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap) GCC_FMT_ATTR(2, 0); diff --git a/qemu-tool.c b/qemu-tool.c index 07fc4f2..d3d86bf 100644 --- a/qemu-tool.c +++ b/qemu-tool.c @@ -111,3 +111,8 @@ void migrate_add_blocker(Error *reason) void migrate_del_blocker(Error *reason) { } + +int qemu_get_fd_file(const char *fdname, bool take_ownership) +{ + return -1; +} diff --git a/qmp-commands.hx b/qmp-commands.hx index db980fa..1825a91 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -891,6 +891,36 @@ Example: EQMP { + .name = "getfd_file", + .args_type = "filename:s", + .params = "getfd_file filename", + .help = "receive a file descriptor via SCM rights and assign it a filename", + .user_print = monitor_user_noop, + .mhandler.cmd_new = do_getfd_file, + }, + + +SQMP + +getfd_file +-------------- + +Receive a file descriptor via SCM rights and assign it a filename. + +Arguments: + +- "filename": filename (json-string) + +Example: + +-> { "execute": "getfd_file", + "arguments": { "filename": "/var/lib/libvirt/images/tst.img" } } +<- { "return": {} } + + +EQMP + + { .name = "block_passwd", .args_type = "device:B,password:s", .mhandler.cmd_new = qmp_marshal_input_block_passwd,
This patch provides support for the getfd_file monitor command. This command will allow passing of a filename and its corresponding file descriptor to a guest via the monitor. This command could be followed, for example, by a drive_add command to hot attach a disk drive. Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com> --- hmp-commands.hx | 17 +++++++++++++ monitor.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ monitor.h | 3 ++ qemu-tool.c | 5 ++++ qmp-commands.hx | 30 +++++++++++++++++++++++ 5 files changed, 125 insertions(+), 0 deletions(-)