Re: [libvirt] [PATCH RFC 1/4] qemu_agent: move agent into util

On 03/02/2017 05:58 PM, Jim Fehlig wrote: > Sorry for the review delay. > > On 02/08/2017 09:44 AM, Joao Martins wrote: >> As it could be shared with libxl which now allows channels to >> be created. Also changed filename to match others in the same >> directory namely to virqemuagent.{h,c} >> >> Signed-off-by: Joao Martins <joao.m.martins(a)oracle.com&gt; >> --- >> po/POTFILES.in | 2 +- >> src/Makefile.am | 2 +- >> src/libvirt_private.syms | 21 + >> src/qemu/qemu_agent.c | 2248 ------------------------------------------ >> src/qemu/qemu_agent.h | 123 --- >> src/qemu/qemu_domain.h | 2 +- >> src/qemu/qemu_driver.c | 2 +- >> src/util/virqemuagent.c | 2248 ++++++++++++++++++++++++++++++++++++++++++ >> src/util/virqemuagent.h | 123 +++ >> tests/qemuagenttest.c | 2 +- >> tests/qemumonitortestutils.c | 2 +- >> tests/qemumonitortestutils.h | 2 +- >> 12 files changed, 2399 insertions(+), 2378 deletions(-) >> delete mode 100644 src/qemu/qemu_agent.c >> delete mode 100644 src/qemu/qemu_agent.h >> create mode 100644 src/util/virqemuagent.c >> create mode 100644 src/util/virqemuagent.h > > I hope others will opine on this change. It seems reasonable to me and I'm > surprised the qemu driver only needed tiny changes to accommodate moving all > this code. Yeap, I too was surprised on how the changes were non disruptive wrt to qemu driver :) > > [...] >> diff --git a/src/util/virqemuagent.h b/src/util/virqemuagent.h >> new file mode 100644 >> index 0000000..2e81020 >> --- /dev/null >> +++ b/src/util/virqemuagent.h >> @@ -0,0 +1,123 @@ >> +/* >> + * virqemuagent.h: interaction with QEMU guest agent >> + * >> + * Copyright (C) 2006-2012 Red Hat, Inc. >> + * Copyright (C) 2006 Daniel P. Berrange >> + * >> + * This library is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU Lesser General Public >> + * License as published by the Free Software Foundation; either >> + * version 2.1 of the License, or (at your option) any later version. >> + * >> + * This library is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + * Lesser General Public License for more details. >> + * >> + * You should have received a copy of the GNU Lesser General Public >> + * License along with this library. If not, see >> + * <http://www.gnu.org/licenses/>;. >> + * >> + * Author: Daniel P. Berrange <berrange(a)redhat.com&gt; >> + */ >> + >> + >> +#ifndef __QEMU_AGENT_H__ >> +# define __QEMU_AGENT_H__ >> + >> +# include "internal.h" >> +# include "domain_conf.h" > > ISTR a recent discussion on the list frowning on using code from src/conf in > src/util, although virthostdev.h also includes domain_conf.h. > > BTW, compilation fails here > > In file included from util/virqemuagent.c:35:0: > util/virqemuagent.h:29:26: fatal error: domain_conf.h: No such file or directory > # include "domain_conf.h" > Hmm, will have to fix it for v1. This series were applied on top of commit 2841e67 ("qemu: propagate bridge MTU into qemu "host_mtu" option") and I didn't observe compilation issues (neither make {syntax-check,check} IIRC). >> + >> +typedef struct _qemuAgent qemuAgent; >> +typedef qemuAgent *qemuAgentPtr; >> + >> +typedef struct _qemuAgentCallbacks qemuAgentCallbacks; >> +typedef qemuAgentCallbacks *qemuAgentCallbacksPtr; >> +struct _qemuAgentCallbacks { >> + void (*destroy)(qemuAgentPtr mon, >> + virDomainObjPtr vm); >> + void (*eofNotify)(qemuAgentPtr mon, >> + virDomainObjPtr vm); >> + void (*errorNotify)(qemuAgentPtr mon, >> + virDomainObjPtr vm); >> +}; >> + >> + >> +qemuAgentPtr qemuAgentOpen(virDomainObjPtr vm, >> + const virDomainChrSourceDef *config, >> + qemuAgentCallbacksPtr cb); >> + >> +void qemuAgentClose(qemuAgentPtr mon); > > Other files in src/util prefix structs and functions with "vir". I'm not sure > how picky folks are about that. If the consensus is towards the "vir" prefix, > perhaps it would be easier done with a follow-up after the move? Yeah I noticed the pattern. Though given the big move of code into util making as a follow-up patch would easy review perhaps.