On 03/03/2017 11:35 AM, Daniel P. Berrange wrote: > On Fri, Mar 03, 2017 at 11:36:25AM +0000, Joao Martins wrote: >> >> >> 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.mart...@oracle.com> >>>> --- >>>> 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 <berra...@redhat.com> >>>> + */ >>>> + >>>> + >>>> +#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. > > It would be desirable to use the vir name prefix here, but that should > certainly be done as a separate patch BTW I said follow-up when I actually meant it in the context of this series i.e. patch 2/5 would rename these functions to be prefixed by vir.
> - it is bad practice to move and > rename code at the same time, as it makes diffs harder to review. /nods Joao -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list