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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.martins@xxxxxxxxxx>
> >> ---
> >>  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@xxxxxxxxxx>
> >> + */
> >> +
> >> +
> >> +#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 - it is bad practice to move and
rename code at the same time, as it makes diffs harder to review.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux