Re: [RFC PATCH 1/2] spapr: Report correct GTSE support via ov5

[Aneesh Kumar K.V]

David Gibson  writes:

> On Mon, Mar 14, 2022 at 07:10:10PM -0300, Fabiano Rosas wrote:
>> David Gibson  writes:
>> 
>> > On Tue, Mar 08, 2022 at 10:23:59PM -0300, Fabiano Rosas wrote:
>>

...

>> To satisfy TCG we could keep a spapr capability as ON and usually the
>> guest would pass cap-gtse=off when running with KVM. However this
>> doesn't work because this crash happens precisely because the nested
>> guest doesn't know that it needs to use cap-rpt-invalidate=on. Another
>> cap wouldn't help.
>> 
>> So I think the only way to have a spapr capability for this is if TCG
>> always defaults to ON and KVM always defaults to OFF. But then we would
>> be changing guest visible behaviour depending on host properties.
>
> Ok, I'd forgotten we already have cap-rpt-invalidate.  It still
> defaults to OFF for now, which might help us.
>
> What's clear is that we should never disable GTSE if
> cap-rpt-invalidate is off - qemu should enforce that before even
> starting the guest if at all possible.
>
> What's less clear to me is if we want to enable GTSE by default or
> not, in the cases where we're able to choose.  Would always disabling
> GTSE when cap-rpt-invalidate=on be ok?  Or do we want to be able to
> control GTSE separately.  In that case we might need a second cap, but
> it would need inverted sense, so e.g. cap-disable-gtse.


GTSE and cap-rpt-invalidate can be looked at as independent such that we
can do GTSE=1 or GTSE=0 with cap-rpt-invalidate=on. But GTSE=0 with
cap-rpt-invalidate=off is not allowed/possible. GTSE value is what is
negotiated via CAS so we should let the hypervisor inform the guest whether it
can do GTSE 0 or 1. The challenge IIUC is Qemu always assumed GTSE=1
which is not true in the case of nested virt where L1 guest that is booted
with GTSE=0.

with cap-disable-gtse how would one interpret that? Whether hypervisor
have the capability to disable gtse? 

>
> I believe a guest that is expecting GTSE==0 should work if
> LPCR[GTSE]==1, just not optimally (as long as H_RPT_INVALIDATE is
> still available, of course).  Is that right?

That is correct.

-aneesh

Re: [PATCH] spapr_numa.c: FORM2 table handle nodes with no distance info

[Aneesh Kumar K.V]

Daniel Henrique Barboza  writes:

> On 11/5/21 10:51, Nicholas Piggin wrote:
>> A configuration that specifies multiple nodes without distance info
>> results in the non-local points in the FORM2 matrix having a distance of
>> 0. This causes Linux to complain "Invalid distance value range" because
>> a node distance is smaller than the local distance.
>> 
>> Fix this by building a simple local / remote fallback for points where
>> distance information is missing.
>
> Thanks for looking this up. I checked the output of this same scenario with
> a FORM1 guest and 4 distance-less NUMA nodes. This is what I got:
>
> [root@localhost ~]# numactl -H
> available: 4 nodes (0-3)
> (...)
> node distances:
> node   0   1   2   3
>0:  10  160  160  160
>1:  160  10  160  160
>2:  160  160  10  160
>3:  160  160  160  10
> [root@localhost ~]#
>
>
> With this patch we're getting '20' instead of '160' because you're using
> NUMA_DISTANCE_DEFAULT, while FORM1 will default this case to the maximum
> NUMA distance the kernel allows for that affinity (160).

where is that enforced? Do we know why FORM1 picked 160? 

>
> I do not have strong feelings about changing this behavior between FORM1 and
> FORM2. I tested the same scenario with a x86_64 guest and they also uses '20'
> in this case as well, so far as QEMU goes using NUMA_DISTANCE_DEFAULT is
> consistent.
>

for FORM2 I would suggest 20 is the right value and it is also
consistent with other architectures. 

> Aneesh is already in CC, so I believe he'll let us know if there's something
> we're missing and we need to preserve the '160' distance in FORM2 for this
> case as well.
>
> For now:
>
>
>> 
>> Signed-off-by: Nicholas Piggin 
>> ---
>
>
> Reviewed-by: Daniel Henrique Barboza 
>
>
>
>>   hw/ppc/spapr_numa.c | 22 +-
>>   1 file changed, 17 insertions(+), 5 deletions(-)
>> 
>> diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
>> index 5822938448..56ab2a5fb6 100644
>> --- a/hw/ppc/spapr_numa.c
>> +++ b/hw/ppc/spapr_numa.c
>> @@ -546,12 +546,24 @@ static void 
>> spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
>>* NUMA nodes, but QEMU adds the default NUMA node without
>>* adding the numa_info to retrieve distance info from.
>>*/
>> -if (src == dst) {
>> -distance_table[i++] = NUMA_DISTANCE_MIN;
>> -continue;

We always initialized the local distance to be NUMA_DISTANCE_MIN
irrespective of what is specified via Qemu command line before? If so
then the above change will break that? 

>> +distance_table[i] = numa_info[src].distance[dst];
>> +if (distance_table[i] == 0) {

we know distance_table[i] is == 0 here and ..

>> +/*
>> + * In case QEMU adds a default NUMA single node when the 
>> user
>> + * did not add any, or where the user did not supply 
>> distances,
>> + * the value will be 0 here. Populate the table with a 
>> fallback
>> + * simple local / remote distance.
>> + */
>> +if (src == dst) {
>> +distance_table[i] = NUMA_DISTANCE_MIN;
>> +} else {
>> +distance_table[i] = numa_info[src].distance[dst];
>> +if (distance_table[i] < NUMA_DISTANCE_MIN) {


considering we reached here after checking distance_table[i] == 0 do we
need to do the above two lines?

>> +distance_table[i] = NUMA_DISTANCE_DEFAULT;
>> +}
>> +}
>>   }
>> -
>> -distance_table[i++] = numa_info[src].distance[dst];
>> +i++;
>>   }
>>   }

Re: [RFC PATCH 6/8] nvdimm: add PPC64 'device-node' property

[Aneesh Kumar K.V]

Daniel Henrique Barboza  writes:

> The spapr-nvdimm driver is able to operate in two ways. The first
> one is as a regular memory in which the NUMA node of the parent
> pc-dimm class is used. The second mode, as persistent memory, allows for
> a different NUMA node to be used based on the locality of the device.
>
> At this moment we don't have a way to express this second NUMA node for
> the persistent memory mode. This patch introduces a new 'device-node'
> property that will be used by the PPC64 spapr-nvdimm driver to set a
> second NUMA node for the nvdimm.

if device-node is not specified on the commandline, we should default to
the same value of node= attribute? Right now I am finding this default
to 0.

-aneesh

Re: [PATCH v4 0/3] nvdimm: Enable sync-dax property for nvdimm

[Aneesh Kumar K.V]

On 5/4/21 11:13 AM, Pankaj Gupta wrote:




What this patch series did was to express that property via a device
tree node and guest driver enables a hypercall based flush mechanism to
ensure persistence.


Would VIRTIO (entirely asynchronous, no trap at host side) based
mechanism is better
than hyper-call based? Registering memory can be done any way. We
implemented virtio-pmem
flush mechanisms with below considerations:

- Proper semantic for guest flush requests.
- Efficient mechanism for performance pov.



sure, virio-pmem can be used as an alternative.


I am just asking myself if we have platform agnostic mechanism already
there, maybe
we can extend it to suit our needs? Maybe I am missing some points here.



What is being attempted in this series is to indicate to the guest OS 
that the backing device/file used for emulated nvdimm device cannot 
guarantee the persistence via cpu cache flush instructions.




On PPC, the default is "sync-dax=writeback" - so the ND_REGION_ASYNC

is set for the region and the guest makes hcalls to issue fsync on the host.


Are you suggesting me to keep it "unsafe" as default for all architectures

including PPC and a user can set it to "writeback" if desired.


No, I am suggesting that "sync-dax" is insufficient to convey this
property. This behavior warrants its own device type, not an ambiguous
property of the memory-backend-file with implicit architecture
assumptions attached.



Why is it insufficient?  Is it because other architectures don't have an
ability express this detail to guest OS? Isn't that an arch limitations?


-aneesh

Re: [PATCH v4 0/3] nvdimm: Enable sync-dax property for nvdimm

[Aneesh Kumar K.V]

On 5/4/21 1:11 AM, Dan Williams wrote:

On Mon, May 3, 2021 at 7:06 AM Shivaprasad G Bhat  wrote:





.



The proposal that "sync-dax=unsafe" for non-PPC architectures, is a
fundamental misrepresentation of how this is supposed to work. Rather
than make "sync-dax" a first class citizen of the device-description
interface I'm proposing that you make this a separate device-type.
This also solves the problem that "sync-dax" with an implicit
architecture backend assumption is not precise, but a new "non-nvdimm"
device type would make it explicit what the host is advertising to the
guest.



Currently, users can use a virtualized nvdimm support in Qemu to share 
host page cache to the guest via the below command


-object memory-backend-file,id=memnvdimm1,mem-path=file_name_in_host_fs
-device nvdimm,memdev=memnvdimm1

Such usage can results in wrong application behavior because there is no 
hint to the application/guest OS that a cpu cache flush is not 
sufficient to ensure persistence.


I understand that virio-pmem is suggested as an alternative for that. 
But why not fix virtualized nvdimm if platforms can express the details.


ie, can ACPI indicate to the guest OS that the device need a flush 
mechanism to ensure persistence in the above case?


What this patch series did was to express that property via a device 
tree node and guest driver enables a hypercall based flush mechanism to 
ensure persistence.







On PPC, the default is "sync-dax=writeback" - so the ND_REGION_ASYNC

is set for the region and the guest makes hcalls to issue fsync on the host.


Are you suggesting me to keep it "unsafe" as default for all architectures

including PPC and a user can set it to "writeback" if desired.


No, I am suggesting that "sync-dax" is insufficient to convey this
property. This behavior warrants its own device type, not an ambiguous
property of the memory-backend-file with implicit architecture
assumptions attached.



Why is it insufficient?  Is it because other architectures don't have an 
ability express this detail to guest OS? Isn't that an arch limitations?


-aneesh

Re: [PATCH v4 0/3] nvdimm: Enable sync-dax property for nvdimm

[Aneesh Kumar K.V]

On 5/1/21 12:44 AM, Dan Williams wrote:

Some corrections to terminology confusion below...


...




file on the host in case of file backed v-nvdimms. This is addressed by
virtio-pmem in case of x86_64 by making explicit flushes translating to
fsync at qemu.


Note that virtio-pmem was a proposal for a specific optimization of
allowing guests to share page cache. The virtio-pmem approach is not
to be confused with actual persistent memory.



.



A new device property sync-dax is added to the nvdimm device. When the
sync-dax is 'writeback'(default for PPC), device property
"hcall-flush-required" is set, and the guest makes hcall H_SCM_FLUSH
requesting for an explicit flush.


I'm not sure "sync-dax" is a suitable name for the property of the
guest persistent memory. There is no requirement that the
memory-backend file for a guest be a dax-capable file. It's also
implementation specific what hypercall needs to be invoked for a given
occurrence of "sync-dax". What does that map to on non-PPC platforms
for example? It seems to me that an "nvdimm" device presents the
synchronous usage model and a whole other device type implements an
async-hypercall setup that the guest happens to service with its
nvdimm stack, but it's not an "nvdimm" anymore at that point.



What is attempted here is to use the same guest driver papr_scm.ko 
support the usecase of sharing page cache from the host instead of 
depending on a new guest driver virtio-pmem. This also try to correctly 
indicate to the guest that an usage like


-object memory-backend-file,id=memnvdimm1,mem-path=file_name
-device nvdimm,memdev=memnvdimm1

correctly indicate to the guest that we are indeed sharing page cache 
and not really emulating a persistent memory.


W.r.t non ppc platforms, it was discussed earlier and one of the 
suggestion there was to mark that as "unsafe".


Any suggestion for an alternate property name than "sync-dax"?


sync-dax is "unsafe" on all other platforms(x86, ARM) and old pseries machines
prior to 5.2 on PPC. sync-dax="writeback" on ARM and x86_64 is prevented
now as the flush semantics are unimplemented.


"sync-dax" has no meaning on its own, I think this needs an explicit
mechanism to convey both the "not-sync" property *and* the callback
method, it shouldn't be inferred by arch type.


Won't a non-sync property imply that guest needs to do a callback to 
ensure persistence? Hence they are related?



-aneesh

Re: [PATCH v4 0/3] nvdimm: Enable sync-dax property for nvdimm

[Aneesh Kumar K.V]

On 4/29/21 9:25 PM, Stefan Hajnoczi wrote:

On Wed, Apr 28, 2021 at 11:48:21PM -0400, Shivaprasad G Bhat wrote:

The nvdimm devices are expected to ensure write persistence during power
failure kind of scenarios.

The libpmem has architecture specific instructions like dcbf on POWER
to flush the cache data to backend nvdimm device during normal writes
followed by explicit flushes if the backend devices are not synchronous
DAX capable.

Qemu - virtual nvdimm devices are memory mapped. The dcbf in the guest
and the subsequent flush doesn't traslate to actual flush to the backend
file on the host in case of file backed v-nvdimms. This is addressed by
virtio-pmem in case of x86_64 by making explicit flushes translating to
fsync at qemu.

On SPAPR, the issue is addressed by adding a new hcall to
request for an explicit flush from the guest ndctl driver when the backend
nvdimm cannot ensure write persistence with dcbf alone. So, the approach
here is to convey when the hcall flush is required in a device tree
property. The guest makes the hcall when the property is found, instead
of relying on dcbf.


Sorry, I'm not very familiar with SPAPR. Why add a hypercall when the
virtio-nvdimm device already exists?



On virtualized ppc64 platforms, guests use papr_scm.ko kernel drive for 
persistent memory support. This was done such that we can use one kernel 
driver to support persistent memory with multiple hypervisors. To avoid 
supporting multiple drivers in the guest, -device nvdimm Qemu 
command-line results in Qemu using PAPR SCM backend. What this patch 
series does is to make sure we expose the correct synchronous fault 
support, when we back such nvdimm device with a file.


The existing PAPR SCM backend enables persistent memory support with the 
help of multiple hypercall.


#define H_SCM_READ_METADATA 0x3E4
#define H_SCM_WRITE_METADATA0x3E8
#define H_SCM_BIND_MEM  0x3EC
#define H_SCM_UNBIND_MEM0x3F0
#define H_SCM_UNBIND_ALL0x3FC

Most of them are already implemented in Qemu. This patch series 
implements H_SCM_FLUSH hypercall.





-aneesh

Re: [PATCH v3 2/3] spapr: nvdimm: Implement H_SCM_FLUSH hcall

[Aneesh Kumar K.V]

On 3/24/21 8:37 AM, David Gibson wrote:

On Tue, Mar 23, 2021 at 09:47:38AM -0400, Shivaprasad G Bhat wrote:

The patch adds support for the SCM flush hcall for the nvdimm devices.
To be available for exploitation by guest through the next patch.

The hcall expects the semantics such that the flush to return
with H_BUSY when the operation is expected to take longer time along
with a continue_token. The hcall to be called again providing the
continue_token to get the status. So, all fresh requsts are put into
a 'pending' list and flush worker is submitted to the thread pool.
The thread pool completion callbacks move the requests to 'completed'
list, which are cleaned up after reporting to guest in subsequent
hcalls to get the status.

The semantics makes it necessary to preserve the continue_tokens
and their return status even across migrations. So, the pre_save
handler for the device waits for the flush worker to complete and
collects all the hcall states from 'completed' list. The necessary
nvdimm flush specific vmstate structures are added to the spapr
machine vmstate.

Signed-off-by: Shivaprasad G Bhat 


An overal question: surely the same issue must arise on x86 with
file-backed NVDIMMs.  How do they handle this case?


On x86 we have different ways nvdimm can be discovered. ACPI NFIT, e820 
map and virtio_pmem. Among these virio_pmem always operated with 
synchronous dax disabled and both ACPI and e820 doesn't have the ability 
to differentiate support for synchronous dax.


With that I would expect users to use virtio_pmem when using using file 
backed NVDIMMS


-aneesh

Re: [PATCH v3 3/3] spapr: nvdimm: Enable sync-dax device property for nvdimm

[Aneesh Kumar K.V]

On 3/24/21 8:39 AM, David Gibson wrote:

On Tue, Mar 23, 2021 at 09:47:55AM -0400, Shivaprasad G Bhat wrote:

The patch adds the 'sync-dax' property to the nvdimm device.

When the sync-dax is 'off', the device tree property
"hcall-flush-required" is added to the nvdimm node which makes the
guest to issue H_SCM_FLUSH hcalls to request for flushes explicitly.
This would be the default behaviour without sync-dax property set
for the nvdimm device.

The sync-dax="on" would mean the guest need not make flush requests
to the qemu. On previous machine versions the sync-dax is set to be
"on" by default using the hw_compat magic.

Signed-off-by: Shivaprasad G Bhat 
---
  hw/core/machine.c   |1 +
  hw/mem/nvdimm.c |1 +
  hw/ppc/spapr_nvdimm.c   |   17 +
  include/hw/mem/nvdimm.h |   10 ++
  include/hw/ppc/spapr.h  |1 +
  5 files changed, 30 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 257a664ea2..f843643574 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -41,6 +41,7 @@ GlobalProperty hw_compat_5_2[] = {
  { "PIIX4_PM", "smm-compat", "on"},
  { "virtio-blk-device", "report-discard-granularity", "off" },
  { "virtio-net-pci", "vectors", "3"},
+{ "nvdimm", "sync-dax", "on" },
  };
  const size_t hw_compat_5_2_len = G_N_ELEMENTS(hw_compat_5_2);
  
diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c

index 7397b67156..8f0e29b191 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -229,6 +229,7 @@ static void nvdimm_write_label_data(NVDIMMDevice *nvdimm, 
const void *buf,
  
  static Property nvdimm_properties[] = {

  DEFINE_PROP_BOOL(NVDIMM_UNARMED_PROP, NVDIMMDevice, unarmed, false),
+DEFINE_PROP_BOOL(NVDIMM_SYNC_DAX_PROP, NVDIMMDevice, sync_dax, false),


I'm a bit uncomfortable adding this base NVDIMM property without at
least some logic about how it's handled on non-PAPR platforms.


yes these should be specific to PAPR. These are there to handle 
migration. with older guest. We can use the backing file to determine 
synchronous dax support. if it is a file backed nvdimm on a fsdax mount 
point, we can do synchronous dax. If it is one on a non dax file system 
synchronous dax can be disabled.





  DEFINE_PROP_END_OF_LIST(),
  };
  
diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c

index 883317c1ed..dd1c90251b 100644
--- a/hw/ppc/spapr_nvdimm.c
+++ b/hw/ppc/spapr_nvdimm.c
@@ -125,6 +125,9 @@ static int spapr_dt_nvdimm(SpaprMachineState *spapr, void 
*fdt,
  uint64_t lsize = nvdimm->label_size;
  uint64_t size = object_property_get_int(OBJECT(nvdimm), PC_DIMM_SIZE_PROP,
  NULL);
+bool sync_dax = object_property_get_bool(OBJECT(nvdimm),
+ NVDIMM_SYNC_DAX_PROP,
+ _abort);
  
  drc = spapr_drc_by_id(TYPE_SPAPR_DRC_PMEM, slot);

  g_assert(drc);
@@ -159,6 +162,11 @@ static int spapr_dt_nvdimm(SpaprMachineState *spapr, void 
*fdt,
   "operating-system")));
  _FDT(fdt_setprop(fdt, child_offset, "ibm,cache-flush-required", NULL, 0));
  
+if (!sync_dax) {

+_FDT(fdt_setprop(fdt, child_offset, "ibm,hcall-flush-required",
+ NULL, 0));
+}
+
  return child_offset;
  }
  
@@ -567,10 +575,12 @@ static target_ulong h_scm_flush(PowerPCCPU *cpu, SpaprMachineState *spapr,

target_ulong opcode, target_ulong *args)
  {
  int ret;
+bool sync_dax;
  uint32_t drc_index = args[0];
  uint64_t continue_token = args[1];
  SpaprDrc *drc = spapr_drc_by_index(drc_index);
  PCDIMMDevice *dimm;
+NVDIMMDevice *nvdimm;
  HostMemoryBackend *backend = NULL;
  SpaprNVDIMMDeviceFlushState *state;
  ThreadPool *pool = aio_get_thread_pool(qemu_get_aio_context());
@@ -580,6 +590,13 @@ static target_ulong h_scm_flush(PowerPCCPU *cpu, 
SpaprMachineState *spapr,
  return H_PARAMETER;
  }
  
+nvdimm = NVDIMM(drc->dev);

+sync_dax = object_property_get_bool(OBJECT(nvdimm), NVDIMM_SYNC_DAX_PROP,
+_abort);
+if (sync_dax) {
+return H_UNSUPPORTED;


Do you want to return UNSUPPORTED here, or just H_SUCCESS, since the
flush should be a no-op in this case.



The reason to handle this as error is to indicate the OS that it is 
using a wrong mechanism to flush.





+}
+
  if (continue_token != 0) {
  ret = spapr_nvdimm_get_flush_status(continue_token);
  if (H_IS_LONG_BUSY(ret)) {
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index bcf62f825c..f82979cf2f 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -51,6 +51,7 @@ OBJECT_DECLARE_TYPE(NVDIMMDevice, NVDIMMClass, NVDIMM)
  #define NVDIMM_LABEL_SIZE_PROP "label-size"
  #define NVDIMM_UUID_PROP   "uuid"
  #define NVDIMM_UNARMED_PROP"unarmed"
+#define 

Re: [Qemu-devel] [PATCH] 9pfs: deprecate handle backend

[Aneesh Kumar K.V]

Greg Kurz <gr...@kaod.org> writes:

> This backend raise some concerns:
>
> - doesn't support symlinks
> - fails +100 tests in the PJD POSIX file system test suite [1]
> - requires the QEMU process to run with the CAP_DAC_READ_SEARCH
>   capability, which isn't recommended for security reasons
>
> For all these reasons, the handle backend is now deprecated.
>
> [1] https://www.tuxera.com/community/posix-test-suite/
>

Reviewed-by: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>

> Signed-off-by: Greg Kurz <gr...@kaod.org>
> ---
>
> Aneesh,
>
> Even if I see the benefit of using file handles in a userspace file
> server, the handle backend still has flaws that make it hardly usable
> IMHO. Also I haven't received anything about it in years. All users
> and contributors seem to stick to the local backend.
>
> My guess is that nobody uses the handle backend, and unless I'm missing
> something, it wouldn't hurt to drop it. My motivation is to reduce the
> number of lines that I don't really have time/motivation to maintain,
> and that could be subject to a CVE in the future.
>
> Any thoughts ?
> ---
>  hw/9pfs/9p-handle.c |2 ++
>  qemu-doc.texi   |8 
>  2 files changed, 10 insertions(+)
>
> diff --git a/hw/9pfs/9p-handle.c b/hw/9pfs/9p-handle.c
> index 9875f1894cc5..1291a2db6782 100644
> --- a/hw/9pfs/9p-handle.c
> +++ b/hw/9pfs/9p-handle.c
> @@ -657,6 +657,8 @@ static int handle_parse_opts(QemuOpts *opts, struct 
> FsDriverEntry *fse)
>  const char *sec_model = qemu_opt_get(opts, "security_model");
>  const char *path = qemu_opt_get(opts, "path");
>
> +warn_report("handle backend is deprecated");
> +
>  if (sec_model) {
>  error_report("Invalid argument security_model specified with handle 
> fsdriver");
>  return -1;
> diff --git a/qemu-doc.texi b/qemu-doc.texi
> index f7317dfc66cd..bf44e2752cb2 100644
> --- a/qemu-doc.texi
> +++ b/qemu-doc.texi
> @@ -2509,6 +2509,14 @@ default channel subsystem image for guests that do not 
> support multiple
>  channel subsystems, all devices can be put into the default channel
>  subsystem image.
>
> +@subsection -fsdev handle (since 2.12.0)
> +
> +The ``handle'' fsdev backend does not support symlinks and causes the 9p
> +filesystem in the guest to fail a fair amount of tests from the PJD POSIX
> +filesystem test suite. Also it requires the CAP_DAC_READ_SEARCH capability,
> +which is not the recommended way to run QEMU. This backend should not be
> +used and it will be removed with no replacement.
> +
>  @section qemu-img command line arguments
>
>  @subsection convert -s (since 2.0.0)

Re: [Qemu-devel] [PATCH] MAINTAINERS: Drop Aneesh as 9pfs maintainer

[Aneesh Kumar K.V]

Greg Kurz <gr...@kaod.org> writes:

> Aneesh has been working on other topics for some time now. Let's reflect
> that in the MAINTAINERS file, so that people stop Cc'ing him.
>
> Signed-off-by: Greg Kurz <gr...@kaod.org>

Acked-by: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>

> ---
>  MAINTAINERS |2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8859a50c365b..a61f3acb84ba 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1082,13 +1082,11 @@ F: include/hw/virtio/
>  F: tests/virtio-balloon-test.c
>
>  virtio-9p
> -M: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
>  M: Greg Kurz <gr...@kaod.org>
>  S: Supported
>  F: hw/9pfs/
>  F: fsdev/
>  F: tests/virtio-9p-test.c
> -T: git git://github.com/kvaneesh/QEMU.git
>  T: git git://github.com/gkurz/qemu.git 9p-next
>
>  virtio-blk

Re: [Qemu-devel] [PATCH v4 1/3] 9pfs: forbid illegal path names

[Aneesh Kumar K.V]

Eric Blake  writes:

> [ Unknown signature status ]
> On 08/30/2016 12:11 PM, Greg Kurz wrote:
>> Empty path components don't make sense for most commands and may cause
>> undefined behavior, depending on the backend.
>> 
>> Also, the walk request described in the 9P spec [1] clearly shows that
>> the client is supposed to send individual path components: the official
>> linux client never sends portions of path containing the / character for
>> example.
>> 
>> Moreover, the 9P spec [2] also states that a system can decide to restrict
>> the set of supported characters used in path components, with an explicit
>> mention "to remove slashes from name components".
>> 
>> This patch introduces a new name_is_illegal() helper that checks the
>> names sent by the client are not empty and don't contain unwanted chars.
>> Since 9pfs is only supported on linux hosts, only the / character is
>> checked at the moment. When support for other hosts (AKA. win32) is added,
>> other chars may need to be blacklisted as well.
>> 
>> If a client sends an illegal path component, the request will fail and
>> ENOENT is returned to the client.
>> 
>> [1] http://man.cat-v.org/plan_9/5/walk
>> [2] http://man.cat-v.org/plan_9/5/intro
>> 
>> Suggested-by: Peter Maydell 
>> Signed-off-by: Greg Kurz 
>> ---
>> v4: dropped the checking of the symbolic link target name: because a target
>> can be a full path and thus contain '/' and linux already complains if
>> it is an empty string. When the symlink gets dereferenced, slashes are
>> interpreted as the usual path component separator.
>
> Can a symlink to "/foo" be used to escape the root (by being absolute
> instead of relative)?  However, if the answer to that question requires
> more code, I'm fine with it being a separate patch.  So for this email,

We resolve "/foo" on the client side. So this is ok.

-aneesh

Re: [Qemu-devel] [PATCH] 9pfs: add check for relative path

[Aneesh Kumar K.V]

P J P  writes:

> From: Prasad J Pandit 
>
> At various places in 9pfs back-end, it creates full path by
> concatenating two path strings. It could lead to a path
> traversal issue if one of the parameter was a relative path.
> Add check to avoid it.
>
> Reported-by: Felix Wilhelm 
> Signed-off-by: Prasad J Pandit 

I am not sure relative path names need to be completely disallowed. What
we need is to disallow the access outside export path. virtfs-proxy was
done primarily to handle such things. It does a chroot to the export
path. 


> ---
>  hw/9pfs/9p-local.c | 31 +++
>  1 file changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index 3f271fc..c20331a 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -493,6 +493,9 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath 
> *dir_path,
>  char *buffer = NULL;
>  
>  v9fs_string_init();
> +if (strstr(name, "../")) {
> +return err;
> +}
>  v9fs_string_sprintf(, "%s/%s", dir_path->data, name);
>  path = fullname.data;
>  
> @@ -554,6 +557,9 @@ static int local_mkdir(FsContext *fs_ctx, V9fsPath 
> *dir_path,
>  char *buffer = NULL;
>  
>  v9fs_string_init();
> +if (strstr(name, "../")) {
> +return err;
> +}
>  v9fs_string_sprintf(, "%s/%s", dir_path->data, name);
>  path = fullname.data;
>  
> @@ -663,6 +669,9 @@ static int local_open2(FsContext *fs_ctx, V9fsPath 
> *dir_path, const char *name,
>  flags |= O_NOFOLLOW;
>  
>  v9fs_string_init();
> +if (strstr(name, "../")) {
> +return err;
> +}
>  v9fs_string_sprintf(, "%s/%s", dir_path->data, name);
>  path = fullname.data;
>  
> @@ -734,6 +743,9 @@ static int local_symlink(FsContext *fs_ctx, const char 
> *oldpath,
>  char *buffer = NULL;
>  
>  v9fs_string_init();
> +if (strstr(name, "../")) {
> +return err;
> +}
>  v9fs_string_sprintf(, "%s/%s", dir_path->data, name);
>  newpath = fullname.data;
>  
> @@ -830,11 +842,14 @@ out:
>  static int local_link(FsContext *ctx, V9fsPath *oldpath,
>V9fsPath *dirpath, const char *name)
>  {
> -int ret;
> +int ret = -1;
>  V9fsString newpath;
>  char *buffer, *buffer1;
>  
>  v9fs_string_init();
> +if (strstr(name, "../")) {
> +return ret;
> +}
>  v9fs_string_sprintf(, "%s/%s", dirpath->data, name);
>  
>  buffer = rpath(ctx, oldpath->data);
> @@ -1059,6 +1074,9 @@ static int local_lremovexattr(FsContext *ctx, V9fsPath 
> *fs_path,
>  static int local_name_to_path(FsContext *ctx, V9fsPath *dir_path,
>const char *name, V9fsPath *target)
>  {
> +if (strstr(name, "../")) {
> +return -1;
> +}
>  if (dir_path) {
>  v9fs_string_sprintf((V9fsString *)target, "%s/%s",
>  dir_path->data, name);
> @@ -1074,12 +1092,15 @@ static int local_renameat(FsContext *ctx, V9fsPath 
> *olddir,
>const char *old_name, V9fsPath *newdir,
>const char *new_name)
>  {
> -int ret;
> +int ret = -1;
>  V9fsString old_full_name, new_full_name;
>  
>  v9fs_string_init(_full_name);
>  v9fs_string_init(_full_name);
>  
> +if (strstr(old_name, "../") || strstr(new_name, "../")) {
> +return ret;
> +}
>  v9fs_string_sprintf(_full_name, "%s/%s", olddir->data, old_name);
>  v9fs_string_sprintf(_full_name, "%s/%s", newdir->data, new_name);
>  
> @@ -1092,12 +1113,14 @@ static int local_renameat(FsContext *ctx, V9fsPath 
> *olddir,
>  static int local_unlinkat(FsContext *ctx, V9fsPath *dir,
>const char *name, int flags)
>  {
> -int ret;
> +int ret = -1;
>  V9fsString fullname;
>  char *buffer;
>  
>  v9fs_string_init();
> -
> +if (strstr(name, "../")) {
> +return ret;
> +}
>  v9fs_string_sprintf(, "%s/%s", dir->data, name);
>  if (ctx->export_flags & V9FS_SM_MAPPED_FILE) {
>  if (flags == AT_REMOVEDIR) {
> -- 
> 2.5.5

[Qemu-devel] [PATCH] powerpc/mm: Update the WIMG check during H_ENTER

[Aneesh Kumar K.V]

Support for 0 value for memeory coherence is optional and with ppc64
we can always enable memory coherence. Linux kernel did that during
the development of 4.7 kernel. But that resulted in failure in Qemu
in H_ENTER hcall due to below check. The mentioned change was reverted
in the kernel and kernel right now enable memory coherence only if
cache inhibited is not set. Nevertheless update qemu WIMG flag check
to cover the case where we enable memory coherence along with cache
inhibited flag.

In order to handle older and newer kernel version consider both Cache
inhibitted and (cache inhibitted | memory conference) as valid values
for wimg flags.

Signed-off-by: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
---
 hw/ppc/spapr_hcall.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 2ba5cbdb194a..e011ed4b664b 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -102,11 +102,15 @@ static target_ulong h_enter(PowerPCCPU *cpu, 
sPAPRMachineState *spapr,
 return H_PARAMETER;
 }
 } else {
+target_ulong wimg_flags;
 /* Looks like an IO address */
 /* FIXME: What WIMG combinations could be sensible for IO?
  * For now we allow WIMG=010x, but are there others? */
 /* FIXME: Should we check against registered IO addresses? */
-if ((ptel & (HPTE64_R_W | HPTE64_R_I | HPTE64_R_M)) != HPTE64_R_I) {
+wimg_flags = (ptel & (HPTE64_R_W | HPTE64_R_I | HPTE64_R_M));
+
+if (wimg_flags != HPTE64_R_I &&
+wimg_flags != (HPTE64_R_I | HPTE64_R_M)) {
 return H_PARAMETER;
 }
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH] hw/9pfs: Add CephFS support in VirtFS

[Aneesh Kumar K.V]

Greg Kurz  writes:

> On Tue, 15 Mar 2016 00:02:48 +0800
> Jevon Qiao  wrote:
>
>> Ceph as a promising unified distributed storage system is widely used in the
>> world of OpenStack. OpenStack users deploying Ceph for block (Cinder) and
>> object (S3/Swift) are unsurprisingly looking at Manila and CephFS to round 
>> out
>> a unified storage solution. Since the typical hypervisor people are using is
>> Qemu/KVM, it is necessary to provide a high performance, easy to use, file
>> system service in it. VirtFS aims to offers paravirtualized system services 
>> and
>> simple passthrough for directories from host to guest, which currently only
>> support local file system, this patch wants to add CephFS support in VirtFS.
>> 
>> Signed-off-by: Jevon Qiao 
>> ---
>
> Jevon,
>
> There's still work to be done on this patch.
>
> One general remark is that there are far too many traces: it obfuscates the 
> code
> and does not bring much benefit in my opinion. If you look at the other fsdev
> drivers, you see they don't do traces at all !
>
> Also, I've found several errors where the code simply cannot work... please 
> run
> a file/io oriented testsuite in the guest to check all the fsdev operations 
> are
> working as expected... maybe some tests from LTP ?

Also use this http://tuxera.com/sw/qa/pjd-fstest-20090130-RC.tgz 

-aneesh

Re: [Qemu-devel] [PATCH 2/2] hw/9pfs: fix alignment issue when host filesystem block size is larger than client msize

[Aneesh Kumar K.V]

Jevon Qiao <scaleq...@gmail.com> writes:

> Hi Aneesh,
>
> Thank you for reviewing my code, please see my reply in-line.
> On 14/2/16 21:38, Aneesh Kumar K.V wrote:
>> Jevon Qiao <scaleq...@gmail.com> writes:
>>
>>> The following patch is to fix alignment issue when host filesystem block
>>> size
>>> is larger than client msize.
>>>
>>> Thanks,
>>> Jevon
>> That is not the right format to send patch. You can send them as a
>> series using git-send-email.
> Yes, you're correct. I will send the patches later after I address all 
> the technical comments.
>>> From: Jevon Qiao <scaleq...@gmail.com>
>>> Date: Sun, 14 Feb 2016 15:11:08 +0800
>>> Subject: [PATCH] hw/9pfs: fix alignment issue when host filesystem block
>>> size
>>>is larger than client msize.
>>>
>>> Per the previous implementation, iounit will be assigned to be 0 after the
>>> first if statement as (s->msize - P9_IOHDRSZ)/stbuf.f_bsize will be zero
>>> when
>>> host filesystem block size is larger than msize. Finally, iounit will be
>>> equal
>>> to s->msize - P9_IOHDRSZ, which is usually not aligned.
>>>
>>> Signed-off-by: Jevon Qiao <scaleq...@gmail.com>
>>> ---
>>>hw/9pfs/virtio-9p.c | 19 ---
>>>1 file changed, 16 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
>>> index f972731..005d3a8 100644
>>> --- a/hw/9pfs/virtio-9p.c
>>> +++ b/hw/9pfs/virtio-9p.c
>>> @@ -1326,7 +1326,7 @@ out_nofid:
>>>static int32_t get_iounit(V9fsPDU *pdu, V9fsPath *path)
>>>{
>>>struct statfs stbuf;
>>> -int32_t iounit = 0;
>>> +int32_t iounit = 0, unit = 0;
>>>V9fsState *s = pdu->s;
>>>
>>>/*
>>> @@ -1334,8 +1334,21 @@ static int32_t get_iounit(V9fsPDU *pdu, V9fsPath
>>> *path)
>>> * and as well as less than (client msize - P9_IOHDRSZ))
>>> */
>>>if (!v9fs_co_statfs(pdu, path, )) {
>>> -iounit = stbuf.f_bsize;
>>> -iounit *= (s->msize - P9_IOHDRSZ)/stbuf.f_bsize;
>>> +/*
>>> + * If host filesystem block size is larger than client msize,
>>> + * we will use PAGESIZE as the unit. The reason why we choose
>>> + * PAGESIZE is because the data will be splitted in terms of
>>> + * PAGESIZE in the virtio layer. In this case, the final
>>> + * iounit is equal to the value of ((msize/unit) - 1) * unit.
>>> + */
>>> +if (stbuf.f_bsize > s->msize) {
>>> +iounit = 4096;
>>> +unit = 4096;
>> What page size it should be guest or host ?. Also why 4096 ?. ppc64 use
>> 64K page size.
> The data to be read or written will be divided into pieces according to the
> size of iounit and msize firstly, and then mapped to pages before being 
> added
> into virtqueue. Since all these operations happen in the guest side, so the
> page size should be guest. Please correct me if I'm wrong.

I am not sure I understand the details correctly. iounit is the size
that we use in client_read to determine the  size in which
we should request I/O from the client. But we still can't do I/O in size
larger than s->msize. If you look at the client side (kernel 9p fs), you
will find

rsize = fid->iounit;
if (!rsize || rsize > clnt->msize-P9_IOHDRSZ)
rsize = clnt->msize - P9_IOHDRSZ;

if your iounit calculation ends up zero, that should be handled
correctly by

if (!iounit) {
iounit = s->msize - P9_IOHDRSZ;
}
return iounit;


So what is the issue here. ?

-aneesh

Re: [Qemu-devel] [PATCH 2/2] hw/9pfs: fix alignment issue when host filesystem block size is larger than client msize

[Aneesh Kumar K.V]

Jevon Qiao  writes:

> The following patch is to fix alignment issue when host filesystem block 
> size
> is larger than client msize.
>
> Thanks,
> Jevon

That is not the right format to send patch. You can send them as a
series using git-send-email.

>
> From: Jevon Qiao 
> Date: Sun, 14 Feb 2016 15:11:08 +0800
> Subject: [PATCH] hw/9pfs: fix alignment issue when host filesystem block 
> size
>   is larger than client msize.
>
> Per the previous implementation, iounit will be assigned to be 0 after the
> first if statement as (s->msize - P9_IOHDRSZ)/stbuf.f_bsize will be zero 
> when
> host filesystem block size is larger than msize. Finally, iounit will be 
> equal
> to s->msize - P9_IOHDRSZ, which is usually not aligned.
>
> Signed-off-by: Jevon Qiao 
> ---
>   hw/9pfs/virtio-9p.c | 19 ---
>   1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> index f972731..005d3a8 100644
> --- a/hw/9pfs/virtio-9p.c
> +++ b/hw/9pfs/virtio-9p.c
> @@ -1326,7 +1326,7 @@ out_nofid:
>   static int32_t get_iounit(V9fsPDU *pdu, V9fsPath *path)
>   {
>   struct statfs stbuf;
> -int32_t iounit = 0;
> +int32_t iounit = 0, unit = 0;
>   V9fsState *s = pdu->s;
>
>   /*
> @@ -1334,8 +1334,21 @@ static int32_t get_iounit(V9fsPDU *pdu, V9fsPath 
> *path)
>* and as well as less than (client msize - P9_IOHDRSZ))
>*/
>   if (!v9fs_co_statfs(pdu, path, )) {
> -iounit = stbuf.f_bsize;
> -iounit *= (s->msize - P9_IOHDRSZ)/stbuf.f_bsize;
> +/*
> + * If host filesystem block size is larger than client msize,
> + * we will use PAGESIZE as the unit. The reason why we choose
> + * PAGESIZE is because the data will be splitted in terms of
> + * PAGESIZE in the virtio layer. In this case, the final
> + * iounit is equal to the value of ((msize/unit) - 1) * unit.
> + */
> +if (stbuf.f_bsize > s->msize) {
> +iounit = 4096;
> +unit = 4096;

What page size it should be guest or host ?. Also why 4096 ?. ppc64 use
64K page size.


> +} else {
> +iounit = stbuf.f_bsize;
> +unit = stbuf.f_bsize;
> +}
> +iounit *= (s->msize - P9_IOHDRSZ)/unit;
>   }
>   if (!iounit) {
>   iounit = s->msize - P9_IOHDRSZ;
> -- 

-aneesh

Re: [Qemu-devel] [PATCH] add CephFS support in VirtFS

[Aneesh Kumar K.V]

> diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> index f972731..385c01d 100644
> --- a/hw/9pfs/virtio-9p.c
> +++ b/hw/9pfs/virtio-9p.c
> @@ -1326,7 +1326,7 @@ out_nofid:
>   static int32_t get_iounit(V9fsPDU *pdu, V9fsPath *path)
>   {
>   struct statfs stbuf;
> -int32_t iounit = 0;
> +int32_t iounit = 0, unit = 0;
>   V9fsState *s = pdu->s;
>
>   /*
> @@ -1334,8 +1334,21 @@ static int32_t get_iounit(V9fsPDU *pdu, V9fsPath 
> *path)
>* and as well as less than (client msize - P9_IOHDRSZ))
>*/
>   if (!v9fs_co_statfs(pdu, path, )) {
> -iounit = stbuf.f_bsize;
> -iounit *= (s->msize - P9_IOHDRSZ)/stbuf.f_bsize;
> +/*
> + * If host filesystem block size is larger than client msize,
> + * we will use AGESIZE as the unit. The reason why we choose
> + * AGESIZE is because the data will be splitted in terms of
> + * AGESIZE in the virtio layer. In this case, the final
> + * iounit is equal to the value of ((msize/unit) - 1) * unit.
> + */
> +if (stbuf.f_bsize > s->msize) {
> +iounit = 4096;
> +unit = 4096;
> +} else {
> +iounit = stbuf.f_bsize;
> +unit = stbuf.f_bsize;
> +}
> +iounit *= (s->msize - P9_IOHDRSZ)/unit;
>   }
>   if (!iounit) {
>   iounit = s->msize - P9_IOHDRSZ;
> --


Can you split this out as a separate patch ?. Also explain what is
AGESIZE.

-aneesh

[Qemu-devel] [PATCH 15/25] 9pfs: factor out virtio_pdu_{, un}marshal

[Aneesh Kumar K.V]

From: Wei Liu <wei.l...@citrix.com>

Signed-off-by: Wei Liu <wei.l...@citrix.com>
Signed-off-by: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
---
 hw/9pfs/virtio-9p-device.c | 14 ++
 hw/9pfs/virtio-9p.c|  6 ++
 hw/9pfs/virtio-9p.h|  5 +
 3 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index f3091cc813e7..d77247f3cdad 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -156,6 +156,20 @@ static void virtio_9p_device_unrealize(DeviceState *dev, 
Error **errp)
 g_free(s->tag);
 }
 
+ssize_t virtio_pdu_vmarshal(V9fsPDU *pdu, size_t offset,
+const char *fmt, va_list ap)
+{
+return v9fs_iov_vmarshal(pdu->elem.in_sg, pdu->elem.in_num,
+ offset, 1, fmt, ap);
+}
+
+ssize_t virtio_pdu_vunmarshal(V9fsPDU *pdu, size_t offset,
+  const char *fmt, va_list ap)
+{
+return v9fs_iov_vunmarshal(pdu->elem.out_sg, pdu->elem.out_num,
+   offset, 1, fmt, ap);
+}
+
 /* virtio-9p device */
 
 static Property virtio_9p_properties[] = {
diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
index a740f85625a3..6d32b81faa25 100644
--- a/hw/9pfs/virtio-9p.c
+++ b/hw/9pfs/virtio-9p.c
@@ -45,8 +45,7 @@ ssize_t pdu_marshal(V9fsPDU *pdu, size_t offset, const char 
*fmt, ...)
 va_list ap;
 
 va_start(ap, fmt);
-ret = v9fs_iov_vmarshal(pdu->elem.in_sg, pdu->elem.in_num,
-offset, 1, fmt, ap);
+ret = virtio_pdu_vmarshal(pdu, offset, fmt, ap);
 va_end(ap);
 
 return ret;
@@ -58,8 +57,7 @@ ssize_t pdu_unmarshal(V9fsPDU *pdu, size_t offset, const char 
*fmt, ...)
 va_list ap;
 
 va_start(ap, fmt);
-ret = v9fs_iov_vunmarshal(pdu->elem.out_sg, pdu->elem.out_num,
-  offset, 1, fmt, ap);
+ret = virtio_pdu_vunmarshal(pdu, offset, fmt, ap);
 va_end(ap);
 
 return ret;
diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
index d6f3ac08a76a..e298949fde40 100644
--- a/hw/9pfs/virtio-9p.h
+++ b/hw/9pfs/virtio-9p.h
@@ -323,6 +323,11 @@ extern int v9fs_name_to_path(V9fsState *s, V9fsPath 
*dirpath,
 ssize_t pdu_marshal(V9fsPDU *pdu, size_t offset, const char *fmt, ...);
 ssize_t pdu_unmarshal(V9fsPDU *pdu, size_t offset, const char *fmt, ...);
 
+ssize_t virtio_pdu_vmarshal(V9fsPDU *pdu, size_t offset,
+const char *fmt, va_list ap);
+ssize_t virtio_pdu_vunmarshal(V9fsPDU *pdu, size_t offset,
+  const char *fmt, va_list ap);
+
 #define TYPE_VIRTIO_9P "virtio-9p-device"
 #define VIRTIO_9P(obj) \
 OBJECT_CHECK(V9fsState, (obj), TYPE_VIRTIO_9P)
-- 
2.5.0

[Qemu-devel] [PATCH 01/25] 9pfs: rename virtio-9p-coth.{c, h} to coth.{c, h}

[Aneesh Kumar K.V]

From: Wei Liu <wei.l...@citrix.com>

Those two files are not virtio specific. Rename them to use generic
names.

Fix includes in various C files. Change define guards and comments in
header files.

Signed-off-by: Wei Liu <wei.l...@citrix.com>
Signed-off-by: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
---
 hw/9pfs/Makefile.objs| 2 +-
 hw/9pfs/codir.c  | 2 +-
 hw/9pfs/cofile.c | 2 +-
 hw/9pfs/cofs.c   | 2 +-
 hw/9pfs/{virtio-9p-coth.c => coth.c} | 4 ++--
 hw/9pfs/{virtio-9p-coth.h => coth.h} | 6 +++---
 hw/9pfs/coxattr.c| 2 +-
 hw/9pfs/virtio-9p-device.c   | 2 +-
 hw/9pfs/virtio-9p.c  | 2 +-
 9 files changed, 12 insertions(+), 12 deletions(-)
 rename hw/9pfs/{virtio-9p-coth.c => coth.c} (95%)
 rename hw/9pfs/{virtio-9p-coth.h => coth.h} (98%)

diff --git a/hw/9pfs/Makefile.objs b/hw/9pfs/Makefile.objs
index 1e9b595cb4df..76dadbe1f2c7 100644
--- a/hw/9pfs/Makefile.objs
+++ b/hw/9pfs/Makefile.objs
@@ -1,7 +1,7 @@
 common-obj-y  = virtio-9p.o
 common-obj-y += virtio-9p-local.o virtio-9p-xattr.o
 common-obj-y += virtio-9p-xattr-user.o virtio-9p-posix-acl.o
-common-obj-y += virtio-9p-coth.o cofs.o codir.o cofile.o
+common-obj-y += coth.o cofs.o codir.o cofile.o
 common-obj-y += coxattr.o virtio-9p-synth.o
 common-obj-$(CONFIG_OPEN_BY_HANDLE) +=  virtio-9p-handle.o
 common-obj-y += virtio-9p-proxy.o
diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c
index ec9cc7fb274a..5a4f74d3e069 100644
--- a/hw/9pfs/codir.c
+++ b/hw/9pfs/codir.c
@@ -15,7 +15,7 @@
 #include "fsdev/qemu-fsdev.h"
 #include "qemu/thread.h"
 #include "qemu/coroutine.h"
-#include "virtio-9p-coth.h"
+#include "coth.h"
 
 int v9fs_co_readdir_r(V9fsPDU *pdu, V9fsFidState *fidp, struct dirent *dent,
   struct dirent **result)
diff --git a/hw/9pfs/cofile.c b/hw/9pfs/cofile.c
index 7cb55ee93a4f..893df2c42247 100644
--- a/hw/9pfs/cofile.c
+++ b/hw/9pfs/cofile.c
@@ -15,7 +15,7 @@
 #include "fsdev/qemu-fsdev.h"
 #include "qemu/thread.h"
 #include "qemu/coroutine.h"
-#include "virtio-9p-coth.h"
+#include "coth.h"
 
 int v9fs_co_st_gen(V9fsPDU *pdu, V9fsPath *path, mode_t st_mode,
V9fsStatDotl *v9stat)
diff --git a/hw/9pfs/cofs.c b/hw/9pfs/cofs.c
index e1953a9aa180..7b4202bd7728 100644
--- a/hw/9pfs/cofs.c
+++ b/hw/9pfs/cofs.c
@@ -15,7 +15,7 @@
 #include "fsdev/qemu-fsdev.h"
 #include "qemu/thread.h"
 #include "qemu/coroutine.h"
-#include "virtio-9p-coth.h"
+#include "coth.h"
 
 static ssize_t __readlink(V9fsState *s, V9fsPath *path, V9fsString *buf)
 {
diff --git a/hw/9pfs/virtio-9p-coth.c b/hw/9pfs/coth.c
similarity index 95%
rename from hw/9pfs/virtio-9p-coth.c
rename to hw/9pfs/coth.c
index ab9425c60fd2..56772d66be89 100644
--- a/hw/9pfs/virtio-9p-coth.c
+++ b/hw/9pfs/coth.c
@@ -1,5 +1,5 @@
 /*
- * Virtio 9p backend
+ * 9p backend
  *
  * Copyright IBM, Corp. 2010
  *
@@ -16,7 +16,7 @@
 #include "block/thread-pool.h"
 #include "qemu/coroutine.h"
 #include "qemu/main-loop.h"
-#include "virtio-9p-coth.h"
+#include "coth.h"
 
 /* Called from QEMU I/O thread.  */
 static void coroutine_enter_cb(void *opaque, int ret)
diff --git a/hw/9pfs/virtio-9p-coth.h b/hw/9pfs/coth.h
similarity index 98%
rename from hw/9pfs/virtio-9p-coth.h
rename to hw/9pfs/coth.h
index 4ac1aaf90292..209fc6a9afbc 100644
--- a/hw/9pfs/virtio-9p-coth.h
+++ b/hw/9pfs/coth.h
@@ -1,5 +1,5 @@
 /*
- * Virtio 9p backend
+ * 9p backend
  *
  * Copyright IBM, Corp. 2010
  *
@@ -12,8 +12,8 @@
  *
  */
 
-#ifndef _QEMU_VIRTIO_9P_COTH_H
-#define _QEMU_VIRTIO_9P_COTH_H
+#ifndef _QEMU_9P_COTH_H
+#define _QEMU_9P_COTH_H
 
 #include "qemu/thread.h"
 #include "qemu/coroutine.h"
diff --git a/hw/9pfs/coxattr.c b/hw/9pfs/coxattr.c
index 55c0d231cb65..0590cbf5c7c8 100644
--- a/hw/9pfs/coxattr.c
+++ b/hw/9pfs/coxattr.c
@@ -15,7 +15,7 @@
 #include "fsdev/qemu-fsdev.h"
 #include "qemu/thread.h"
 #include "qemu/coroutine.h"
-#include "virtio-9p-coth.h"
+#include "coth.h"
 
 int v9fs_co_llistxattr(V9fsPDU *pdu, V9fsPath *path, void *value, size_t size)
 {
diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index b42d3b30a027..667b54aeb829 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -18,7 +18,7 @@
 #include "virtio-9p.h"
 #include "fsdev/qemu-fsdev.h"
 #include "virtio-9p-xattr.h"
-#include "virtio-9p-coth.h"
+#include "coth.h"
 #include "hw/virtio/virtio-access.h"
 
 static uint64_t virtio_9p_get_features(VirtIODevice *vdev, uint64_t features,
diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
index f972731f5a8d..0f178dec32f3 100644
--- a/hw/9pfs/virtio-9p.c
+++ b/hw/9pfs/virtio-9p.c
@@ -19,7 +19,7 @@
 #include "virtio-9p.h"
 #include "fsdev/qemu-fsdev.h"
 #include "virtio-9p-xattr.h"
-#include "virtio-9p-coth.h"
+#include "coth.h"
 #include "trace.h"
 #include "migration/migration.h"
 
-- 
2.5.0

[Qemu-devel] [PATCH 12/25] 9pfs: PDU processing functions don't need to take V9fsState as argument

[Aneesh Kumar K.V]

From: Wei Liu <wei.l...@citrix.com>

V9fsState can be referenced by pdu->s. Initialise that in device
realization function.

Signed-off-by: Wei Liu <wei.l...@citrix.com>
Signed-off-by: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
---
 hw/9pfs/virtio-9p-device.c |  1 +
 hw/9pfs/virtio-9p.c| 98 +-
 2 files changed, 46 insertions(+), 53 deletions(-)

diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index 885b94068355..f3091cc813e7 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -69,6 +69,7 @@ static void virtio_9p_device_realize(DeviceState *dev, Error 
**errp)
 QLIST_INIT(>active_list);
 for (i = 0; i < (MAX_REQ - 1); i++) {
 QLIST_INSERT_HEAD(>free_list, >pdus[i], next);
+s->pdus[i].s = s;
 }
 
 s->vq = virtio_add_queue(vdev, MAX_REQ, handle_9p_output);
diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
index 30ff82865ea4..0a016dc11a7c 100644
--- a/hw/9pfs/virtio-9p.c
+++ b/hw/9pfs/virtio-9p.c
@@ -575,9 +575,10 @@ static V9fsPDU *alloc_pdu(V9fsState *s)
 return pdu;
 }
 
-static void free_pdu(V9fsState *s, V9fsPDU *pdu)
+static void free_pdu(V9fsPDU *pdu)
 {
 if (pdu) {
+V9fsState *s = pdu->s;
 /*
  * Cancelled pdu are added back to the freelist
  * by flush request .
@@ -594,9 +595,10 @@ static void free_pdu(V9fsState *s, V9fsPDU *pdu)
  * because we always expect to have enough space to encode
  * error details
  */
-static void complete_pdu(V9fsState *s, V9fsPDU *pdu, ssize_t len)
+static void complete_pdu(V9fsPDU *pdu, ssize_t len)
 {
 int8_t id = pdu->id + 1; /* Response */
+V9fsState *s = pdu->s;
 
 if (len < 0) {
 int err = -len;
@@ -636,7 +638,7 @@ static void complete_pdu(V9fsState *s, V9fsPDU *pdu, 
ssize_t len)
 /* Now wakeup anybody waiting in flush for this request */
 qemu_co_queue_next(>complete);
 
-free_pdu(s, pdu);
+free_pdu(pdu);
 }
 
 static mode_t v9mode_to_mode(uint32_t mode, V9fsString *extension)
@@ -931,7 +933,7 @@ static void v9fs_version(void *opaque)
 offset += err;
 trace_v9fs_version_return(pdu->tag, pdu->id, s->msize, version.data);
 out:
-complete_pdu(s, pdu, offset);
+complete_pdu(pdu, offset);
 v9fs_string_free();
 }
 
@@ -995,7 +997,7 @@ static void v9fs_attach(void *opaque)
 out:
 put_fid(pdu, fidp);
 out_nofid:
-complete_pdu(s, pdu, err);
+complete_pdu(pdu, err);
 v9fs_string_free();
 v9fs_string_free();
 }
@@ -1009,7 +1011,6 @@ static void v9fs_stat(void *opaque)
 struct stat stbuf;
 V9fsFidState *fidp;
 V9fsPDU *pdu = opaque;
-V9fsState *s = pdu->s;
 
 err = pdu_unmarshal(pdu, offset, "d", );
 if (err < 0) {
@@ -1042,7 +1043,7 @@ static void v9fs_stat(void *opaque)
 out:
 put_fid(pdu, fidp);
 out_nofid:
-complete_pdu(s, pdu, err);
+complete_pdu(pdu, err);
 }
 
 static void v9fs_getattr(void *opaque)
@@ -1105,7 +1106,7 @@ static void v9fs_getattr(void *opaque)
 out:
 put_fid(pdu, fidp);
 out_nofid:
-complete_pdu(s, pdu, retval);
+complete_pdu(pdu, retval);
 }
 
 /* Attribute flags */
@@ -1129,7 +1130,6 @@ static void v9fs_setattr(void *opaque)
 size_t offset = 7;
 V9fsIattr v9iattr;
 V9fsPDU *pdu = opaque;
-V9fsState *s = pdu->s;
 
 err = pdu_unmarshal(pdu, offset, "dI", , );
 if (err < 0) {
@@ -1203,7 +1203,7 @@ static void v9fs_setattr(void *opaque)
 out:
 put_fid(pdu, fidp);
 out_nofid:
-complete_pdu(s, pdu, err);
+complete_pdu(pdu, err);
 }
 
 static int v9fs_walk_marshal(V9fsPDU *pdu, uint16_t nwnames, V9fsQID *qids)
@@ -1245,7 +1245,7 @@ static void v9fs_walk(void *opaque)
 
 err = pdu_unmarshal(pdu, offset, "ddw", , , );
 if (err < 0) {
-complete_pdu(s, pdu, err);
+complete_pdu(pdu, err);
 return ;
 }
 offset += err;
@@ -1313,7 +1313,7 @@ out:
 v9fs_path_free();
 v9fs_path_free();
 out_nofid:
-complete_pdu(s, pdu, err);
+complete_pdu(pdu, err);
 if (nwnames && nwnames <= P9_MAXWELEM) {
 for (name_idx = 0; name_idx < nwnames; name_idx++) {
 v9fs_string_free([name_idx]);
@@ -1430,7 +1430,7 @@ static void v9fs_open(void *opaque)
 out:
 put_fid(pdu, fidp);
 out_nofid:
-complete_pdu(s, pdu, err);
+complete_pdu(pdu, err);
 }
 
 static void v9fs_lcreate(void *opaque)
@@ -1487,7 +1487,7 @@ static void v9fs_lcreate(void *opaque)
 out:
 put_fid(pdu, fidp);
 out_nofid:
-complete_pdu(pdu->s, pdu, err);
+complete_pdu(pdu, err);
 v9fs_string_free();
 }
 
@@ -1499,7 +1499,6 @@ static void v9fs_fsync(void *opaque)
 size_t offset = 7;
 V9fsFidState *fidp;
 V9fsPDU *pdu = opaque;
-V9fsState *s = pdu->s;
 
 err = pdu_unmarshal(pdu, offset, "dd", , );
 if (err < 0) {
@@ -1518,7 

[Qemu-devel] [PATCH 08/25] 9pfs: merge hw/virtio/virtio-9p.h into hw/9pfs/virtio-9p.h

[Aneesh Kumar K.V]

From: Wei Liu <wei.l...@citrix.com>

The deleted file only contained V9fsConf which wasn't virtio specific.
Merge that to the general header of 9pfs.

Fixed header inclusions as I went along.

Signed-off-by: Wei Liu <wei.l...@citrix.com>
Signed-off-by: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
---
 hw/9pfs/virtio-9p-device.c|  1 -
 hw/9pfs/virtio-9p.h   |  8 +++-
 hw/virtio/virtio-pci.h|  1 -
 include/hw/virtio/virtio-9p.h | 24 
 4 files changed, 7 insertions(+), 27 deletions(-)
 delete mode 100644 include/hw/virtio/virtio-9p.h

diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index 92ac19b24b83..885b94068355 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -12,7 +12,6 @@
  */
 
 #include "hw/virtio/virtio.h"
-#include "hw/virtio/virtio-9p.h"
 #include "hw/i386/pc.h"
 #include "qemu/sockets.h"
 #include "virtio-9p.h"
diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
index d7a4dc1e9ad7..ac4cb006b30e 100644
--- a/hw/9pfs/virtio-9p.h
+++ b/hw/9pfs/virtio-9p.h
@@ -9,7 +9,6 @@
 #include 
 #include "standard-headers/linux/virtio_9p.h"
 #include "hw/virtio/virtio.h"
-#include "hw/virtio/virtio-9p.h"
 #include "fsdev/file-op-9p.h"
 #include "fsdev/virtio-9p-marshal.h"
 #include "qemu/thread.h"
@@ -156,6 +155,13 @@ enum {
 P9_FID_XATTR,
 };
 
+typedef struct V9fsConf
+{
+/* tag name for the device */
+char *tag;
+char *fsdev_id;
+} V9fsConf;
+
 typedef struct V9fsXattr
 {
 int64_t copied_len;
diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
index a104ff20729b..7cf597461b94 100644
--- a/hw/virtio/virtio-pci.h
+++ b/hw/virtio/virtio-pci.h
@@ -23,7 +23,6 @@
 #include "hw/virtio/virtio-scsi.h"
 #include "hw/virtio/virtio-balloon.h"
 #include "hw/virtio/virtio-bus.h"
-#include "hw/virtio/virtio-9p.h"
 #include "hw/virtio/virtio-input.h"
 #include "hw/virtio/virtio-gpu.h"
 #ifdef CONFIG_VIRTFS
diff --git a/include/hw/virtio/virtio-9p.h b/include/hw/virtio/virtio-9p.h
deleted file mode 100644
index 65789db1317f..
--- a/include/hw/virtio/virtio-9p.h
+++ /dev/null
@@ -1,24 +0,0 @@
-/*
- * Virtio 9p
- *
- * Copyright IBM, Corp. 2010
- *
- * Authors:
- *  Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
- *
- * This work is licensed under the terms of the GNU GPL, version 2.  See
- * the COPYING file in the top-level directory.
- *
- */
-
-#ifndef QEMU_VIRTIO_9P_DEVICE_H
-#define QEMU_VIRTIO_9P_DEVICE_H
-
-typedef struct V9fsConf
-{
-/* tag name for the device */
-char *tag;
-char *fsdev_id;
-} V9fsConf;
-
-#endif
-- 
2.5.0

[Qemu-devel] [PATCH 22/25] 9pfs: rename virtio_9p_set_fd_limit to use v9fs_ prefix

[Aneesh Kumar K.V]

From: Wei Liu 

It's not virtio specific.

Signed-off-by: Wei Liu 
---
 hw/9pfs/virtio-9p.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
index 7fb05240987e..379fdcb2fe86 100644
--- a/hw/9pfs/virtio-9p.c
+++ b/hw/9pfs/virtio-9p.c
@@ -3266,7 +3266,7 @@ void pdu_submit(V9fsPDU *pdu)
 qemu_coroutine_enter(co, pdu);
 }
 
-static void __attribute__((__constructor__)) virtio_9p_set_fd_limit(void)
+static void __attribute__((__constructor__)) v9fs_set_fd_limit(void)
 {
 struct rlimit rlim;
 if (getrlimit(RLIMIT_NOFILE, ) < 0) {
-- 
2.5.0

[Qemu-devel] [PATCH 13/25] 9pfs: PDU processing functions should start pdu_ prefix

[Aneesh Kumar K.V]

From: Wei Liu <wei.l...@citrix.com>

This matches naming convention of pdu_marshal and pdu_unmarshal.

Signed-off-by: Wei Liu <wei.l...@citrix.com>
Signed-off-by: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
---
 hw/9pfs/virtio-9p.c | 88 ++---
 1 file changed, 44 insertions(+), 44 deletions(-)

diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
index 0a016dc11a7c..d8ce12ed8858 100644
--- a/hw/9pfs/virtio-9p.c
+++ b/hw/9pfs/virtio-9p.c
@@ -563,7 +563,7 @@ static int fid_to_qid(V9fsPDU *pdu, V9fsFidState *fidp, 
V9fsQID *qidp)
 return 0;
 }
 
-static V9fsPDU *alloc_pdu(V9fsState *s)
+static V9fsPDU *pdu_alloc(V9fsState *s)
 {
 V9fsPDU *pdu = NULL;
 
@@ -575,7 +575,7 @@ static V9fsPDU *alloc_pdu(V9fsState *s)
 return pdu;
 }
 
-static void free_pdu(V9fsPDU *pdu)
+static void pdu_free(V9fsPDU *pdu)
 {
 if (pdu) {
 V9fsState *s = pdu->s;
@@ -595,7 +595,7 @@ static void free_pdu(V9fsPDU *pdu)
  * because we always expect to have enough space to encode
  * error details
  */
-static void complete_pdu(V9fsPDU *pdu, ssize_t len)
+static void pdu_complete(V9fsPDU *pdu, ssize_t len)
 {
 int8_t id = pdu->id + 1; /* Response */
 V9fsState *s = pdu->s;
@@ -638,7 +638,7 @@ static void complete_pdu(V9fsPDU *pdu, ssize_t len)
 /* Now wakeup anybody waiting in flush for this request */
 qemu_co_queue_next(>complete);
 
-free_pdu(pdu);
+pdu_free(pdu);
 }
 
 static mode_t v9mode_to_mode(uint32_t mode, V9fsString *extension)
@@ -933,7 +933,7 @@ static void v9fs_version(void *opaque)
 offset += err;
 trace_v9fs_version_return(pdu->tag, pdu->id, s->msize, version.data);
 out:
-complete_pdu(pdu, offset);
+pdu_complete(pdu, offset);
 v9fs_string_free();
 }
 
@@ -997,7 +997,7 @@ static void v9fs_attach(void *opaque)
 out:
 put_fid(pdu, fidp);
 out_nofid:
-complete_pdu(pdu, err);
+pdu_complete(pdu, err);
 v9fs_string_free();
 v9fs_string_free();
 }
@@ -1043,7 +1043,7 @@ static void v9fs_stat(void *opaque)
 out:
 put_fid(pdu, fidp);
 out_nofid:
-complete_pdu(pdu, err);
+pdu_complete(pdu, err);
 }
 
 static void v9fs_getattr(void *opaque)
@@ -1106,7 +1106,7 @@ static void v9fs_getattr(void *opaque)
 out:
 put_fid(pdu, fidp);
 out_nofid:
-complete_pdu(pdu, retval);
+pdu_complete(pdu, retval);
 }
 
 /* Attribute flags */
@@ -1203,7 +1203,7 @@ static void v9fs_setattr(void *opaque)
 out:
 put_fid(pdu, fidp);
 out_nofid:
-complete_pdu(pdu, err);
+pdu_complete(pdu, err);
 }
 
 static int v9fs_walk_marshal(V9fsPDU *pdu, uint16_t nwnames, V9fsQID *qids)
@@ -1245,7 +1245,7 @@ static void v9fs_walk(void *opaque)
 
 err = pdu_unmarshal(pdu, offset, "ddw", , , );
 if (err < 0) {
-complete_pdu(pdu, err);
+pdu_complete(pdu, err);
 return ;
 }
 offset += err;
@@ -1313,7 +1313,7 @@ out:
 v9fs_path_free();
 v9fs_path_free();
 out_nofid:
-complete_pdu(pdu, err);
+pdu_complete(pdu, err);
 if (nwnames && nwnames <= P9_MAXWELEM) {
 for (name_idx = 0; name_idx < nwnames; name_idx++) {
 v9fs_string_free([name_idx]);
@@ -1430,7 +1430,7 @@ static void v9fs_open(void *opaque)
 out:
 put_fid(pdu, fidp);
 out_nofid:
-complete_pdu(pdu, err);
+pdu_complete(pdu, err);
 }
 
 static void v9fs_lcreate(void *opaque)
@@ -1487,7 +1487,7 @@ static void v9fs_lcreate(void *opaque)
 out:
 put_fid(pdu, fidp);
 out_nofid:
-complete_pdu(pdu, err);
+pdu_complete(pdu, err);
 v9fs_string_free();
 }
 
@@ -1517,7 +1517,7 @@ static void v9fs_fsync(void *opaque)
 }
 put_fid(pdu, fidp);
 out_nofid:
-complete_pdu(pdu, err);
+pdu_complete(pdu, err);
 }
 
 static void v9fs_clunk(void *opaque)
@@ -1550,7 +1550,7 @@ static void v9fs_clunk(void *opaque)
 err = offset;
 }
 out_nofid:
-complete_pdu(pdu, err);
+pdu_complete(pdu, err);
 }
 
 static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
@@ -1760,7 +1760,7 @@ static void v9fs_read(void *opaque)
 out:
 put_fid(pdu, fidp);
 out_nofid:
-complete_pdu(pdu, err);
+pdu_complete(pdu, err);
 }
 
 static size_t v9fs_readdir_data_size(V9fsString *name)
@@ -1883,7 +1883,7 @@ static void v9fs_readdir(void *opaque)
 out:
 put_fid(pdu, fidp);
 out_nofid:
-complete_pdu(pdu, retval);
+pdu_complete(pdu, retval);
 }
 
 static int v9fs_xattr_write(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
@@ -1950,7 +1950,7 @@ static void v9fs_write(void *opaque)
 
 err = pdu_unmarshal(pdu, offset, "dqd", , , );
 if (err < 0) {
-complete_pdu(pdu, err);
+pdu_complete(pdu, err);
 return;
 }
 offset += err;
@@ -2013,7 +2013,7 @@ out:
 put_fid(pdu, fidp);
 out_nofid:
 qemu_iovec_destroy(_full);
-complete_pdu(pdu, err);
+pdu_complete(pdu, err);
 }
 
 static void v9fs_create

[Qemu-devel] [PATCH 04/25] 9pfs: rename virtio-9p-posix-acl.c to 9p-posix-acl.c

[Aneesh Kumar K.V]

From: Wei Liu <wei.l...@citrix.com>

This file is not virtio specific. Rename it to use generic name.

Fix comment and remove unneeded inclusion of virtio.h.

Signed-off-by: Wei Liu <wei.l...@citrix.com>
Signed-off-by: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
---
 hw/9pfs/{virtio-9p-posix-acl.c => 9p-posix-acl.c} | 3 +--
 hw/9pfs/Makefile.objs | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)
 rename hw/9pfs/{virtio-9p-posix-acl.c => 9p-posix-acl.c} (98%)

diff --git a/hw/9pfs/virtio-9p-posix-acl.c b/hw/9pfs/9p-posix-acl.c
similarity index 98%
rename from hw/9pfs/virtio-9p-posix-acl.c
rename to hw/9pfs/9p-posix-acl.c
index 09dad071e487..1ee7bdc807ad 100644
--- a/hw/9pfs/virtio-9p-posix-acl.c
+++ b/hw/9pfs/9p-posix-acl.c
@@ -1,5 +1,5 @@
 /*
- * Virtio 9p system.posix* xattr callback
+ * 9p system.posix* xattr callback
  *
  * Copyright IBM, Corp. 2010
  *
@@ -13,7 +13,6 @@
 
 #include 
 #include "qemu/xattr.h"
-#include "hw/virtio/virtio.h"
 #include "virtio-9p.h"
 #include "fsdev/file-op-9p.h"
 #include "virtio-9p-xattr.h"
diff --git a/hw/9pfs/Makefile.objs b/hw/9pfs/Makefile.objs
index 505968187518..0721462d8870 100644
--- a/hw/9pfs/Makefile.objs
+++ b/hw/9pfs/Makefile.objs
@@ -1,6 +1,6 @@
 common-obj-y  = virtio-9p.o
 common-obj-y += 9p-local.o virtio-9p-xattr.o
-common-obj-y += virtio-9p-xattr-user.o virtio-9p-posix-acl.o
+common-obj-y += virtio-9p-xattr-user.o 9p-posix-acl.o
 common-obj-y += coth.o cofs.o codir.o cofile.o
 common-obj-y += coxattr.o virtio-9p-synth.o
 common-obj-$(CONFIG_OPEN_BY_HANDLE) +=  9p-handle.o
-- 
2.5.0

[Qemu-devel] [PATCH 05/25] 9pfs: rename virtio-9p-proxy.{c, h} to 9p-proxy.{c, h}

[Aneesh Kumar K.V]

From: Wei Liu <wei.l...@citrix.com>

Those two files are not virtio specific. Rename them to use generic
names.

Fix includes in various C files. Change define guards and comments
in header files.

Signed-off-by: Wei Liu <wei.l...@citrix.com>
Signed-off-by: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
---
 fsdev/virtfs-proxy-helper.c   | 2 +-
 hw/9pfs/{virtio-9p-proxy.c => 9p-proxy.c} | 5 ++---
 hw/9pfs/{virtio-9p-proxy.h => 9p-proxy.h} | 6 +++---
 hw/9pfs/Makefile.objs | 2 +-
 4 files changed, 7 insertions(+), 8 deletions(-)
 rename hw/9pfs/{virtio-9p-proxy.c => 9p-proxy.c} (99%)
 rename hw/9pfs/{virtio-9p-proxy.h => 9p-proxy.h} (95%)

diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index ad1da0d6f530..77536548d072 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -24,7 +24,7 @@
 #include "qemu/sockets.h"
 #include "qemu/xattr.h"
 #include "virtio-9p-marshal.h"
-#include "hw/9pfs/virtio-9p-proxy.h"
+#include "hw/9pfs/9p-proxy.h"
 #include "fsdev/virtio-9p-marshal.h"
 
 #define PROGNAME "virtfs-proxy-helper"
diff --git a/hw/9pfs/virtio-9p-proxy.c b/hw/9pfs/9p-proxy.c
similarity index 99%
rename from hw/9pfs/virtio-9p-proxy.c
rename to hw/9pfs/9p-proxy.c
index 1bc7881f03d5..67c1fb93f894 100644
--- a/hw/9pfs/virtio-9p-proxy.c
+++ b/hw/9pfs/9p-proxy.c
@@ -1,5 +1,5 @@
 /*
- * Virtio 9p Proxy callback
+ * 9p Proxy callback
  *
  * Copyright IBM, Corp. 2011
  *
@@ -11,11 +11,10 @@
  */
 #include 
 #include 
-#include "hw/virtio/virtio.h"
 #include "virtio-9p.h"
 #include "qemu/error-report.h"
 #include "fsdev/qemu-fsdev.h"
-#include "virtio-9p-proxy.h"
+#include "9p-proxy.h"
 
 typedef struct V9fsProxy {
 int sockfd;
diff --git a/hw/9pfs/virtio-9p-proxy.h b/hw/9pfs/9p-proxy.h
similarity index 95%
rename from hw/9pfs/virtio-9p-proxy.h
rename to hw/9pfs/9p-proxy.h
index 005c1ad75726..56150b948b4c 100644
--- a/hw/9pfs/virtio-9p-proxy.h
+++ b/hw/9pfs/9p-proxy.h
@@ -1,5 +1,5 @@
 /*
- * Virtio 9p Proxy callback
+ * 9p Proxy callback
  *
  * Copyright IBM, Corp. 2011
  *
@@ -9,8 +9,8 @@
  * This work is licensed under the terms of the GNU GPL, version 2.  See
  * the COPYING file in the top-level directory.
  */
-#ifndef _QEMU_VIRTIO_9P_PROXY_H
-#define _QEMU_VIRTIO_9P_PROXY_H
+#ifndef _QEMU_9P_PROXY_H
+#define _QEMU_9P_PROXY_H
 
 #define PROXY_MAX_IO_SZ (64 * 1024)
 #define V9FS_FD_VALID INT_MAX
diff --git a/hw/9pfs/Makefile.objs b/hw/9pfs/Makefile.objs
index 0721462d8870..cd5d146a246b 100644
--- a/hw/9pfs/Makefile.objs
+++ b/hw/9pfs/Makefile.objs
@@ -4,6 +4,6 @@ common-obj-y += virtio-9p-xattr-user.o 9p-posix-acl.o
 common-obj-y += coth.o cofs.o codir.o cofile.o
 common-obj-y += coxattr.o virtio-9p-synth.o
 common-obj-$(CONFIG_OPEN_BY_HANDLE) +=  9p-handle.o
-common-obj-y += virtio-9p-proxy.o
+common-obj-y += 9p-proxy.o
 
 obj-y += virtio-9p-device.o
-- 
2.5.0

[Qemu-devel] [PATCH 25/25] 9pfs: introduce V9fsVirtioState

[Aneesh Kumar K.V]

From: Wei Liu <wei.l...@citrix.com>

V9fsState now only contains generic fields. Introduce V9fsVirtioState
for virtio transport.  Change virtio-pci and virtio-ccw to use
V9fsVirtioState.

Signed-off-by: Wei Liu <wei.l...@citrix.com>
Signed-off-by: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
---
 hw/9pfs/9p.c   | 11 +--
 hw/9pfs/9p.h   |  6 +---
 hw/9pfs/virtio-9p-device.c | 78 +-
 hw/9pfs/virtio-9p.h| 12 ++-
 hw/s390x/virtio-ccw.h  |  2 +-
 hw/virtio/virtio-pci.h |  2 +-
 6 files changed, 71 insertions(+), 40 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index a9044039aef3..3ff310605cd4 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1585,6 +1585,8 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, 
V9fsFidState *fidp,
 size_t offset = 7;
 int read_count;
 int64_t xattr_len;
+V9fsVirtioState *v = container_of(s, V9fsVirtioState, state);
+VirtQueueElement *elem = >elems[pdu->idx];
 
 xattr_len = fidp->fs.xattr.len;
 read_count = xattr_len - off;
@@ -1601,7 +1603,8 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, 
V9fsFidState *fidp,
 return err;
 }
 offset += err;
-err = v9fs_pack(pdu->elem.in_sg, pdu->elem.in_num, offset,
+
+err = v9fs_pack(elem->in_sg, elem->in_num, offset,
 ((char *)fidp->fs.xattr.value) + off,
 read_count);
 if (err < 0) {
@@ -3269,6 +3272,7 @@ void pdu_submit(V9fsPDU *pdu)
 /* Returns 0 on success, 1 on failure. */
 int v9fs_device_realize_common(V9fsState *s, Error **errp)
 {
+V9fsVirtioState *v = container_of(s, V9fsVirtioState, state);
 int i, len;
 struct stat stat;
 FsDriverEntry *fse;
@@ -3279,8 +3283,9 @@ int v9fs_device_realize_common(V9fsState *s, Error **errp)
 QLIST_INIT(>free_list);
 QLIST_INIT(>active_list);
 for (i = 0; i < (MAX_REQ - 1); i++) {
-QLIST_INSERT_HEAD(>free_list, >pdus[i], next);
-s->pdus[i].s = s;
+QLIST_INSERT_HEAD(>free_list, >pdus[i], next);
+v->pdus[i].s = s;
+v->pdus[i].idx = i;
 }
 
 v9fs_path_init();
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index 3fe4da4e2890..edcd51be1525 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -131,9 +131,9 @@ struct V9fsPDU
 uint8_t id;
 uint8_t cancelled;
 CoQueue complete;
-VirtQueueElement elem;
 struct V9fsState *s;
 QLIST_ENTRY(V9fsPDU) next;
+uint32_t idx;
 };
 
 
@@ -205,16 +205,12 @@ struct V9fsFidState
 
 typedef struct V9fsState
 {
-VirtIODevice parent_obj;
-VirtQueue *vq;
-V9fsPDU pdus[MAX_REQ];
 QLIST_HEAD(, V9fsPDU) free_list;
 QLIST_HEAD(, V9fsPDU) active_list;
 V9fsFidState *fid_list;
 FileOperations *ops;
 FsContext ctx;
 char *tag;
-size_t config_size;
 enum p9_proto_version proto_version;
 int32_t msize;
 /*
diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index 4aa8a6b33e7a..5643fd5b13e8 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -24,33 +24,41 @@
 void virtio_9p_push_and_notify(V9fsPDU *pdu)
 {
 V9fsState *s = pdu->s;
+V9fsVirtioState *v = container_of(s, V9fsVirtioState, state);
+VirtQueueElement *elem = >elems[pdu->idx];
 
 /* push onto queue and notify */
-virtqueue_push(s->vq, >elem, pdu->size);
+virtqueue_push(v->vq, elem, pdu->size);
 
 /* FIXME: we should batch these completions */
-virtio_notify(VIRTIO_DEVICE(s), s->vq);
+virtio_notify(VIRTIO_DEVICE(v), v->vq);
 }
 
 static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
 {
-V9fsState *s = (V9fsState *)vdev;
+V9fsVirtioState *v = (V9fsVirtioState *)vdev;
+V9fsState *s = >state;
 V9fsPDU *pdu;
 ssize_t len;
 
-while ((pdu = pdu_alloc(s)) &&
-(len = virtqueue_pop(vq, >elem)) != 0) {
+while ((pdu = pdu_alloc(s))) {
 struct {
 uint32_t size_le;
 uint8_t id;
 uint16_t tag_le;
 } QEMU_PACKED out;
-int len;
+VirtQueueElement *elem = >elems[pdu->idx];
 
-BUG_ON(pdu->elem.out_num == 0 || pdu->elem.in_num == 0);
+len = virtqueue_pop(vq, elem);
+if (!len) {
+pdu_free(pdu);
+break;
+}
+
+BUG_ON(elem->out_num == 0 || elem->in_num == 0);
 QEMU_BUILD_BUG_ON(sizeof out != 7);
 
-len = iov_to_buf(pdu->elem.out_sg, pdu->elem.out_num, 0,
+len = iov_to_buf(elem->out_sg, elem->out_num, 0,
  , sizeof out);
 BUG_ON(len != sizeof out);
 
@@ -62,7 +70,6 @@ static void handle_9p_output(VirtIODevice *vdev, VirtQueue 
*vq)
 qemu_co_queue_init(>complete);
 pdu_submit(pdu);
 }
-pdu_free(pdu);
 }
 
 static uint64_t

[Qemu-devel] [PATCH 16/25] 9pfs: factor out pdu_push_and_notify

[Aneesh Kumar K.V]

From: Wei Liu <wei.l...@citrix.com>

Signed-off-by: Wei Liu <wei.l...@citrix.com>
Signed-off-by: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
---
 hw/9pfs/virtio-9p.c | 17 -
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
index 6d32b81faa25..e97adc8ba3f2 100644
--- a/hw/9pfs/virtio-9p.c
+++ b/hw/9pfs/virtio-9p.c
@@ -63,6 +63,17 @@ ssize_t pdu_unmarshal(V9fsPDU *pdu, size_t offset, const 
char *fmt, ...)
 return ret;
 }
 
+static void pdu_push_and_notify(V9fsPDU *pdu)
+{
+V9fsState *s = pdu->s;
+
+/* push onto queue and notify */
+virtqueue_push(s->vq, >elem, pdu->size);
+
+/* FIXME: we should batch these completions */
+virtio_notify(VIRTIO_DEVICE(s), s->vq);
+}
+
 static int omode_to_uflags(int8_t mode)
 {
 int ret = 0;
@@ -653,11 +664,7 @@ static void pdu_complete(V9fsPDU *pdu, ssize_t len)
 pdu->size = len;
 pdu->id = id;
 
-/* push onto queue and notify */
-virtqueue_push(s->vq, >elem, len);
-
-/* FIXME: we should batch these completions */
-virtio_notify(VIRTIO_DEVICE(s), s->vq);
+pdu_push_and_notify(pdu);
 
 /* Now wakeup anybody waiting in flush for this request */
 qemu_co_queue_next(>complete);
-- 
2.5.0

[Qemu-devel] [PATCH 06/25] 9pfs: rename virtio-9p-synth.{c, h} to 9p-synth.{c, h}

[Aneesh Kumar K.V]

From: Wei Liu <wei.l...@citrix.com>

These two files are not virtio specific. Rename them to use generic
names.

Fix includes in various C files. Change define guards and comments
in header files.

Signed-off-by: Wei Liu <wei.l...@citrix.com>
Signed-off-by: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
---
 hw/9pfs/{virtio-9p-synth.c => 9p-synth.c} | 2 +-
 hw/9pfs/{virtio-9p-synth.h => 9p-synth.h} | 6 +++---
 hw/9pfs/Makefile.objs | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)
 rename hw/9pfs/{virtio-9p-synth.c => 9p-synth.c} (99%)
 rename hw/9pfs/{virtio-9p-synth.h => 9p-synth.h} (94%)

diff --git a/hw/9pfs/virtio-9p-synth.c b/hw/9pfs/9p-synth.c
similarity index 99%
rename from hw/9pfs/virtio-9p-synth.c
rename to hw/9pfs/9p-synth.c
index a0ab9a86a905..6d34b89eef8d 100644
--- a/hw/9pfs/virtio-9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -16,7 +16,7 @@
 #include "virtio-9p.h"
 #include "virtio-9p-xattr.h"
 #include "fsdev/qemu-fsdev.h"
-#include "virtio-9p-synth.h"
+#include "9p-synth.h"
 #include "qemu/rcu.h"
 #include "qemu/rcu_queue.h"
 #include 
diff --git a/hw/9pfs/virtio-9p-synth.h b/hw/9pfs/9p-synth.h
similarity index 94%
rename from hw/9pfs/virtio-9p-synth.h
rename to hw/9pfs/9p-synth.h
index ab05a8e78c60..eaf5a0c2933f 100644
--- a/hw/9pfs/virtio-9p-synth.h
+++ b/hw/9pfs/9p-synth.h
@@ -1,5 +1,5 @@
 /*
- * Virtio 9p
+ * 9p
  *
  * Copyright IBM, Corp. 2011
  *
@@ -10,8 +10,8 @@
  * the COPYING file in the top-level directory.
  *
  */
-#ifndef HW_9PFS_VIRTIO9P_SYNTH_H
-#define HW_9PFS_VIRTIO9P_SYNTH_H 1
+#ifndef HW_9PFS_SYNTH_H
+#define HW_9PFS_SYNTH_H 1
 
 #include 
 #include 
diff --git a/hw/9pfs/Makefile.objs b/hw/9pfs/Makefile.objs
index cd5d146a246b..ba62571d5706 100644
--- a/hw/9pfs/Makefile.objs
+++ b/hw/9pfs/Makefile.objs
@@ -2,7 +2,7 @@ common-obj-y  = virtio-9p.o
 common-obj-y += 9p-local.o virtio-9p-xattr.o
 common-obj-y += virtio-9p-xattr-user.o 9p-posix-acl.o
 common-obj-y += coth.o cofs.o codir.o cofile.o
-common-obj-y += coxattr.o virtio-9p-synth.o
+common-obj-y += coxattr.o 9p-synth.o
 common-obj-$(CONFIG_OPEN_BY_HANDLE) +=  9p-handle.o
 common-obj-y += 9p-proxy.o
 
-- 
2.5.0

[Qemu-devel] [PATCH 03/25] 9pfs: rename virtio-9p-local.c to 9p-local.c

[Aneesh Kumar K.V]

From: Wei Liu <wei.l...@citrix.com>

This file is not virtio specific. Rename it to use generic name.

Fix comment and remove unneeded inclusion of virtio.h.

Signed-off-by: Wei Liu <wei.l...@citrix.com>
Signed-off-by: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
---
 hw/9pfs/{virtio-9p-local.c => 9p-local.c} | 3 +--
 hw/9pfs/Makefile.objs | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)
 rename hw/9pfs/{virtio-9p-local.c => 9p-local.c} (99%)

diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/9p-local.c
similarity index 99%
rename from hw/9pfs/virtio-9p-local.c
rename to hw/9pfs/9p-local.c
index f1f2e2573b54..877ad86c7ab8 100644
--- a/hw/9pfs/virtio-9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1,5 +1,5 @@
 /*
- * Virtio 9p Posix callback
+ * 9p Posix callback
  *
  * Copyright IBM, Corp. 2010
  *
@@ -11,7 +11,6 @@
  *
  */
 
-#include "hw/virtio/virtio.h"
 #include "virtio-9p.h"
 #include "virtio-9p-xattr.h"
 #include "fsdev/qemu-fsdev.h"   /* local_ops */
diff --git a/hw/9pfs/Makefile.objs b/hw/9pfs/Makefile.objs
index 9fdd8a4b09d0..505968187518 100644
--- a/hw/9pfs/Makefile.objs
+++ b/hw/9pfs/Makefile.objs
@@ -1,5 +1,5 @@
 common-obj-y  = virtio-9p.o
-common-obj-y += virtio-9p-local.o virtio-9p-xattr.o
+common-obj-y += 9p-local.o virtio-9p-xattr.o
 common-obj-y += virtio-9p-xattr-user.o virtio-9p-posix-acl.o
 common-obj-y += coth.o cofs.o codir.o cofile.o
 common-obj-y += coxattr.o virtio-9p-synth.o
-- 
2.5.0

[Qemu-devel] [PATCH 19/25] 9pfs: factor out virtio_9p_push_and_notify

[Aneesh Kumar K.V]

From: Wei Liu <wei.l...@citrix.com>

The new function resides in virtio specific file.

Signed-off-by: Wei Liu <wei.l...@citrix.com>
Signed-off-by: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
---
 hw/9pfs/virtio-9p-device.c | 11 +++
 hw/9pfs/virtio-9p.c|  8 +---
 hw/9pfs/virtio-9p.h|  2 ++
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index 5cad654d8e65..cfad13afe7e9 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -20,6 +20,17 @@
 #include "coth.h"
 #include "hw/virtio/virtio-access.h"
 
+void virtio_9p_push_and_notify(V9fsPDU *pdu)
+{
+V9fsState *s = pdu->s;
+
+/* push onto queue and notify */
+virtqueue_push(s->vq, >elem, pdu->size);
+
+/* FIXME: we should batch these completions */
+virtio_notify(VIRTIO_DEVICE(s), s->vq);
+}
+
 static uint64_t virtio_9p_get_features(VirtIODevice *vdev, uint64_t features,
Error **errp)
 {
diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
index 2bd862fd94da..691a1d91f21b 100644
--- a/hw/9pfs/virtio-9p.c
+++ b/hw/9pfs/virtio-9p.c
@@ -65,13 +65,7 @@ ssize_t pdu_unmarshal(V9fsPDU *pdu, size_t offset, const 
char *fmt, ...)
 
 static void pdu_push_and_notify(V9fsPDU *pdu)
 {
-V9fsState *s = pdu->s;
-
-/* push onto queue and notify */
-virtqueue_push(s->vq, >elem, pdu->size);
-
-/* FIXME: we should batch these completions */
-virtio_notify(VIRTIO_DEVICE(s), s->vq);
+virtio_9p_push_and_notify(pdu);
 }
 
 static int omode_to_uflags(int8_t mode)
diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
index b4d344af9506..a1ac3980ee73 100644
--- a/hw/9pfs/virtio-9p.h
+++ b/hw/9pfs/virtio-9p.h
@@ -6,6 +6,8 @@
 #include "9p.h"
 
 extern void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq);
+extern void virtio_9p_push_and_notify(V9fsPDU *pdu);
+
 ssize_t virtio_pdu_vmarshal(V9fsPDU *pdu, size_t offset,
 const char *fmt, va_list ap);
 ssize_t virtio_pdu_vunmarshal(V9fsPDU *pdu, size_t offset,
-- 
2.5.0

[Qemu-devel] [PATCH 10/25] fsdev: break out 9p-marshal.{c, h} from virtio-9p-marshal.{c, h}

[Aneesh Kumar K.V]

From: Wei Liu <wei.l...@citrix.com>

Break out some generic functions for marshaling 9p state. Pure code
motion plus minor fixes for build system.

Signed-off-by: Wei Liu <wei.l...@citrix.com>
Signed-off-by: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
---
 Makefile  |  2 +-
 fsdev/9p-marshal.c| 56 +++
 fsdev/9p-marshal.h| 84 +++
 fsdev/Makefile.objs   |  2 +-
 fsdev/virtio-9p-marshal.c | 31 -
 fsdev/virtio-9p-marshal.h | 79 +---
 6 files changed, 143 insertions(+), 111 deletions(-)
 create mode 100644 fsdev/9p-marshal.c
 create mode 100644 fsdev/9p-marshal.h

diff --git a/Makefile b/Makefile
index 82b2fc8b96d8..7e881d88664f 100644
--- a/Makefile
+++ b/Makefile
@@ -240,7 +240,7 @@ qemu-io$(EXESUF): qemu-io.o $(block-obj-y) $(crypto-obj-y) 
$(qom-obj-y) libqemuu
 
 qemu-bridge-helper$(EXESUF): qemu-bridge-helper.o
 
-fsdev/virtfs-proxy-helper$(EXESUF): fsdev/virtfs-proxy-helper.o 
fsdev/virtio-9p-marshal.o libqemuutil.a libqemustub.a
+fsdev/virtfs-proxy-helper$(EXESUF): fsdev/virtfs-proxy-helper.o 
fsdev/9p-marshal.o fsdev/virtio-9p-marshal.o libqemuutil.a libqemustub.a
 fsdev/virtfs-proxy-helper$(EXESUF): LIBS += -lcap
 
 qemu-img-cmds.h: $(SRC_PATH)/qemu-img-cmds.hx
diff --git a/fsdev/9p-marshal.c b/fsdev/9p-marshal.c
new file mode 100644
index ..991e35d24280
--- /dev/null
+++ b/fsdev/9p-marshal.c
@@ -0,0 +1,56 @@
+/*
+ * 9p backend
+ *
+ * Copyright IBM, Corp. 2010
+ *
+ * Authors:
+ *  Anthony Liguori   <aligu...@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "qemu/compiler.h"
+#include "9p-marshal.h"
+
+void v9fs_string_free(V9fsString *str)
+{
+g_free(str->data);
+str->data = NULL;
+str->size = 0;
+}
+
+void v9fs_string_null(V9fsString *str)
+{
+v9fs_string_free(str);
+}
+
+void GCC_FMT_ATTR(2, 3)
+v9fs_string_sprintf(V9fsString *str, const char *fmt, ...)
+{
+va_list ap;
+
+v9fs_string_free(str);
+
+va_start(ap, fmt);
+str->size = g_vasprintf(>data, fmt, ap);
+va_end(ap);
+}
+
+void v9fs_string_copy(V9fsString *lhs, V9fsString *rhs)
+{
+v9fs_string_free(lhs);
+v9fs_string_sprintf(lhs, "%s", rhs->data);
+}
diff --git a/fsdev/9p-marshal.h b/fsdev/9p-marshal.h
new file mode 100644
index ..e91b24e9ca69
--- /dev/null
+++ b/fsdev/9p-marshal.h
@@ -0,0 +1,84 @@
+#ifndef _QEMU_9P_MARSHAL_H
+#define _QEMU_9P_MARSHAL_H
+
+typedef struct V9fsString
+{
+uint16_t size;
+char *data;
+} V9fsString;
+
+typedef struct V9fsQID
+{
+int8_t type;
+int32_t version;
+int64_t path;
+} V9fsQID;
+
+typedef struct V9fsStat
+{
+int16_t size;
+int16_t type;
+int32_t dev;
+V9fsQID qid;
+int32_t mode;
+int32_t atime;
+int32_t mtime;
+int64_t length;
+V9fsString name;
+V9fsString uid;
+V9fsString gid;
+V9fsString muid;
+/* 9p2000.u */
+V9fsString extension;
+int32_t n_uid;
+int32_t n_gid;
+int32_t n_muid;
+} V9fsStat;
+
+typedef struct V9fsIattr
+{
+int32_t valid;
+int32_t mode;
+int32_t uid;
+int32_t gid;
+int64_t size;
+int64_t atime_sec;
+int64_t atime_nsec;
+int64_t mtime_sec;
+int64_t mtime_nsec;
+} V9fsIattr;
+
+typedef struct V9fsStatDotl {
+uint64_t st_result_mask;
+V9fsQID qid;
+uint32_t st_mode;
+uint32_t st_uid;
+uint32_t st_gid;
+uint64_t st_nlink;
+uint64_t st_rdev;
+uint64_t st_size;
+uint64_t st_blksize;
+uint64_t st_blocks;
+uint64_t st_atime_sec;
+uint64_t st_atime_nsec;
+uint64_t st_mtime_sec;
+uint64_t st_mtime_nsec;
+uint64_t st_ctime_sec;
+uint64_t st_ctime_nsec;
+uint64_t st_btime_sec;
+uint64_t st_btime_nsec;
+uint64_t st_gen;
+uint64_t st_data_version;
+} V9fsStatDotl;
+
+static inline void v9fs_string_init(V9fsString *str)
+{
+str->data = NULL;
+str->size = 0;
+}
+extern void v9fs_string_free(V9fsString *str);
+extern void v9fs_string_null(V9fsString *str);
+extern void v9fs_string_sprintf(V9fsString *str, const char *fmt, ...);
+extern void v9fs_string_copy(V9fsString *lhs, V9fsString *rhs);
+
+#endif
diff --git a/fsdev/Makefile.objs b/fsdev/Makefile.objs
index c27dad3f6dc7..8357851fe7ba 100644
--- a/fsdev/Makefile.objs
+++ b/fsdev/Makefile.objs
@@ -1,7 +1,7 @@
 ifeq ($(CONFIG_VIRTIO)$(CONFIG_VIRTFS)$(CONFIG_PCI),yyy)
 # Lots of the fsdev/9pcode is pulled in by vl.c via qemu_fsdev_add.
 # only pull in the actual virtio-9p device if we also enabled virtio.
-common-obj-y = qemu-fsdev.o virtio-9p-marshal.o
+common-obj-y = qemu-fsdev.o 9p-marshal.o virtio-9p-marshal.

[Qemu-devel] [PATCH 24/25] 9pfs: factor out v9fs_device_{, un}realize_common

[Aneesh Kumar K.V]

From: Wei Liu <wei.l...@citrix.com>

Signed-off-by: Wei Liu <wei.l...@citrix.com>
Signed-off-by: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
---
 hw/9pfs/9p.c   | 95 ++
 hw/9pfs/9p.h   |  2 +
 hw/9pfs/virtio-9p-device.c | 90 ---
 3 files changed, 104 insertions(+), 83 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 379fdcb2fe86..a9044039aef3 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -3266,6 +3266,101 @@ void pdu_submit(V9fsPDU *pdu)
 qemu_coroutine_enter(co, pdu);
 }
 
+/* Returns 0 on success, 1 on failure. */
+int v9fs_device_realize_common(V9fsState *s, Error **errp)
+{
+int i, len;
+struct stat stat;
+FsDriverEntry *fse;
+V9fsPath path;
+int rc = 1;
+
+/* initialize pdu allocator */
+QLIST_INIT(>free_list);
+QLIST_INIT(>active_list);
+for (i = 0; i < (MAX_REQ - 1); i++) {
+QLIST_INSERT_HEAD(>free_list, >pdus[i], next);
+s->pdus[i].s = s;
+}
+
+v9fs_path_init();
+
+fse = get_fsdev_fsentry(s->fsconf.fsdev_id);
+
+if (!fse) {
+/* We don't have a fsdev identified by fsdev_id */
+error_setg(errp, "9pfs device couldn't find fsdev with the "
+   "id = %s",
+   s->fsconf.fsdev_id ? s->fsconf.fsdev_id : "NULL");
+goto out;
+}
+
+if (!s->fsconf.tag) {
+/* we haven't specified a mount_tag */
+error_setg(errp, "fsdev with id %s needs mount_tag arguments",
+   s->fsconf.fsdev_id);
+goto out;
+}
+
+s->ctx.export_flags = fse->export_flags;
+s->ctx.fs_root = g_strdup(fse->path);
+s->ctx.exops.get_st_gen = NULL;
+len = strlen(s->fsconf.tag);
+if (len > MAX_TAG_LEN - 1) {
+error_setg(errp, "mount tag '%s' (%d bytes) is longer than "
+   "maximum (%d bytes)", s->fsconf.tag, len, MAX_TAG_LEN - 1);
+goto out;
+}
+
+s->tag = g_strdup(s->fsconf.tag);
+s->ctx.uid = -1;
+
+s->ops = fse->ops;
+
+s->fid_list = NULL;
+qemu_co_rwlock_init(>rename_lock);
+
+if (s->ops->init(>ctx) < 0) {
+error_setg(errp, "9pfs Failed to initialize fs-driver with id:%s"
+   " and export path:%s", s->fsconf.fsdev_id, s->ctx.fs_root);
+goto out;
+}
+
+/*
+ * Check details of export path, We need to use fs driver
+ * call back to do that. Since we are in the init path, we don't
+ * use co-routines here.
+ */
+if (s->ops->name_to_path(>ctx, NULL, "/", ) < 0) {
+error_setg(errp,
+   "error in converting name to path %s", strerror(errno));
+goto out;
+}
+if (s->ops->lstat(>ctx, , )) {
+error_setg(errp, "share path %s does not exist", fse->path);
+goto out;
+} else if (!S_ISDIR(stat.st_mode)) {
+error_setg(errp, "share path %s is not a directory", fse->path);
+goto out;
+}
+v9fs_path_free();
+
+rc = 0;
+out:
+if (rc) {
+g_free(s->ctx.fs_root);
+g_free(s->tag);
+v9fs_path_free();
+}
+return rc;
+}
+
+void v9fs_device_unrealize_common(V9fsState *s, Error **errp)
+{
+g_free(s->ctx.fs_root);
+g_free(s->tag);
+}
+
 static void __attribute__((__constructor__)) v9fs_set_fd_limit(void)
 {
 struct rlimit rlim;
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index b62c9a885a45..3fe4da4e2890 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -318,6 +318,8 @@ extern void v9fs_path_free(V9fsPath *path);
 extern void v9fs_path_copy(V9fsPath *lhs, V9fsPath *rhs);
 extern int v9fs_name_to_path(V9fsState *s, V9fsPath *dirpath,
  const char *name, V9fsPath *path);
+extern int v9fs_device_realize_common(V9fsState *s, Error **errp);
+extern void v9fs_device_unrealize_common(V9fsState *s, Error **errp);
 
 ssize_t pdu_marshal(V9fsPDU *pdu, size_t offset, const char *fmt, ...);
 ssize_t pdu_unmarshal(V9fsPDU *pdu, size_t offset, const char *fmt, ...);
diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index bc103b7fa757..4aa8a6b33e7a 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -101,93 +101,18 @@ static void virtio_9p_device_realize(DeviceState *dev, 
Error **errp)
 {
 VirtIODevice *vdev = VIRTIO_DEVICE(dev);
 V9fsState *s = VIRTIO_9P(dev);
-int i, len;
-struct stat stat;
-FsDriverEntry *fse;
-V9fsPath path;
-
-virtio_init(vdev, "virtio-9p", VIRTIO_ID_9P,
-sizeof(struct virtio_9p_config) + MAX_TAG_LEN);
-
-/* initialize pdu allocator */
-QLIST_INIT(>free_list);
-QLIST_INIT(>active_list);
- 

[Qemu-devel] [PATCH 20/25] 9pfs: export pdu_{submit,alloc,free}

[Aneesh Kumar K.V]

From: Wei Liu <wei.l...@citrix.com>

They will be used in later patches.

Signed-off-by: Wei Liu <wei.l...@citrix.com>
Signed-off-by: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
---
 hw/9pfs/9p.h| 3 +++
 hw/9pfs/virtio-9p.c | 6 +++---
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index 9aeb8745cf20..b62c9a885a45 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -321,5 +321,8 @@ extern int v9fs_name_to_path(V9fsState *s, V9fsPath 
*dirpath,
 
 ssize_t pdu_marshal(V9fsPDU *pdu, size_t offset, const char *fmt, ...);
 ssize_t pdu_unmarshal(V9fsPDU *pdu, size_t offset, const char *fmt, ...);
+V9fsPDU *pdu_alloc(V9fsState *s);
+void pdu_free(V9fsPDU *pdu);
+void pdu_submit(V9fsPDU *pdu);
 
 #endif
diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
index 691a1d91f21b..3d5140d1e3b6 100644
--- a/hw/9pfs/virtio-9p.c
+++ b/hw/9pfs/virtio-9p.c
@@ -592,7 +592,7 @@ static int fid_to_qid(V9fsPDU *pdu, V9fsFidState *fidp, 
V9fsQID *qidp)
 return 0;
 }
 
-static V9fsPDU *pdu_alloc(V9fsState *s)
+V9fsPDU *pdu_alloc(V9fsState *s)
 {
 V9fsPDU *pdu = NULL;
 
@@ -604,7 +604,7 @@ static V9fsPDU *pdu_alloc(V9fsState *s)
 return pdu;
 }
 
-static void pdu_free(V9fsPDU *pdu)
+void pdu_free(V9fsPDU *pdu)
 {
 if (pdu) {
 V9fsState *s = pdu->s;
@@ -3246,7 +3246,7 @@ static inline bool is_read_only_op(V9fsPDU *pdu)
 }
 }
 
-static void pdu_submit(V9fsPDU *pdu)
+void pdu_submit(V9fsPDU *pdu)
 {
 Coroutine *co;
 CoroutineEntry *handler;
-- 
2.5.0

[Qemu-devel] [PATCH 07/25] 9pfs: rename virtio-9p-xattr{, -user}.{c, h} to 9p-xattr{, -user}.{c, h}

[Aneesh Kumar K.V]

From: Wei Liu <wei.l...@citrix.com>

These three files are not virtio specific. Rename them to generic
names.

Fix comments and header inclusion in various files.

Signed-off-by: Wei Liu <wei.l...@citrix.com>
Signed-off-by: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
---
 hw/9pfs/9p-handle.c | 2 +-
 hw/9pfs/9p-local.c  | 2 +-
 hw/9pfs/9p-posix-acl.c  | 2 +-
 hw/9pfs/9p-synth.c  | 2 +-
 hw/9pfs/{virtio-9p-xattr-user.c => 9p-xattr-user.c} | 5 ++---
 hw/9pfs/{virtio-9p-xattr.c => 9p-xattr.c}   | 5 ++---
 hw/9pfs/{virtio-9p-xattr.h => 9p-xattr.h}   | 6 +++---
 hw/9pfs/Makefile.objs   | 4 ++--
 hw/9pfs/virtio-9p-device.c  | 2 +-
 hw/9pfs/virtio-9p.c | 2 +-
 10 files changed, 15 insertions(+), 17 deletions(-)
 rename hw/9pfs/{virtio-9p-xattr-user.c => 9p-xattr-user.c} (97%)
 rename hw/9pfs/{virtio-9p-xattr.c => 9p-xattr.c} (97%)
 rename hw/9pfs/{virtio-9p-xattr.h => 9p-xattr.h} (97%)

diff --git a/hw/9pfs/9p-handle.c b/hw/9pfs/9p-handle.c
index a48dbc9e8d37..51a9d15fee0d 100644
--- a/hw/9pfs/9p-handle.c
+++ b/hw/9pfs/9p-handle.c
@@ -12,7 +12,7 @@
  */
 
 #include "virtio-9p.h"
-#include "virtio-9p-xattr.h"
+#include "9p-xattr.h"
 #include 
 #include 
 #include 
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 877ad86c7ab8..ac553e0db745 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -12,7 +12,7 @@
  */
 
 #include "virtio-9p.h"
-#include "virtio-9p-xattr.h"
+#include "9p-xattr.h"
 #include "fsdev/qemu-fsdev.h"   /* local_ops */
 #include 
 #include 
diff --git a/hw/9pfs/9p-posix-acl.c b/hw/9pfs/9p-posix-acl.c
index 1ee7bdc807ad..073af39983ca 100644
--- a/hw/9pfs/9p-posix-acl.c
+++ b/hw/9pfs/9p-posix-acl.c
@@ -15,7 +15,7 @@
 #include "qemu/xattr.h"
 #include "virtio-9p.h"
 #include "fsdev/file-op-9p.h"
-#include "virtio-9p-xattr.h"
+#include "9p-xattr.h"
 
 #define MAP_ACL_ACCESS "user.virtfs.system.posix_acl_access"
 #define MAP_ACL_DEFAULT "user.virtfs.system.posix_acl_default"
diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
index 6d34b89eef8d..b1064e3aeaeb 100644
--- a/hw/9pfs/9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -14,7 +14,7 @@
 
 #include "hw/virtio/virtio.h"
 #include "virtio-9p.h"
-#include "virtio-9p-xattr.h"
+#include "9p-xattr.h"
 #include "fsdev/qemu-fsdev.h"
 #include "9p-synth.h"
 #include "qemu/rcu.h"
diff --git a/hw/9pfs/virtio-9p-xattr-user.c b/hw/9pfs/9p-xattr-user.c
similarity index 97%
rename from hw/9pfs/virtio-9p-xattr-user.c
rename to hw/9pfs/9p-xattr-user.c
index 46133e06dbdf..163b158362d9 100644
--- a/hw/9pfs/virtio-9p-xattr-user.c
+++ b/hw/9pfs/9p-xattr-user.c
@@ -1,5 +1,5 @@
 /*
- * Virtio 9p user. xattr callback
+ * 9p user. xattr callback
  *
  * Copyright IBM, Corp. 2010
  *
@@ -12,10 +12,9 @@
  */
 
 #include 
-#include "hw/virtio/virtio.h"
 #include "virtio-9p.h"
 #include "fsdev/file-op-9p.h"
-#include "virtio-9p-xattr.h"
+#include "9p-xattr.h"
 
 
 static ssize_t mp_user_getxattr(FsContext *ctx, const char *path,
diff --git a/hw/9pfs/virtio-9p-xattr.c b/hw/9pfs/9p-xattr.c
similarity index 97%
rename from hw/9pfs/virtio-9p-xattr.c
rename to hw/9pfs/9p-xattr.c
index 07183887c5ee..1d7861b27be1 100644
--- a/hw/9pfs/virtio-9p-xattr.c
+++ b/hw/9pfs/9p-xattr.c
@@ -1,5 +1,5 @@
 /*
- * Virtio 9p  xattr callback
+ * 9p  xattr callback
  *
  * Copyright IBM, Corp. 2010
  *
@@ -11,10 +11,9 @@
  *
  */
 
-#include "hw/virtio/virtio.h"
 #include "virtio-9p.h"
 #include "fsdev/file-op-9p.h"
-#include "virtio-9p-xattr.h"
+#include "9p-xattr.h"
 
 
 static XattrOperations *get_xattr_operations(XattrOperations **h,
diff --git a/hw/9pfs/virtio-9p-xattr.h b/hw/9pfs/9p-xattr.h
similarity index 97%
rename from hw/9pfs/virtio-9p-xattr.h
rename to hw/9pfs/9p-xattr.h
index 327b32b5aa9e..4d39a20262ad 100644
--- a/hw/9pfs/virtio-9p-xattr.h
+++ b/hw/9pfs/9p-xattr.h
@@ -1,5 +1,5 @@
 /*
- * Virtio 9p
+ * 9p
  *
  * Copyright IBM, Corp. 2010
  *
@@ -10,8 +10,8 @@
  * the COPYING file in the top-level directory.
  *
  */
-#ifndef _QEMU_VIRTIO_9P_XATTR_H
-#define _QEMU_VIRTIO_9P_XATTR_H
+#ifndef _QEMU_9P_XATTR_H
+#define _QEMU_9P_XATTR_H
 
 #include "qemu/xattr.h"
 
diff --git a/hw/9pfs/Makefile.objs b/hw/9pfs/Makefile.objs
index ba62571d5706..838c5e1eb951 100644
--- a/hw/9pfs/Makefile.objs
+++ b/hw/9pfs/Makefile.objs
@@ -1,6 +1,6 @@
 common-obj-y  = virtio-9p.o
-common-obj-y += 9p-local.o virtio-9p-xattr.o
-common-obj-y += virtio-9p-xattr-user.o 9p-posix-acl.o
+common-obj-y += 9p-local.o 9p-xattr.o
+comm

[Qemu-devel] [PULL 00/25] VirtFS update

[Aneesh Kumar K.V]

The following changes since commit a7e00e2536941a6e570b45b7ab4afec4505ff67e:

  petalogix-ml605: Set the MicroBlaze CPU version to 8.10.a (2016-01-07 
14:57:26 +0100)

are available in the git repository at:

  https://github.com/kvaneesh/qemu.git tags/for-upstream-signed

for you to fetch changes up to 00588a0aa2ade2e32a552633bbbefdc6ae5e32a2:

  9pfs: introduce V9fsVirtioState (2016-01-12 11:04:14 +0530)


VirtFS update:

Cleanups mostly isolating virtio related details into separate files. This
is done to enable easy addition of Xen transport for VirtFS.

The changes include:

1. Rename a bunch of files and functions to make clear they are generic.
2. disentangle virtio transport code and generic 9pfs code.
3. Some function name clean-up.



Note: We do get few checkpatch.pl warning for typedef as below.
"ERROR: open brace '{' following struct go on the same line"
But VirtFS code always had open brace in the line next to typedef.
I guess we should handle that in a separate patch

Wei Liu (25):
  9pfs: rename virtio-9p-coth.{c,h} to coth.{c,h}
  9pfs: rename virtio-9p-handle.c to 9p-handle.c
  9pfs: rename virtio-9p-local.c to 9p-local.c
  9pfs: rename virtio-9p-posix-acl.c to 9p-posix-acl.c
  9pfs: rename virtio-9p-proxy.{c,h} to 9p-proxy.{c,h}
  9pfs: rename virtio-9p-synth.{c,h} to 9p-synth.{c,h}
  9pfs: rename virtio-9p-xattr{,-user}.{c,h} to 9p-xattr{,-user}.{c,h}
  9pfs: merge hw/virtio/virtio-9p.h into hw/9pfs/virtio-9p.h
  9pfs: remove dead code
  fsdev: break out 9p-marshal.{c,h} from virtio-9p-marshal.{c,h}
  fsdev: rename virtio-9p-marshal.{c,h} to 9p-iov-marshal.{c,h}
  9pfs: PDU processing functions don't need to take V9fsState as argument
  9pfs: PDU processing functions should start pdu_ prefix
  9pfs: make pdu_{,un}marshal proper functions
  9pfs: factor out virtio_pdu_{,un}marshal
  9pfs: factor out pdu_push_and_notify
  9pfs: break out virtio_init_iov_from_pdu
  9pfs: break out 9p.h from virtio-9p.h
  9pfs: factor out virtio_9p_push_and_notify
  9pfs: export pdu_{submit,alloc,free}
  9pfs: move handle_9p_output and make it static function
  9pfs: rename virtio_9p_set_fd_limit to use v9fs_ prefix
  9pfs: rename virtio-9p.c to 9p.c
  9pfs: factor out v9fs_device_{,un}realize_common
  9pfs: introduce V9fsVirtioState

 Makefile   |   2 +-
 fsdev/{virtio-9p-marshal.c => 9p-iov-marshal.c}| 175 +
 fsdev/9p-iov-marshal.h |  18 +
 fsdev/9p-marshal.c |  56 +++
 fsdev/{virtio-9p-marshal.h => 9p-marshal.h}|  12 +-
 fsdev/Makefile.objs|   2 +-
 fsdev/virtfs-proxy-helper.c|   6 +-
 hw/9pfs/{virtio-9p-handle.c => 9p-handle.c}|   7 +-
 hw/9pfs/{virtio-9p-local.c => 9p-local.c}  |   7 +-
 hw/9pfs/{virtio-9p-posix-acl.c => 9p-posix-acl.c}  |   7 +-
 hw/9pfs/{virtio-9p-proxy.c => 9p-proxy.c}  |   7 +-
 hw/9pfs/{virtio-9p-proxy.h => 9p-proxy.h}  |  10 +-
 hw/9pfs/{virtio-9p-synth.c => 9p-synth.c}  |   6 +-
 hw/9pfs/{virtio-9p-synth.h => 9p-synth.h}  |   6 +-
 .../{virtio-9p-xattr-user.c => 9p-xattr-user.c}|   7 +-
 hw/9pfs/{virtio-9p-xattr.c => 9p-xattr.c}  |   7 +-
 hw/9pfs/{virtio-9p-xattr.h => 9p-xattr.h}  |   6 +-
 hw/9pfs/{virtio-9p.c => 9p.c}  | 260 +-
 hw/9pfs/9p.h   | 326 +
 hw/9pfs/Makefile.objs  |  14 +-
 hw/9pfs/codir.c|   2 +-
 hw/9pfs/cofile.c   |   2 +-
 hw/9pfs/cofs.c |   2 +-
 hw/9pfs/{virtio-9p-coth.c => coth.c}   |   4 +-
 hw/9pfs/{virtio-9p-coth.h => coth.h}   |   6 +-
 hw/9pfs/coxattr.c  |   2 +-
 hw/9pfs/virtio-9p-device.c | 201 ++-
 hw/9pfs/virtio-9p.h| 391 +
 hw/s390x/virtio-ccw.h  |   2 +-
 hw/virtio/virtio-pci.h |   3 +-
 include/hw/virtio/virtio-9p.h  |  24 --
 31 files changed, 835 insertions(+), 745 deletions(-)
 rename fsdev/{virtio-9p-marshal.c => 9p-iov-marshal.c} (59%)
 create mode 100644 fsdev/9p-iov-marshal.h
 create mode 100644 fsdev/9p-marshal.c
 rename fsdev/{virtio-9p-marshal.h => 9p-marshal.h} (78%)
 rename hw/9pfs/{virtio-9p-handle.c => 9p-handle.c} (99%)
 rename hw/9pfs/{virtio-9p-local.c => 9p-local.c} (99%)
 rename hw/9pfs/{virtio-9p-posix-acl.c => 9p-posix-acl.c} (97%)
 rename hw/9pfs/{virtio-9p-proxy.c => 9p-proxy.c} (99%)
 rename hw/9pfs/{virtio-9p-proxy.h => 9p-proxy.h} 

[Qemu-devel] [PATCH 14/25] 9pfs: make pdu_{, un}marshal proper functions

[Aneesh Kumar K.V]

From: Wei Liu <wei.l...@citrix.com>

Factor out v9fs_iov_v{,un}marshal. Implement pdu_{,un}marshal with those
functions.

Signed-off-by: Wei Liu <wei.l...@citrix.com>
Signed-off-by: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
---
 fsdev/9p-iov-marshal.c | 42 ++
 fsdev/9p-iov-marshal.h |  5 +
 hw/9pfs/virtio-9p.c| 26 ++
 hw/9pfs/virtio-9p.h|  6 ++
 4 files changed, 63 insertions(+), 16 deletions(-)

diff --git a/fsdev/9p-iov-marshal.c b/fsdev/9p-iov-marshal.c
index 4883b60d17ca..08d783ca1172 100644
--- a/fsdev/9p-iov-marshal.c
+++ b/fsdev/9p-iov-marshal.c
@@ -76,15 +76,13 @@ ssize_t v9fs_pack(struct iovec *in_sg, int in_num, size_t 
offset,
 return v9fs_packunpack((void *)src, in_sg, in_num, offset, size, 1);
 }
 
-ssize_t v9fs_iov_unmarshal(struct iovec *out_sg, int out_num, size_t offset,
-   int bswap, const char *fmt, ...)
+ssize_t v9fs_iov_vunmarshal(struct iovec *out_sg, int out_num, size_t offset,
+int bswap, const char *fmt, va_list ap)
 {
 int i;
-va_list ap;
 ssize_t copied = 0;
 size_t old_offset = offset;
 
-va_start(ap, fmt);
 for (i = 0; fmt[i]; i++) {
 switch (fmt[i]) {
 case 'b': {
@@ -180,25 +178,34 @@ ssize_t v9fs_iov_unmarshal(struct iovec *out_sg, int 
out_num, size_t offset,
 break;
 }
 if (copied < 0) {
-va_end(ap);
 return copied;
 }
 offset += copied;
 }
-va_end(ap);
 
 return offset - old_offset;
 }
 
-ssize_t v9fs_iov_marshal(struct iovec *in_sg, int in_num, size_t offset,
- int bswap, const char *fmt, ...)
+ssize_t v9fs_iov_unmarshal(struct iovec *out_sg, int out_num, size_t offset,
+   int bswap, const char *fmt, ...)
 {
-int i;
+ssize_t ret;
 va_list ap;
+
+va_start(ap, fmt);
+ret = v9fs_iov_vunmarshal(out_sg, out_num, offset, bswap, fmt, ap);
+va_end(ap);
+
+return ret;
+}
+
+ssize_t v9fs_iov_vmarshal(struct iovec *in_sg, int in_num, size_t offset,
+  int bswap, const char *fmt, va_list ap)
+{
+int i;
 ssize_t copied = 0;
 size_t old_offset = offset;
 
-va_start(ap, fmt);
 for (i = 0; fmt[i]; i++) {
 switch (fmt[i]) {
 case 'b': {
@@ -290,12 +297,23 @@ ssize_t v9fs_iov_marshal(struct iovec *in_sg, int in_num, 
size_t offset,
 break;
 }
 if (copied < 0) {
-va_end(ap);
 return copied;
 }
 offset += copied;
 }
-va_end(ap);
 
 return offset - old_offset;
 }
+
+ssize_t v9fs_iov_marshal(struct iovec *in_sg, int in_num, size_t offset,
+ int bswap, const char *fmt, ...)
+{
+ssize_t ret;
+va_list ap;
+
+va_start(ap, fmt);
+ret = v9fs_iov_vmarshal(in_sg, in_num, offset, bswap, fmt, ap);
+va_end(ap);
+
+return ret;
+}
diff --git a/fsdev/9p-iov-marshal.h b/fsdev/9p-iov-marshal.h
index 993614f54400..6bccbfb41a2c 100644
--- a/fsdev/9p-iov-marshal.h
+++ b/fsdev/9p-iov-marshal.h
@@ -10,4 +10,9 @@ ssize_t v9fs_iov_unmarshal(struct iovec *out_sg, int out_num, 
size_t offset,
int bswap, const char *fmt, ...);
 ssize_t v9fs_iov_marshal(struct iovec *in_sg, int in_num, size_t offset,
  int bswap, const char *fmt, ...);
+
+ssize_t v9fs_iov_vunmarshal(struct iovec *out_sg, int out_num, size_t offset,
+int bswap, const char *fmt, va_list ap);
+ssize_t v9fs_iov_vmarshal(struct iovec *in_sg, int in_num, size_t offset,
+  int bswap, const char *fmt, va_list ap);
 #endif
diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
index d8ce12ed8858..a740f85625a3 100644
--- a/hw/9pfs/virtio-9p.c
+++ b/hw/9pfs/virtio-9p.c
@@ -39,6 +39,32 @@ enum {
 Oappend = 0x80,
 };
 
+ssize_t pdu_marshal(V9fsPDU *pdu, size_t offset, const char *fmt, ...)
+{
+ssize_t ret;
+va_list ap;
+
+va_start(ap, fmt);
+ret = v9fs_iov_vmarshal(pdu->elem.in_sg, pdu->elem.in_num,
+offset, 1, fmt, ap);
+va_end(ap);
+
+return ret;
+}
+
+ssize_t pdu_unmarshal(V9fsPDU *pdu, size_t offset, const char *fmt, ...)
+{
+ssize_t ret;
+va_list ap;
+
+va_start(ap, fmt);
+ret = v9fs_iov_vunmarshal(pdu->elem.out_sg, pdu->elem.out_num,
+  offset, 1, fmt, ap);
+va_end(ap);
+
+return ret;
+}
+
 static int omode_to_uflags(int8_t mode)
 {
 int ret = 0;
diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
index 3a7e136ab691..d6f3ac08a76a 100644
--- a/hw/9pfs/virtio-9p.h
+++ b/hw/9pfs/virtio-9p.h
@@ -320,10 +320,8 @@ extern void v9fs_path_copy(V9fsPath *lhs, V9fsPath *rhs);
 extern int v9fs_name_to_path(V9fsState *s, V9fsPath *dirpath,
  const char *name, V9fsPat

[Qemu-devel] [PATCH 11/25] fsdev: rename virtio-9p-marshal.{c, h} to 9p-iov-marshal.{c, h}

[Aneesh Kumar K.V]

From: Wei Liu <wei.l...@citrix.com>

And rename v9fs_marshal to v9fs_iov_marshal, v9fs_unmarshal to
v9fs_iov_unmarshal.

The rationale behind this change is that, this marshalling interface is
used both by virtio and proxy helper. Renaming files and functions to
reflect the true nature of this interface.

Xen transport is going to have its own marshalling interface.

Signed-off-by: Wei Liu <wei.l...@citrix.com>
Signed-off-by: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
---
 Makefile|   2 +-
 fsdev/{virtio-9p-marshal.c => 9p-iov-marshal.c} | 110 +---
 fsdev/9p-iov-marshal.h  |  13 +++
 fsdev/Makefile.objs |   2 +-
 fsdev/virtfs-proxy-helper.c |   4 +-
 fsdev/virtio-9p-marshal.h   |  13 ---
 hw/9pfs/9p-proxy.h  |   4 +-
 hw/9pfs/virtio-9p.h |   6 +-
 8 files changed, 82 insertions(+), 72 deletions(-)
 rename fsdev/{virtio-9p-marshal.c => 9p-iov-marshal.c} (62%)
 create mode 100644 fsdev/9p-iov-marshal.h
 delete mode 100644 fsdev/virtio-9p-marshal.h

diff --git a/Makefile b/Makefile
index 7e881d88664f..d0de2d46b663 100644
--- a/Makefile
+++ b/Makefile
@@ -240,7 +240,7 @@ qemu-io$(EXESUF): qemu-io.o $(block-obj-y) $(crypto-obj-y) 
$(qom-obj-y) libqemuu
 
 qemu-bridge-helper$(EXESUF): qemu-bridge-helper.o
 
-fsdev/virtfs-proxy-helper$(EXESUF): fsdev/virtfs-proxy-helper.o 
fsdev/9p-marshal.o fsdev/virtio-9p-marshal.o libqemuutil.a libqemustub.a
+fsdev/virtfs-proxy-helper$(EXESUF): fsdev/virtfs-proxy-helper.o 
fsdev/9p-marshal.o fsdev/9p-iov-marshal.o libqemuutil.a libqemustub.a
 fsdev/virtfs-proxy-helper$(EXESUF): LIBS += -lcap
 
 qemu-img-cmds.h: $(SRC_PATH)/qemu-img-cmds.hx
diff --git a/fsdev/virtio-9p-marshal.c b/fsdev/9p-iov-marshal.c
similarity index 62%
rename from fsdev/virtio-9p-marshal.c
rename to fsdev/9p-iov-marshal.c
index f236bab374cb..4883b60d17ca 100644
--- a/fsdev/virtio-9p-marshal.c
+++ b/fsdev/9p-iov-marshal.c
@@ -1,5 +1,5 @@
 /*
- * Virtio 9p backend
+ * 9p backend
  *
  * Copyright IBM, Corp. 2010
  *
@@ -22,7 +22,7 @@
 #include 
 
 #include "qemu/compiler.h"
-#include "virtio-9p-marshal.h"
+#include "9p-iov-marshal.h"
 #include "qemu/bswap.h"
 
 static ssize_t v9fs_packunpack(void *addr, struct iovec *sg, int sg_count,
@@ -76,8 +76,8 @@ ssize_t v9fs_pack(struct iovec *in_sg, int in_num, size_t 
offset,
 return v9fs_packunpack((void *)src, in_sg, in_num, offset, size, 1);
 }
 
-ssize_t v9fs_unmarshal(struct iovec *out_sg, int out_num, size_t offset,
-   int bswap, const char *fmt, ...)
+ssize_t v9fs_iov_unmarshal(struct iovec *out_sg, int out_num, size_t offset,
+   int bswap, const char *fmt, ...)
 {
 int i;
 va_list ap;
@@ -127,8 +127,8 @@ ssize_t v9fs_unmarshal(struct iovec *out_sg, int out_num, 
size_t offset,
 }
 case 's': {
 V9fsString *str = va_arg(ap, V9fsString *);
-copied = v9fs_unmarshal(out_sg, out_num, offset, bswap,
-"w", >size);
+copied = v9fs_iov_unmarshal(out_sg, out_num, offset, bswap,
+"w", >size);
 if (copied > 0) {
 offset += copied;
 str->data = g_malloc(str->size + 1);
@@ -144,31 +144,36 @@ ssize_t v9fs_unmarshal(struct iovec *out_sg, int out_num, 
size_t offset,
 }
 case 'Q': {
 V9fsQID *qidp = va_arg(ap, V9fsQID *);
-copied = v9fs_unmarshal(out_sg, out_num, offset, bswap, "bdq",
->type, >version, >path);
+copied = v9fs_iov_unmarshal(out_sg, out_num, offset, bswap,
+"bdq", >type, >version,
+>path);
 break;
 }
 case 'S': {
 V9fsStat *statp = va_arg(ap, V9fsStat *);
-copied = v9fs_unmarshal(out_sg, out_num, offset, bswap,
-"wwdQdddqsddd",
->size, >type, >dev,
->qid, >mode, >atime,
->mtime, >length,
->name, >uid, >gid,
->muid, >extension,
->n_uid, >n_gid,
->n_muid);
+copied = v9fs_iov_unmarshal(out_sg, out_num, offset, bswap,
+"wwdQdddqsddd",
+>size, >type,
+>dev, >qid,
+  

[Qemu-devel] [PATCH 09/25] 9pfs: remove dead code

[Aneesh Kumar K.V]

From: Wei Liu <wei.l...@citrix.com>

Some structures in virtio-9p.h have been unused since 2011 when relevant
functions switched to use coroutines.

The declaration of pdu_packunpack and function do_pdu_unpack are
useless.

Signed-off-by: Wei Liu <wei.l...@citrix.com>
Signed-off-by: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
---
 hw/9pfs/virtio-9p.h | 68 -
 1 file changed, 68 deletions(-)

diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
index ac4cb006b30e..3c78d3cee1d0 100644
--- a/hw/9pfs/virtio-9p.h
+++ b/hw/9pfs/virtio-9p.h
@@ -227,65 +227,6 @@ typedef struct V9fsState
 V9fsConf fsconf;
 } V9fsState;
 
-typedef struct V9fsStatState {
-V9fsPDU *pdu;
-size_t offset;
-V9fsStat v9stat;
-V9fsFidState *fidp;
-struct stat stbuf;
-} V9fsStatState;
-
-typedef struct V9fsOpenState {
-V9fsPDU *pdu;
-size_t offset;
-int32_t mode;
-V9fsFidState *fidp;
-V9fsQID qid;
-struct stat stbuf;
-int iounit;
-} V9fsOpenState;
-
-typedef struct V9fsReadState {
-V9fsPDU *pdu;
-size_t offset;
-int32_t count;
-int32_t total;
-int64_t off;
-V9fsFidState *fidp;
-struct iovec iov[128]; /* FIXME: bad, bad, bad */
-struct iovec *sg;
-off_t dir_pos;
-struct dirent *dent;
-struct stat stbuf;
-V9fsString name;
-V9fsStat v9stat;
-int32_t len;
-int32_t cnt;
-int32_t max_count;
-} V9fsReadState;
-
-typedef struct V9fsWriteState {
-V9fsPDU *pdu;
-size_t offset;
-int32_t len;
-int32_t count;
-int32_t total;
-int64_t off;
-V9fsFidState *fidp;
-struct iovec iov[128]; /* FIXME: bad, bad, bad */
-struct iovec *sg;
-int cnt;
-} V9fsWriteState;
-
-typedef struct V9fsMkState {
-V9fsPDU *pdu;
-size_t offset;
-V9fsQID qid;
-struct stat stbuf;
-V9fsString name;
-V9fsString fullname;
-} V9fsMkState;
-
 /* 9p2000.L open flags */
 #define P9_DOTL_RDONLY
 #define P9_DOTL_WRONLY0001
@@ -345,15 +286,6 @@ typedef struct V9fsGetlock
 extern int open_fd_hw;
 extern int total_open_fd;
 
-size_t pdu_packunpack(void *addr, struct iovec *sg, int sg_count,
-  size_t offset, size_t size, int pack);
-
-static inline size_t do_pdu_unpack(void *dst, struct iovec *sg, int sg_count,
-size_t offset, size_t size)
-{
-return pdu_packunpack(dst, sg, sg_count, offset, size, 0);
-}
-
 static inline void v9fs_path_write_lock(V9fsState *s)
 {
 if (s->ctx.export_flags & V9FS_PATHNAME_FSCONTEXT) {
-- 
2.5.0

[Qemu-devel] [PATCH 21/25] 9pfs: move handle_9p_output and make it static function

[Aneesh Kumar K.V]

From: Wei Liu <wei.l...@citrix.com>

It's only used in virtio device.

Signed-off-by: Wei Liu <wei.l...@citrix.com>
Signed-off-by: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
---
 hw/9pfs/virtio-9p-device.c | 34 ++
 hw/9pfs/virtio-9p.c| 33 -
 hw/9pfs/virtio-9p.h|  1 -
 3 files changed, 34 insertions(+), 34 deletions(-)

diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index cfad13afe7e9..bc103b7fa757 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -19,6 +19,7 @@
 #include "9p-xattr.h"
 #include "coth.h"
 #include "hw/virtio/virtio-access.h"
+#include "qemu/iov.h"
 
 void virtio_9p_push_and_notify(V9fsPDU *pdu)
 {
@@ -31,6 +32,39 @@ void virtio_9p_push_and_notify(V9fsPDU *pdu)
 virtio_notify(VIRTIO_DEVICE(s), s->vq);
 }
 
+static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
+{
+V9fsState *s = (V9fsState *)vdev;
+V9fsPDU *pdu;
+ssize_t len;
+
+while ((pdu = pdu_alloc(s)) &&
+(len = virtqueue_pop(vq, >elem)) != 0) {
+struct {
+uint32_t size_le;
+uint8_t id;
+uint16_t tag_le;
+} QEMU_PACKED out;
+int len;
+
+BUG_ON(pdu->elem.out_num == 0 || pdu->elem.in_num == 0);
+QEMU_BUILD_BUG_ON(sizeof out != 7);
+
+len = iov_to_buf(pdu->elem.out_sg, pdu->elem.out_num, 0,
+ , sizeof out);
+BUG_ON(len != sizeof out);
+
+pdu->size = le32_to_cpu(out.size_le);
+
+pdu->id = out.id;
+pdu->tag = le16_to_cpu(out.tag_le);
+
+qemu_co_queue_init(>complete);
+pdu_submit(pdu);
+}
+pdu_free(pdu);
+}
+
 static uint64_t virtio_9p_get_features(VirtIODevice *vdev, uint64_t features,
Error **errp)
 {
diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
index 3d5140d1e3b6..7fb05240987e 100644
--- a/hw/9pfs/virtio-9p.c
+++ b/hw/9pfs/virtio-9p.c
@@ -3266,39 +3266,6 @@ void pdu_submit(V9fsPDU *pdu)
 qemu_coroutine_enter(co, pdu);
 }
 
-void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
-{
-V9fsState *s = (V9fsState *)vdev;
-V9fsPDU *pdu;
-ssize_t len;
-
-while ((pdu = pdu_alloc(s)) &&
-(len = virtqueue_pop(vq, >elem)) != 0) {
-struct {
-uint32_t size_le;
-uint8_t id;
-uint16_t tag_le;
-} QEMU_PACKED out;
-int len;
-
-BUG_ON(pdu->elem.out_num == 0 || pdu->elem.in_num == 0);
-QEMU_BUILD_BUG_ON(sizeof out != 7);
-
-len = iov_to_buf(pdu->elem.out_sg, pdu->elem.out_num, 0,
- , sizeof out);
-BUG_ON(len != sizeof out);
-
-pdu->size = le32_to_cpu(out.size_le);
-
-pdu->id = out.id;
-pdu->tag = le16_to_cpu(out.tag_le);
-
-qemu_co_queue_init(>complete);
-pdu_submit(pdu);
-}
-pdu_free(pdu);
-}
-
 static void __attribute__((__constructor__)) virtio_9p_set_fd_limit(void)
 {
 struct rlimit rlim;
diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
index a1ac3980ee73..474ab94c0859 100644
--- a/hw/9pfs/virtio-9p.h
+++ b/hw/9pfs/virtio-9p.h
@@ -5,7 +5,6 @@
 #include "hw/virtio/virtio.h"
 #include "9p.h"
 
-extern void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq);
 extern void virtio_9p_push_and_notify(V9fsPDU *pdu);
 
 ssize_t virtio_pdu_vmarshal(V9fsPDU *pdu, size_t offset,
-- 
2.5.0

[Qemu-devel] [PATCH 18/25] 9pfs: break out 9p.h from virtio-9p.h

[Aneesh Kumar K.V]

From: Wei Liu <wei.l...@citrix.com>

Move out generic definitions from virtio-9p.h to 9p.h. Fix header
inclusions.

Signed-off-by: Wei Liu <wei.l...@citrix.com>
Signed-off-by: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
---
 hw/9pfs/9p-handle.c |   2 +-
 hw/9pfs/9p-local.c  |   2 +-
 hw/9pfs/9p-posix-acl.c  |   2 +-
 hw/9pfs/9p-proxy.c  |   2 +-
 hw/9pfs/9p-synth.c  |   2 +-
 hw/9pfs/9p-xattr-user.c |   2 +-
 hw/9pfs/9p-xattr.c  |   2 +-
 hw/9pfs/9p.h| 325 
 hw/9pfs/virtio-9p.h | 319 +--
 9 files changed, 333 insertions(+), 325 deletions(-)
 create mode 100644 hw/9pfs/9p.h

diff --git a/hw/9pfs/9p-handle.c b/hw/9pfs/9p-handle.c
index 51a9d15fee0d..58b77b4c942d 100644
--- a/hw/9pfs/9p-handle.c
+++ b/hw/9pfs/9p-handle.c
@@ -11,7 +11,7 @@
  *
  */
 
-#include "virtio-9p.h"
+#include "9p.h"
 #include "9p-xattr.h"
 #include 
 #include 
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index ac553e0db745..bf63eab729ad 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -11,7 +11,7 @@
  *
  */
 
-#include "virtio-9p.h"
+#include "9p.h"
 #include "9p-xattr.h"
 #include "fsdev/qemu-fsdev.h"   /* local_ops */
 #include 
diff --git a/hw/9pfs/9p-posix-acl.c b/hw/9pfs/9p-posix-acl.c
index 073af39983ca..8df822833c52 100644
--- a/hw/9pfs/9p-posix-acl.c
+++ b/hw/9pfs/9p-posix-acl.c
@@ -13,7 +13,7 @@
 
 #include 
 #include "qemu/xattr.h"
-#include "virtio-9p.h"
+#include "9p.h"
 #include "fsdev/file-op-9p.h"
 #include "9p-xattr.h"
 
diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
index 67c1fb93f894..73d00dd74d11 100644
--- a/hw/9pfs/9p-proxy.c
+++ b/hw/9pfs/9p-proxy.c
@@ -11,7 +11,7 @@
  */
 #include 
 #include 
-#include "virtio-9p.h"
+#include "9p.h"
 #include "qemu/error-report.h"
 #include "fsdev/qemu-fsdev.h"
 #include "9p-proxy.h"
diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
index b1064e3aeaeb..090ae0cd4c31 100644
--- a/hw/9pfs/9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -13,7 +13,7 @@
  */
 
 #include "hw/virtio/virtio.h"
-#include "virtio-9p.h"
+#include "9p.h"
 #include "9p-xattr.h"
 #include "fsdev/qemu-fsdev.h"
 #include "9p-synth.h"
diff --git a/hw/9pfs/9p-xattr-user.c b/hw/9pfs/9p-xattr-user.c
index 163b158362d9..c490ec3bf0a6 100644
--- a/hw/9pfs/9p-xattr-user.c
+++ b/hw/9pfs/9p-xattr-user.c
@@ -12,7 +12,7 @@
  */
 
 #include 
-#include "virtio-9p.h"
+#include "9p.h"
 #include "fsdev/file-op-9p.h"
 #include "9p-xattr.h"
 
diff --git a/hw/9pfs/9p-xattr.c b/hw/9pfs/9p-xattr.c
index 1d7861b27be1..741dd03744be 100644
--- a/hw/9pfs/9p-xattr.c
+++ b/hw/9pfs/9p-xattr.c
@@ -11,7 +11,7 @@
  *
  */
 
-#include "virtio-9p.h"
+#include "9p.h"
 #include "fsdev/file-op-9p.h"
 #include "9p-xattr.h"
 
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
new file mode 100644
index ..9aeb8745cf20
--- /dev/null
+++ b/hw/9pfs/9p.h
@@ -0,0 +1,325 @@
+#ifndef _QEMU_9P_H
+#define _QEMU_9P_H
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "standard-headers/linux/virtio_9p.h"
+#include "hw/virtio/virtio.h"
+#include "fsdev/file-op-9p.h"
+#include "fsdev/9p-iov-marshal.h"
+#include "qemu/thread.h"
+#include "qemu/coroutine.h"
+
+enum {
+P9_TLERROR = 6,
+P9_RLERROR,
+P9_TSTATFS = 8,
+P9_RSTATFS,
+P9_TLOPEN = 12,
+P9_RLOPEN,
+P9_TLCREATE = 14,
+P9_RLCREATE,
+P9_TSYMLINK = 16,
+P9_RSYMLINK,
+P9_TMKNOD = 18,
+P9_RMKNOD,
+P9_TRENAME = 20,
+P9_RRENAME,
+P9_TREADLINK = 22,
+P9_RREADLINK,
+P9_TGETATTR = 24,
+P9_RGETATTR,
+P9_TSETATTR = 26,
+P9_RSETATTR,
+P9_TXATTRWALK = 30,
+P9_RXATTRWALK,
+P9_TXATTRCREATE = 32,
+P9_RXATTRCREATE,
+P9_TREADDIR = 40,
+P9_RREADDIR,
+P9_TFSYNC = 50,
+P9_RFSYNC,
+P9_TLOCK = 52,
+P9_RLOCK,
+P9_TGETLOCK = 54,
+P9_RGETLOCK,
+P9_TLINK = 70,
+P9_RLINK,
+P9_TMKDIR = 72,
+P9_RMKDIR,
+P9_TRENAMEAT = 74,
+P9_RRENAMEAT,
+P9_TUNLINKAT = 76,
+P9_RUNLINKAT,
+P9_TVERSION = 100,
+P9_RVERSION,
+P9_TAUTH = 102,
+P9_RAUTH,
+P9_TATTACH = 104,
+P9_RATTACH,
+P9_TERROR = 106,
+P9_RERROR,
+P9_TFLUSH = 108,
+P9_RFLUSH,
+P9_TWALK = 110,
+P9_RWALK,
+P9_TOPEN = 112,
+P9_ROPEN,
+P9_TCREATE = 114,
+P9_RCREATE,
+P9_TREAD = 116,
+P9_RREAD,
+P9_TWRITE = 118,
+P9_RWRITE,
+P9_TCLUNK = 120,
+P9_RCLUNK,
+P9_TREMOVE = 122,
+P9_RREMOVE,
+P9_TSTAT = 124,
+P9_RSTAT,
+P9_TWSTAT = 126,
+P9_RWSTAT,
+};
+
+
+/* qid.types *

[Qemu-devel] [PATCH 17/25] 9pfs: break out virtio_init_iov_from_pdu

[Aneesh Kumar K.V]

From: Wei Liu <wei.l...@citrix.com>

Signed-off-by: Wei Liu <wei.l...@citrix.com>
Signed-off-by: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
---
 hw/9pfs/virtio-9p-device.c | 12 
 hw/9pfs/virtio-9p.c|  8 +---
 hw/9pfs/virtio-9p.h|  2 ++
 3 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index d77247f3cdad..5cad654d8e65 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -170,6 +170,18 @@ ssize_t virtio_pdu_vunmarshal(V9fsPDU *pdu, size_t offset,
offset, 1, fmt, ap);
 }
 
+void virtio_init_iov_from_pdu(V9fsPDU *pdu, struct iovec **piov,
+  unsigned int *pniov, bool is_write)
+{
+if (is_write) {
+*piov = pdu->elem.out_sg;
+*pniov = pdu->elem.out_num;
+} else {
+*piov = pdu->elem.in_sg;
+*pniov = pdu->elem.in_num;
+}
+}
+
 /* virtio-9p device */
 
 static Property virtio_9p_properties[] = {
diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
index e97adc8ba3f2..2bd862fd94da 100644
--- a/hw/9pfs/virtio-9p.c
+++ b/hw/9pfs/virtio-9p.c
@@ -1697,13 +1697,7 @@ static void v9fs_init_qiov_from_pdu(QEMUIOVector *qiov, 
V9fsPDU *pdu,
 struct iovec *iov;
 unsigned int niov;
 
-if (is_write) {
-iov = pdu->elem.out_sg;
-niov = pdu->elem.out_num;
-} else {
-iov = pdu->elem.in_sg;
-niov = pdu->elem.in_num;
-}
+virtio_init_iov_from_pdu(pdu, , , is_write);
 
 qemu_iovec_init_external(, iov, niov);
 qemu_iovec_init(qiov, niov);
diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
index e298949fde40..5024ad0460dc 100644
--- a/hw/9pfs/virtio-9p.h
+++ b/hw/9pfs/virtio-9p.h
@@ -327,6 +327,8 @@ ssize_t virtio_pdu_vmarshal(V9fsPDU *pdu, size_t offset,
 const char *fmt, va_list ap);
 ssize_t virtio_pdu_vunmarshal(V9fsPDU *pdu, size_t offset,
   const char *fmt, va_list ap);
+void virtio_init_iov_from_pdu(V9fsPDU *pdu, struct iovec **piov,
+  unsigned int *pniov, bool is_write);
 
 #define TYPE_VIRTIO_9P "virtio-9p-device"
 #define VIRTIO_9P(obj) \
-- 
2.5.0

[Qemu-devel] [PATCH 23/25] 9pfs: rename virtio-9p.c to 9p.c

[Aneesh Kumar K.V]

From: Wei Liu <wei.l...@citrix.com>

Now that file only contains generic code.

Signed-off-by: Wei Liu <wei.l...@citrix.com>
Signed-off-by: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
---
 hw/9pfs/{virtio-9p.c => 9p.c} | 0
 hw/9pfs/Makefile.objs | 2 +-
 2 files changed, 1 insertion(+), 1 deletion(-)
 rename hw/9pfs/{virtio-9p.c => 9p.c} (100%)

diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/9p.c
similarity index 100%
rename from hw/9pfs/virtio-9p.c
rename to hw/9pfs/9p.c
diff --git a/hw/9pfs/Makefile.objs b/hw/9pfs/Makefile.objs
index 838c5e1eb951..da0ae0cfdbae 100644
--- a/hw/9pfs/Makefile.objs
+++ b/hw/9pfs/Makefile.objs
@@ -1,4 +1,4 @@
-common-obj-y  = virtio-9p.o
+common-obj-y  = 9p.o
 common-obj-y += 9p-local.o 9p-xattr.o
 common-obj-y += 9p-xattr-user.o 9p-posix-acl.o
 common-obj-y += coth.o cofs.o codir.o cofile.o
-- 
2.5.0

[Qemu-devel] [PATCH 02/25] 9pfs: rename virtio-9p-handle.c to 9p-handle.c

[Aneesh Kumar K.V]

From: Wei Liu <wei.l...@citrix.com>

This file is not virtio specific. Rename it to use generic name.

Fix comment and remove unneeded inclusion of virtio.h.

Signed-off-by: Wei Liu <wei.l...@citrix.com>
Signed-off-by: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
---
 hw/9pfs/{virtio-9p-handle.c => 9p-handle.c} | 3 +--
 hw/9pfs/Makefile.objs   | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)
 rename hw/9pfs/{virtio-9p-handle.c => 9p-handle.c} (99%)

diff --git a/hw/9pfs/virtio-9p-handle.c b/hw/9pfs/9p-handle.c
similarity index 99%
rename from hw/9pfs/virtio-9p-handle.c
rename to hw/9pfs/9p-handle.c
index 13eabb98a401..a48dbc9e8d37 100644
--- a/hw/9pfs/virtio-9p-handle.c
+++ b/hw/9pfs/9p-handle.c
@@ -1,5 +1,5 @@
 /*
- * Virtio 9p handle callback
+ * 9p handle callback
  *
  * Copyright IBM, Corp. 2011
  *
@@ -11,7 +11,6 @@
  *
  */
 
-#include "hw/virtio/virtio.h"
 #include "virtio-9p.h"
 #include "virtio-9p-xattr.h"
 #include 
diff --git a/hw/9pfs/Makefile.objs b/hw/9pfs/Makefile.objs
index 76dadbe1f2c7..9fdd8a4b09d0 100644
--- a/hw/9pfs/Makefile.objs
+++ b/hw/9pfs/Makefile.objs
@@ -3,7 +3,7 @@ common-obj-y += virtio-9p-local.o virtio-9p-xattr.o
 common-obj-y += virtio-9p-xattr-user.o virtio-9p-posix-acl.o
 common-obj-y += coth.o cofs.o codir.o cofile.o
 common-obj-y += coxattr.o virtio-9p-synth.o
-common-obj-$(CONFIG_OPEN_BY_HANDLE) +=  virtio-9p-handle.o
+common-obj-$(CONFIG_OPEN_BY_HANDLE) +=  9p-handle.o
 common-obj-y += virtio-9p-proxy.o
 
 obj-y += virtio-9p-device.o
-- 
2.5.0

Re: [Qemu-devel] [PATCH v3 2/3] 9pfs: use V9fsBlob to transmit xattr

[Aneesh Kumar K.V]

Wei Liu  writes:

> And make v9fs_pack static function. Now we only need to export
> v9fs_{,un}marshal to device.
>
> Signed-off-by: Wei Liu 
> ---
> v3: fix bug discovered by Aneesh
> ---
>  fsdev/9p-iov-marshal.c |  4 ++--
>  fsdev/9p-iov-marshal.h |  3 ---
>  hw/9pfs/9p.c   | 21 +
>  3 files changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/fsdev/9p-iov-marshal.c b/fsdev/9p-iov-marshal.c
> index 1f9edf3..5c911c8 100644
> --- a/fsdev/9p-iov-marshal.c
> +++ b/fsdev/9p-iov-marshal.c
> @@ -70,8 +70,8 @@ static ssize_t v9fs_unpack(void *dst, struct iovec *out_sg, 
> int out_num,
>  return v9fs_packunpack(dst, out_sg, out_num, offset, size, 0);
>  }
>
> -ssize_t v9fs_pack(struct iovec *in_sg, int in_num, size_t offset,
> -  const void *src, size_t size)
> +static ssize_t v9fs_pack(struct iovec *in_sg, int in_num, size_t offset,
> + const void *src, size_t size)
>  {
>  return v9fs_packunpack((void *)src, in_sg, in_num, offset, size, 1);
>  }
> diff --git a/fsdev/9p-iov-marshal.h b/fsdev/9p-iov-marshal.h
> index 6bccbfb..410a1ea 100644
> --- a/fsdev/9p-iov-marshal.h
> +++ b/fsdev/9p-iov-marshal.h
> @@ -3,9 +3,6 @@
>
>  #include "9p-marshal.h"
>
> -
> -ssize_t v9fs_pack(struct iovec *in_sg, int in_num, size_t offset,
> -  const void *src, size_t size);
>  ssize_t v9fs_iov_unmarshal(struct iovec *out_sg, int out_num, size_t offset,
> int bswap, const char *fmt, ...);
>  ssize_t v9fs_iov_marshal(struct iovec *in_sg, int in_num, size_t offset,
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index a904403..84cb1d9 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -1585,6 +1585,7 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, 
> V9fsFidState *fidp,
>  size_t offset = 7;
>  int read_count;
>  int64_t xattr_len;
> +V9fsBlob blob;
>
>  xattr_len = fidp->fs.xattr.len;
>  read_count = xattr_len - off;
> @@ -1596,14 +1597,18 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU 
> *pdu, V9fsFidState *fidp,
>   */
>  read_count = 0;
>  }
> -err = pdu_marshal(pdu, offset, "d", read_count);
> -if (err < 0) {
> -return err;
> -}
> -offset += err;
> -err = v9fs_pack(pdu->elem.in_sg, pdu->elem.in_num, offset,
> -((char *)fidp->fs.xattr.value) + off,
> -read_count);
> +
> +v9fs_blob_init();
> +
> +blob.data = g_malloc(read_count);
> +memcpy(blob.data, ((char *)fidp->fs.xattr.value) + off,
> +   read_count);

Can we do this without the malloc and memcpy ? . I am sure you need this
for Xen abstraction. But for now i am inclined to drop this from the
series and add this later with Xen transport.  v9fs_xattr_read is
essentially T_READ on a xattr fid and we don't use blob ("B") in other
code path. We also want to avoid that extra malloc and memcpy.



> +blob.size = read_count;
> +
> +err = pdu_marshal(pdu, offset, "B", );
> +
> +v9fs_blob_free();
> +
>  if (err < 0) {
>  return err;
>  }
> -- 
> 2.1.4

-aneesh

Re: [Qemu-devel] [PATCH v3 2/3] 9pfs: use V9fsBlob to transmit xattr

[Aneesh Kumar K.V]

Wei Liu <wei.l...@citrix.com> writes:

> On Mon, Jan 11, 2016 at 07:26:39PM +0530, Aneesh Kumar K.V wrote:
> [...]
>> >  xattr_len = fidp->fs.xattr.len;
>> >  read_count = xattr_len - off;
>> > @@ -1596,14 +1597,18 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU 
>> > *pdu, V9fsFidState *fidp,
>> >   */
>> >  read_count = 0;
>> >  }
>> > -err = pdu_marshal(pdu, offset, "d", read_count);
>> > -if (err < 0) {
>> > -return err;
>> > -}
>> > -offset += err;
>> > -err = v9fs_pack(pdu->elem.in_sg, pdu->elem.in_num, offset,
>> > -((char *)fidp->fs.xattr.value) + off,
>> > -read_count);
>> > +
>> > +v9fs_blob_init();
>> > +
>> > +blob.data = g_malloc(read_count);
>> > +memcpy(blob.data, ((char *)fidp->fs.xattr.value) + off,
>> > +   read_count);
>> 
>> Can we do this without the malloc and memcpy ? . I am sure you need this
>> for Xen abstraction. But for now i am inclined to drop this from the
>> series and add this later with Xen transport.  v9fs_xattr_read is
>> essentially T_READ on a xattr fid and we don't use blob ("B") in other
>> code path. We also want to avoid that extra malloc and memcpy.
>> 
>
> That's fine.
>
> Do you want me to resend the whole series or just this one patch
> (assuming you don't have other comments on my other patches)?
>

No. I did this change on top of patch 3

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index a772963fb742..3ff310605cd4 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1585,6 +1585,8 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, 
V9fsFidState *fidp,
 size_t offset = 7;
 int read_count;
 int64_t xattr_len;
+V9fsVirtioState *v = container_of(s, V9fsVirtioState, state);
+VirtQueueElement *elem = >elems[pdu->idx];
 
 xattr_len = fidp->fs.xattr.len;
 read_count = xattr_len - off;
@@ -1601,7 +1603,8 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, 
V9fsFidState *fidp,
 return err;
 }
 offset += err;
-err = v9fs_pack(pdu->elem.in_sg, pdu->elem.in_num, offset,
+
+err = v9fs_pack(elem->in_sg, elem->in_num, offset,
 ((char *)fidp->fs.xattr.value) + off,
 read_count);
 if (err < 0) {

Re: [Qemu-devel] [PATCH v2 12/27] 9pfs: use V9fsBlob to transmit xattr

[Aneesh Kumar K.V]

Wei Liu  writes:

> And make v9fs_pack static function. Now we only need to export
> v9fs_{,un}marshal to device.
>
> Signed-off-by: Wei Liu 
> ---
>  fsdev/virtio-9p-marshal.c |  4 ++--
>  fsdev/virtio-9p-marshal.h |  3 ---
>  hw/9pfs/virtio-9p.c   | 21 +
>  3 files changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/fsdev/virtio-9p-marshal.c b/fsdev/virtio-9p-marshal.c
> index c3ac316..d120bd2 100644
> --- a/fsdev/virtio-9p-marshal.c
> +++ b/fsdev/virtio-9p-marshal.c
> @@ -70,8 +70,8 @@ static ssize_t v9fs_unpack(void *dst, struct iovec *out_sg, 
> int out_num,
>  return v9fs_packunpack(dst, out_sg, out_num, offset, size, 0);
>  }
>
> -ssize_t v9fs_pack(struct iovec *in_sg, int in_num, size_t offset,
> -  const void *src, size_t size)
> +static ssize_t v9fs_pack(struct iovec *in_sg, int in_num, size_t offset,
> + const void *src, size_t size)
>  {
>  return v9fs_packunpack((void *)src, in_sg, in_num, offset, size, 1);
>  }
> diff --git a/fsdev/virtio-9p-marshal.h b/fsdev/virtio-9p-marshal.h
> index 0709bcd..766a48e 100644
> --- a/fsdev/virtio-9p-marshal.h
> +++ b/fsdev/virtio-9p-marshal.h
> @@ -3,9 +3,6 @@
>
>  #include "9p-marshal.h"
>
> -
> -ssize_t v9fs_pack(struct iovec *in_sg, int in_num, size_t offset,
> -  const void *src, size_t size);
>  ssize_t v9fs_unmarshal(struct iovec *out_sg, int out_num, size_t offset,
> int bswap, const char *fmt, ...);
>  ssize_t v9fs_marshal(struct iovec *in_sg, int in_num, size_t offset,
> diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> index 30ff828..654c103 100644
> --- a/hw/9pfs/virtio-9p.c
> +++ b/hw/9pfs/virtio-9p.c
> @@ -1561,6 +1561,7 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, 
> V9fsFidState *fidp,
>  size_t offset = 7;
>  int read_count;
>  int64_t xattr_len;
> +V9fsBlob blob;
>
>  xattr_len = fidp->fs.xattr.len;
>  read_count = xattr_len - off;
> @@ -1572,14 +1573,18 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU 
> *pdu, V9fsFidState *fidp,
>   */
>  read_count = 0;
>  }
> -err = pdu_marshal(pdu, offset, "d", read_count);
> -if (err < 0) {
> -return err;
> -}
> -offset += err;
> -err = v9fs_pack(pdu->elem.in_sg, pdu->elem.in_num, offset,
> -((char *)fidp->fs.xattr.value) + off,
> -read_count);
> +
> +v9fs_blob_init();
> +
> +blob.data = g_malloc(read_count);
> +memcpy(blob.data, ((char *)fidp->fs.xattr.value) + off,
> +   read_count);
> +blob.size = read_count;
> +
> +err = pdu_marshal(pdu, offset, "dB", read_count, );

Is this correct ?

earlier we had read_count 
now we have
read_count,  which is read_count, blob->size, data


-aneesh

Re: [Qemu-devel] [PATCH v2 12/27] 9pfs: use V9fsBlob to transmit xattr

[Aneesh Kumar K.V]

Wei Liu <wei.l...@citrix.com> writes:

> On Fri, Jan 08, 2016 at 02:00:31PM +0530, Aneesh Kumar K.V wrote:
>> Wei Liu <wei.l...@citrix.com> writes:
>> 
>> > And make v9fs_pack static function. Now we only need to export
>> > v9fs_{,un}marshal to device.
>> >
>> > Signed-off-by: Wei Liu <wei.l...@citrix.com>
>> > ---
>> >  fsdev/virtio-9p-marshal.c |  4 ++--
>> >  fsdev/virtio-9p-marshal.h |  3 ---
>> >  hw/9pfs/virtio-9p.c   | 21 +
>> >  3 files changed, 15 insertions(+), 13 deletions(-)
>> >
>> > diff --git a/fsdev/virtio-9p-marshal.c b/fsdev/virtio-9p-marshal.c
>> > index c3ac316..d120bd2 100644
>> > --- a/fsdev/virtio-9p-marshal.c
>> > +++ b/fsdev/virtio-9p-marshal.c
>> > @@ -70,8 +70,8 @@ static ssize_t v9fs_unpack(void *dst, struct iovec 
>> > *out_sg, int out_num,
>> >  return v9fs_packunpack(dst, out_sg, out_num, offset, size, 0);
>> >  }
>> >
>> > -ssize_t v9fs_pack(struct iovec *in_sg, int in_num, size_t offset,
>> > -  const void *src, size_t size)
>> > +static ssize_t v9fs_pack(struct iovec *in_sg, int in_num, size_t offset,
>> > + const void *src, size_t size)
>> >  {
>> >  return v9fs_packunpack((void *)src, in_sg, in_num, offset, size, 1);
>> >  }
>> > diff --git a/fsdev/virtio-9p-marshal.h b/fsdev/virtio-9p-marshal.h
>> > index 0709bcd..766a48e 100644
>> > --- a/fsdev/virtio-9p-marshal.h
>> > +++ b/fsdev/virtio-9p-marshal.h
>> > @@ -3,9 +3,6 @@
>> >
>> >  #include "9p-marshal.h"
>> >
>> > -
>> > -ssize_t v9fs_pack(struct iovec *in_sg, int in_num, size_t offset,
>> > -  const void *src, size_t size);
>> >  ssize_t v9fs_unmarshal(struct iovec *out_sg, int out_num, size_t offset,
>> > int bswap, const char *fmt, ...);
>> >  ssize_t v9fs_marshal(struct iovec *in_sg, int in_num, size_t offset,
>> > diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
>> > index 30ff828..654c103 100644
>> > --- a/hw/9pfs/virtio-9p.c
>> > +++ b/hw/9pfs/virtio-9p.c
>> > @@ -1561,6 +1561,7 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU 
>> > *pdu, V9fsFidState *fidp,
>> >  size_t offset = 7;
>> >  int read_count;
>> >  int64_t xattr_len;
>> > +V9fsBlob blob;
>> >
>> >  xattr_len = fidp->fs.xattr.len;
>> >  read_count = xattr_len - off;
>> > @@ -1572,14 +1573,18 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU 
>> > *pdu, V9fsFidState *fidp,
>> >   */
>> >  read_count = 0;
>> >  }
>> > -err = pdu_marshal(pdu, offset, "d", read_count);
>> > -if (err < 0) {
>> > -return err;
>> > -}
>> > -offset += err;
>> > -err = v9fs_pack(pdu->elem.in_sg, pdu->elem.in_num, offset,
>> > -((char *)fidp->fs.xattr.value) + off,
>> > -read_count);
>> > +
>> > +v9fs_blob_init();
>> > +
>> > +blob.data = g_malloc(read_count);
>> > +memcpy(blob.data, ((char *)fidp->fs.xattr.value) + off,
>> > +   read_count);
>> > +blob.size = read_count;
>> > +
>> > +err = pdu_marshal(pdu, offset, "dB", read_count, );
>> 
>> Is this correct ?
>> 
>> earlier we had read_count 
>> now we have
>> read_count,  which is read_count, blob->size, data
>> 
>
> Yes, you're right. There is an error.
>
> The new code should be
>
> err = pdu_marshal(pdu, offset, "B", );
>
> Thanks for your careful review.
>

We would then need 'B' to encode size as int ie, 'd' instead of 'w'

-aneesh

Re: [Qemu-devel] [PATCH v2 00/27] 9pfs: disentangling virtio and generic code

[Aneesh Kumar K.V]

Wei Liu  writes:

> Hi all
>
> Version 2 of this series is even longer. :-)
>
> Back in 2015 summer one of our OPW interns Linda Jacobson explored the
> possibility of making 9pfs work on Xen. It turned out lots of code in QEMU can
> be reused.
>
> This patch series can be found at:
>
>   git://xenbits.xen.org/people/liuw/qemu.git wip.9pfs-refactor-v2
>

I pushed most of the patches to

https://github.com/kvaneesh/qemu/commits/upstream-v9fs

Patches not yet applied are

fsdev: 9p-marshal: introduce V9fsBlob
9pfs: use V9fsBlob to transmit xattr
9pfs: disentangle V9fsState

Test result for pjd-fstest:

Test Summary Report
---
./tests/xacl/00.t(Wstat: 0 Tests: 45 Failed: 1)
  Failed test:  45
Files=191, Tests=2287, 109 wallclock secs ( 2.96 usr  1.36 sys + 13.96 cusr 
40.93 csys = 59.21 CPU)

I will continue to run more tests with different security model and proxy
config before pushing this upstream.

-aneesh

Re: [Qemu-devel] [PATCH v2 21/27] 9pfs: factor out virtio_9p_push_and_notify

[Aneesh Kumar K.V]

Wei Liu  writes:

> The new function resides in virtio specific file.
>
> Signed-off-by: Wei Liu 
> ---
> v2: new, part of original "9pfs: break out generic code from
> virtio-9p.{c,h}"
> ---
>  hw/9pfs/virtio-9p-device.c | 11 +++
>  hw/9pfs/virtio-9p.c|  8 +---
>  hw/9pfs/virtio-9p.h|  2 ++
>  3 files changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> index 5cad654..cfad13a 100644
> --- a/hw/9pfs/virtio-9p-device.c
> +++ b/hw/9pfs/virtio-9p-device.c
> @@ -20,6 +20,17 @@
>  #include "coth.h"
>  #include "hw/virtio/virtio-access.h"
>
> +void virtio_9p_push_and_notify(V9fsPDU *pdu)
> +{
> +V9fsState *s = pdu->s;
> +
> +/* push onto queue and notify */
> +virtqueue_push(s->vq, >elem, pdu->size);
> +
> +/* FIXME: we should batch these completions */
> +virtio_notify(VIRTIO_DEVICE(s), s->vq);
> +}
> +
>  static uint64_t virtio_9p_get_features(VirtIODevice *vdev, uint64_t features,
> Error **errp)
>  {
> diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> index 3c39247..0ba2312 100644
> --- a/hw/9pfs/virtio-9p.c
> +++ b/hw/9pfs/virtio-9p.c
> @@ -65,13 +65,7 @@ ssize_t pdu_unmarshal(V9fsPDU *pdu, size_t offset, const 
> char *fmt, ...)
>
>  static void pdu_push_and_notify(V9fsPDU *pdu)
>  {
> -V9fsState *s = pdu->s;
> -
> -/* push onto queue and notify */
> -virtqueue_push(s->vq, >elem, pdu->size);
> -
> -/* FIXME: we should batch these completions */
> -virtio_notify(VIRTIO_DEVICE(s), s->vq);
> +virtio_9p_push_and_notify(pdu);
>  }
>
>  static int omode_to_uflags(int8_t mode)
> diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
> index b4d344a..a1ac398 100644
> --- a/hw/9pfs/virtio-9p.h
> +++ b/hw/9pfs/virtio-9p.h
> @@ -6,6 +6,8 @@
>  #include "9p.h"
>
>  extern void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq);
> +extern void virtio_9p_push_and_notify(V9fsPDU *pdu);
> +
>  ssize_t virtio_pdu_vmarshal(V9fsPDU *pdu, size_t offset,
>  const char *fmt, va_list ap);
>  ssize_t virtio_pdu_vunmarshal(V9fsPDU *pdu, size_t offset,
> -- 
> 2.1.4

How is this different from pdu_push_notify added by  [PATCH 18/27]

-aneesh

Re: [Qemu-devel] [PATCH 00/22] 9pfs: disentangling virtio and generic code

[Aneesh Kumar K.V]

Wei Liu  writes:

> Hi all
>
> Back in 2015 summer one of our OPW interns Linda Jacobson explored the
> possibility of making 9pfs work on Xen. It turned out lots of code in QEMU can
> be reused.
>
> This series refactors 9pfs related code:
>
> 1. Rename a bunch of files and functions to make clear they are generic.
> 2. Only export two functions (marshal and unmarshal) from transport to generic
>code.
> 3. disentangle virtio transport code and generic 9pfs code.
> 4. Some function name clean-up.
>
> To make sure this series doesn't break compilation a rune is use to compile
> every commit.
>
> $ git rebase -i origin/master --exec "make -j16 clean; ./configure 
> --target-list=x86_64-softmmu --enable-virtfs --enable-kvm; make -j16 && echo 
> ok."
>
> Three use cases are tested:
>
> 1. Local file system driver with passthrough security policy
> ./qemu-system-x86_64 -L .  -hda /dev/DATA/jessie   -vnc 0.0.0.0:0,to=99  
> -fsdev local,path=/root/qemu,security_model=passthrough,id=fs1  -device 
> virtio-9p-pci,fsdev=fs1,mount_tag=qemu  &
>
> 2. Local file system driver with mapped security policy
> ./qemu-system-x86_64 -L . -hda /dev/DATA/jessie -vnc 0.0.0.0:0,to=99 -fsdev 
> local,path=/root/qemu,security_model=mapped,id=fs1 -device 
> virtio-9p-pci,fsdev=fs1,mount_tag=qemu &
>
> 3. Proxy file system driver
> ./virtfs-proxy-helper -p /root/qemu -n -s virtfs-helper-sock -u 0 -g 0 &
> ./qemu-system-x86_64 -L .  -hda /dev/DATA/jessie   -vnc 0.0.0.0:0,to=99  
> -fsdev proxy,socket=virtfs-helper-sock,id=fs1  -device 
> virtio-9p-pci,fsdev=fs1,mount_tag=qemu &
>
> During each of the tests, mounting, unmounting, read and write operations are
> performed. In "mapped" test, getfattr in host was used to inspect the
> attributes.
>
> Let me know if you would like to see more tests.


you can also try this http://www.tuxera.com/community/posix-test-suite/
with the xen transport. Are you looking at getting this merged before
the xen transport is done or are we expecting further changes to this
as you make progress with Xen transport ?

W.r.t posix test suite with upstream we have one expected failure in the acl
test. You can ignore that. I will also try to get some test run going here.


>
> Xen transport is still under development. I figure it would be better to post
> this series as soon as possible because rebasing such huge series from time to
> time is prone to error.
>
> Comments are welcome. Thanks!

Good series with some nice cleanups. 

-aneesh

Re: [Qemu-devel] [PATCH 20/22] 9pfs: break out generic code from virtio-9p.{c, h}

[Aneesh Kumar K.V]

Wei Liu <wei.l...@citrix.com> writes:

> On Thu, Jan 07, 2016 at 10:27:13PM +0530, Aneesh Kumar K.V wrote:
>> Wei Liu <wei.l...@citrix.com> writes:
>> 
>> > The vast majority of code in virtio-9p.c is actually generic code.
>> > Rename that file to 9p.c and move virtio specific code to
>> > virtio-9p-device.c. Rename virtio-9p.h to 9p.h and split out virtio
>> > specific code to new virtio-9p.h.
>> >
>> > Finally fix up virtio-pci.h header file inclusion.
>> >
>> > Note that V9fsState and V9fsPDU are still tied to virtio at the moment.
>> > They will be handled later.
>> 
>> Can you split this to smaller patches as done for others.
>> 
>
> I'm not sure what you're asking for. IMHO splitting out C and header
> file is better done in one single commit.
>

virtio-9p.h -> 9p.h one patch
virtio_p9_push_and_notify . There is one other instance of that
conversion in the next patch
handle_p9_output movement
V9fsThPool, what is that ?

-aneesh

Re: [Qemu-devel] [PATCH 09/22] 9pfs: remove dead code

[Aneesh Kumar K.V]

Wei Liu  writes:

> Some structures virtio-9p.h have been unused since 2011 when relevant
> functions switched to use coroutines.
>
> The declaration of pdu_packunpack and function do_pdu_unpack are
> useless.
>
> The function virtio_9p_set_fd_limit is unused.
>
> Signed-off-by: Wei Liu 
> ---
>  hw/9pfs/virtio-9p.c | 11 -
>  hw/9pfs/virtio-9p.h | 68 
> -
>  2 files changed, 79 deletions(-)
>
> diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> index 30ff828..084fa6a 100644
> --- a/hw/9pfs/virtio-9p.c
> +++ b/hw/9pfs/virtio-9p.c
> @@ -3287,14 +3287,3 @@ void handle_9p_output(VirtIODevice *vdev, VirtQueue 
> *vq)
>  }
>  free_pdu(s, pdu);
>  }
> -
> -static void __attribute__((__constructor__)) virtio_9p_set_fd_limit(void)
> -{
> -struct rlimit rlim;
> -if (getrlimit(RLIMIT_NOFILE, ) < 0) {
> -fprintf(stderr, "Failed to get the resource limit\n");
> -exit(1);
> -}
> -open_fd_hw = rlim.rlim_cur - MIN(400, rlim.rlim_cur/3);
> -open_fd_rc = rlim.rlim_cur/2;
> -}

I am looking at when we stopped using that. We still do file descriptor
reclaim. If we do that we need to set the open_fd_hw/rc.



> diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
> index ac4cb00..3c78d3c 100644
> --- a/hw/9pfs/virtio-9p.h
> +++ b/hw/9pfs/virtio-9p.h
> @@ -227,65 +227,6 @@ typedef struct V9fsState
>  V9fsConf fsconf;
>2.1.4

-aneesh

Re: [Qemu-devel] [PATCH v2 27/27] 9pfs: disentangle V9fsState

[Aneesh Kumar K.V]

Wei Liu  writes:

> V9fsState now only contains generic fields. Introduce V9fsVirtioState
> for virtio transport.  Change virtio-pci and virtio-ccw to use
> V9fsVirtioState. Handle transport enumeration in generic routines.
>

Few comments below


> Signed-off-by: Wei Liu 
> ---
>  hw/9pfs/9p.c   | 41 ++-
>  hw/9pfs/9p.h   | 19 +++
>  hw/9pfs/virtio-9p-device.c | 82 
> --
>  hw/9pfs/virtio-9p.h| 12 ++-
>  hw/s390x/virtio-ccw.h  |  2 +-
>  hw/virtio/virtio-pci.h |  2 +-
>  6 files changed, 109 insertions(+), 49 deletions(-)
>
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 6858b21..2cf8580 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -45,7 +45,13 @@ ssize_t pdu_marshal(V9fsPDU *pdu, size_t offset, const 
> char *fmt, ...)
>  va_list ap;
>
>  va_start(ap, fmt);
> -ret = virtio_pdu_vmarshal(pdu, offset, fmt, ap);
> +switch (pdu->transport) {
> +case VIRTIO:
> +ret = virtio_pdu_vmarshal(pdu, offset, fmt, ap);
> +break;
> +default:
> +ret = -1;
> +}
>  va_end(ap);
>


All that switch(pdu->transport) can go in the next series along with Xen
support. It is not really needed now and when we complete Xen transport
we will pull that. 

>  return ret;
> @@ -57,7 +63,13 @@ ssize_t pdu_unmarshal(V9fsPDU *pdu, size_t offset, const 
> char *fmt, ...)
>  va_list ap;
>
>  va_start(ap, fmt);
> -ret = virtio_pdu_vunmarshal(pdu, offset, fmt, ap);
> +switch (pdu->transport) {
> +case VIRTIO:
> +ret = virtio_pdu_vunmarshal(pdu, offset, fmt, ap);
> +break;
> +default:
> +ret = -1;
> +}
>  va_end(ap);
>
>  return ret;
> @@ -65,7 +77,11 @@ ssize_t pdu_unmarshal(V9fsPDU *pdu, size_t offset, const 
> char *fmt, ...)
>
>  static void pdu_push_and_notify(V9fsPDU *pdu)
>  {
> -virtio_9p_push_and_notify(pdu);
> +switch (pdu->transport) {
> +case VIRTIO:
> +virtio_9p_push_and_notify(pdu);
> +break;
> +}
>  }
>
>  static int omode_to_uflags(int8_t mode)
> @@ -1696,7 +1712,11 @@ static void v9fs_init_qiov_from_pdu(QEMUIOVector 
> *qiov, V9fsPDU *pdu,
>  struct iovec *iov;
>  unsigned int niov;
>
> -virtio_init_iov_from_pdu(pdu, , , is_write);
> +switch (pdu->transport) {
> +case VIRTIO:
> +virtio_init_iov_from_pdu(pdu, , , is_write);
> +break;
> +}
>
>  qemu_iovec_init_external(, iov, niov);
>  qemu_iovec_init(qiov, niov);
> @@ -3272,8 +3292,10 @@ void pdu_submit(V9fsPDU *pdu)
>  }
>
>  /* Returns 0 on success, 1 on failure. */
> -int v9fs_device_realize_common(V9fsState *s, Error **errp)
> +int v9fs_device_realize_common(V9fsState *s, enum p9_transport transport,
> +   Error **errp)
>  {
> +V9fsVirtioState *v = container_of(s, V9fsVirtioState, state);
>  int i, len;
>  struct stat stat;
>  FsDriverEntry *fse;
> @@ -3284,8 +3306,10 @@ int v9fs_device_realize_common(V9fsState *s, Error 
> **errp)
>  QLIST_INIT(>free_list);
>  QLIST_INIT(>active_list);
>  for (i = 0; i < (MAX_REQ - 1); i++) {
> -QLIST_INSERT_HEAD(>free_list, >pdus[i], next);
> -s->pdus[i].s = s;
> +QLIST_INSERT_HEAD(>free_list, >pdus[i], next);
> +v->pdus[i].s = s;
> +v->pdus[i].idx = i;
> +v->pdus[i].transport = transport;
>  }
>
>  v9fs_path_init();
> @@ -3360,7 +3384,8 @@ out:
>  return rc;
>  }
>
> -void v9fs_device_unrealize_common(V9fsState *s, Error **errp)
> +void v9fs_device_unrealize_common(V9fsState *s, enum p9_transport transport,
> +  Error **errp)
>  {
>  g_free(s->ctx.fs_root);
>  g_free(s->tag);
> diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> index 3fe4da4..bd8588d 100644
> --- a/hw/9pfs/9p.h
> +++ b/hw/9pfs/9p.h
> @@ -14,6 +14,10 @@
>  #include "qemu/thread.h"
>  #include "qemu/coroutine.h"
>
> +enum p9_transport {
> +VIRTIO = 0x1,
> +};
> +
>  enum {
>  P9_TLERROR = 6,
>  P9_RLERROR,
> @@ -131,9 +135,10 @@ struct V9fsPDU
>  uint8_t id;
>  uint8_t cancelled;
>  CoQueue complete;
> -VirtQueueElement elem;
>  struct V9fsState *s;
>  QLIST_ENTRY(V9fsPDU) next;
> +uint32_t idx; /* index inside the array */
> +enum p9_transport transport;
>  };
>


Can you do this change as a separate patch ? ie, Make V9fsPDU
independent of virtio . Also introduce V9fsVirtioState 


>
> @@ -205,16 +210,12 @@ struct V9fsFidState
>

-aneesh

Re: [Qemu-devel] [PATCH] MAINTAINERS: add maintainer to virtio-9p

[Aneesh Kumar K.V]

Greg Kurz <gk...@linux.vnet.ibm.com> writes:

> As suggested by Paolo, I add myself as maintainer for virtio-9p.
>
> Signed-off-by: Greg Kurz <gk...@linux.vnet.ibm.com>

Acked-by: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com> 

> ---
>  MAINTAINERS |1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index bb1f3e40622b..e8cee1e2668f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -863,6 +863,7 @@ F: net/vhost-user.c
>  
>  virtio-9p
>  M: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
> +M: Greg Kurz <gk...@linux.vnet.ibm.com>
>  S: Supported
>  F: hw/9pfs/
>  F: fsdev/

Re: [Qemu-devel] [PATCH] mmap-alloc: use same backend for all mappings

[Aneesh Kumar K.V]

"Michael S. Tsirkin"  writes:

> On Mon, Nov 30, 2015 at 02:46:31PM +0100, Greg Kurz wrote:
>> On Mon, 30 Nov 2015 15:06:33 +0200
>> "Michael S. Tsirkin"  wrote:
>> 



>> 
>> On ppc64, the address space is divided in 256MB-sized segments where all 
>> pages
>> have the same size. This is a hw limitation IIUC. I don't know if it can be
>> fixed and I'll let Ben comment on it.
>
> But it's anonymous memory with PROT_NONE.  There should be no pages there:
> just a chunk of virtual memory reserved.
>

ppc64 use page size (called as base page size) to find the hash slot in
which we find the virtual address to real address translation. All the
pages in a segment should have same base page size. Hugetlb pages have a
base page size of 16M whereas a regular linux page have 64K. mmap will
fail to map a hugetlb mapping in a segment that already have regular
pages mapped.

-aneesh

Re: [Qemu-devel] [PATCH] mmap-alloc: use same backend for all mappings

[Aneesh Kumar K.V]

"Michael S. Tsirkin" <m...@redhat.com> writes:

> On Tue, Dec 01, 2015 at 04:23:11PM +0530, Aneesh Kumar K.V wrote:
>> "Michael S. Tsirkin" <m...@redhat.com> writes:
>> 
>> > On Mon, Nov 30, 2015 at 02:46:31PM +0100, Greg Kurz wrote:
>> >> On Mon, 30 Nov 2015 15:06:33 +0200
>> >> "Michael S. Tsirkin" <m...@redhat.com> wrote:
>> >> 
>> 
>> 
>> 
>> >> 
>> >> On ppc64, the address space is divided in 256MB-sized segments where all 
>> >> pages
>> >> have the same size. This is a hw limitation IIUC. I don't know if it can 
>> >> be
>> >> fixed and I'll let Ben comment on it.
>> >
>> > But it's anonymous memory with PROT_NONE.  There should be no pages there:
>> > just a chunk of virtual memory reserved.
>> >
>> 
>> ppc64 use page size (called as base page size) to find the hash slot in
>> which we find the virtual address to real address translation. All the
>> pages in a segment should have same base page size. Hugetlb pages have a
>> base page size of 16M whereas a regular linux page have 64K. mmap will
>> fail to map a hugetlb mapping in a segment that already have regular
>> pages mapped.
>> 
>> -aneesh
>
>
> I see this in kernel:
>
>} else if (flags & MAP_HUGETLB) {
> struct user_struct *user = NULL;
> struct hstate *hs;
>
> hs = hstate_sizelog((flags >> MAP_HUGE_SHIFT) & 
> SHM_HUGE_MASK);
> if (!hs)
> return -EINVAL;
>
> len = ALIGN(len, huge_page_size(hs));
> /*
>  * VM_NORESERVE is used because the reservations will be
>  * taken when vm_ops->mmap() is called
>  * A dummy user value is used because we are not locking
>  * memory so no accounting is necessary
>  */
> file = hugetlb_file_setup(HUGETLB_ANON_FILE, len,
> VM_NORESERVE,
> , HUGETLB_ANONHUGE_INODE,
> (flags >> MAP_HUGE_SHIFT) & MAP_HUGE_MASK);
> if (IS_ERR(file))
> return PTR_ERR(file);
> }
>
> So maybe it's a question of passing in MAP_HUGETLB and the
> correct size mask.
>

Can you explain this more ?

If the question is do we need to pass fd and remove MAP_ANONYMOUS to map
hugetlb, we don't. A good example is
tools/testing/selftest/vm/map_hugetlb.c

If the question is whether we will loose hugepages on mmap even if the
mapping is PROT_NONE, then the answer is we do in the form of hugetlb
reservation.

-aneesh

Re: [Qemu-devel] [PATCH 2/3] virtio-9p: add unrealize handler

[Aneesh Kumar K.V]

Stefan Hajnoczi  writes:

> On Mon, Oct 05, 2015 at 11:07:23AM +0200, Greg Kurz wrote:
>> If the user tries to hot unplug a virtio-9p device, it seems to succeed but
>> in fact:
>> - virtio-9p coroutines thread pool and async queue are leaked
>> - QEMU crashes in virtio_vmstate_change() if the user tries to live migrate
>> 
>> This patch brings hot unplug support to virtio-9p-device. It fixes both
>> above issues.
>> 
>> Signed-off-by: Greg Kurz 
>> ---
>>  hw/9pfs/virtio-9p-device.c |   12 
>>  1 file changed, 12 insertions(+)
>
> What happens to in-flight I/O requests?  We cannot assume that the guest
> driver quiesces the device.

We enable migration blocker when we have an active mount. So if we get
here, that should indicate no active 9p mounts.

-aneesh

Re: [Qemu-devel] [PATCH] virtio-9p: migrate virtio subsections

[Aneesh Kumar K.V]

Greg Kurz  writes:

> In a cross-endian setup, the virtio-9p device has state in @device_endian. It
> must be migrated.
>
> Signed-off-by: Greg Kurz 

With 9p mounted, we don't support qemu migration. We still have
migration blocker added in v9fs_attach.


> ---
>  hw/9pfs/virtio-9p-device.c |   11 +++
>  1 file changed, 11 insertions(+)
>
> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> index 93a407c45926..e3abcfaffb2a 100644
> --- a/hw/9pfs/virtio-9p-device.c
> +++ b/hw/9pfs/virtio-9p-device.c
> @@ -43,6 +43,16 @@ static void virtio_9p_get_config(VirtIODevice *vdev, 
> uint8_t *config)
>  g_free(cfg);
>  }
>
> +static void virtio_9p_save(QEMUFile *f, void *opaque)
> +{
> +virtio_save(VIRTIO_DEVICE(opaque), f);
> +}
> +
> +static int virtio_9p_load(QEMUFile *f, void *opaque, int version_id)
> +{
> +return virtio_load(VIRTIO_DEVICE(opaque), f, version_id);
> +}
> +
>  static void virtio_9p_device_realize(DeviceState *dev, Error **errp)
>  {
>  VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> @@ -130,6 +140,7 @@ static void virtio_9p_device_realize(DeviceState *dev, 
> Error **errp)
>  }
>  v9fs_path_free();
>
> +register_savevm(dev, "virtio-9p", -1, 1, virtio_9p_save, virtio_9p_load, 
> s);
>  return;
>  out:
>  g_free(s->ctx.fs_root);

Re: [Qemu-devel] [PATCH] virtio-9p: migrate virtio subsections

[Aneesh Kumar K.V]

Greg Kurz  writes:

> In a cross-endian setup, the virtio-9p device has state in @device_endian. It
> must be migrated.
>
> Signed-off-by: Greg Kurz 

With 9p mounted, we don't support qemu migration. We have
migration blocker added in v9fs_attach. 

> ---
>  hw/9pfs/virtio-9p-device.c |   11 +++
>  1 file changed, 11 insertions(+)
>
> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> index 93a407c45926..e3abcfaffb2a 100644
> --- a/hw/9pfs/virtio-9p-device.c
> +++ b/hw/9pfs/virtio-9p-device.c
> @@ -43,6 +43,16 @@ static void virtio_9p_get_config(VirtIODevice *vdev, 
> uint8_t *config)
>  g_free(cfg);
>  }
>
> +static void virtio_9p_save(QEMUFile *f, void *opaque)
> +{
> +virtio_save(VIRTIO_DEVICE(opaque), f);
> +}
> +
> +static int virtio_9p_load(QEMUFile *f, void *opaque, int version_id)
> +{
> +return virtio_load(VIRTIO_DEVICE(opaque), f, version_id);
> +}
> +
>  static void virtio_9p_device_realize(DeviceState *dev, Error **errp)
>  {
>  VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> @@ -130,6 +140,7 @@ static void virtio_9p_device_realize(DeviceState *dev, 
> Error **errp)
>  }
>  v9fs_path_free();
>
> +register_savevm(dev, "virtio-9p", -1, 1, virtio_9p_save, virtio_9p_load, 
> s);
>  return;
>  out:
>  g_free(s->ctx.fs_root);

[Qemu-devel] [PATCH 1/2] virtfs-proxy-helper: add missing long option terminator

[Aneesh Kumar K.V]

From: Stefan Hajnoczi stefa...@redhat.com

The getopt_long(3) long options array must have a zeroed terminator.

This patch solves a segmentation fault when an unknown command-line
option is encountered:

  $ fsdev/virtfs-proxy-helper --help
  Segmentation fault (core dumped)

Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
Signed-off-by: Aneesh Kumar K.V aneesh.ku...@linux.vnet.ibm.com
---
 fsdev/virtfs-proxy-helper.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index a698e2dbb37b..91e8b9b7f1cf 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -49,6 +49,7 @@ static struct option helper_opts[] = {
 {socket, required_argument, NULL, 's'},
 {uid, required_argument, NULL, 'u'},
 {gid, required_argument, NULL, 'g'},
+{},
 };
 
 static bool is_daemon;
-- 
2.1.4

[Qemu-devel] [PATCH 2/2] virtfs-proxy-helper: fail gracefully if socket path is too long

[Aneesh Kumar K.V]

From: Stefan Hajnoczi stefa...@redhat.com

Replace the assertion check with graceful failure when the socket path
is too long.  Programs should not crash on invalid input.  Print an
error message and exit properly.

Cc: Shannon Zhao zhaoshengl...@huawei.com
Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
Signed-off-by: Aneesh Kumar K.V aneesh.ku...@linux.vnet.ibm.com
---
 fsdev/virtfs-proxy-helper.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index 91e8b9b7f1cf..9097d15c989c 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -739,7 +739,12 @@ static int proxy_socket(const char *path, uid_t uid, gid_t 
gid)
 return -1;
 }
 
-g_assert(strlen(path)  sizeof(proxy.sun_path));
+if (strlen(path) = sizeof(proxy.sun_path)) {
+do_log(LOG_CRIT, UNIX domain socket path exceeds %zu characters\n,
+   sizeof(proxy.sun_path));
+return -1;
+}
+
 sock = socket(AF_UNIX, SOCK_STREAM, 0);
 if (sock  0) {
 do_perror(socket);
-- 
2.1.4

[Qemu-devel] [PULL] VirtFS update

[Aneesh Kumar K.V]

The following changes since commit 93f6d1c16036aaf34055d16f54ea770fb8d6d280:

  Merge remote-tracking branch 'remotes/kraxel/tags/pull-vga-20150615-1' into 
staging (2015-06-16 10:35:43 +0100)

are available in the git repository at:

  https://github.com/kvaneesh/qemu.git tags/for-upstream-signed

for you to fetch changes up to f8d30a4f96d6c3a12e692d2e69b8fe4734b916c6:

  virtfs-proxy-helper: fail gracefully if socket path is too long (2015-06-16 
20:32:29 +0530)


VirtFS update:

* Fix for virtfs-proxy-helper crash
* Gracefully handle the error condition on input validation in 
virtfs-proxy-helper


Stefan Hajnoczi (2):
  virtfs-proxy-helper: add missing long option terminator
  virtfs-proxy-helper: fail gracefully if socket path is too long

 fsdev/virtfs-proxy-helper.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

-- 
2.1.4

Re: [Qemu-devel] [PATCH 0/2] virtfs-proxy-helper: small fixes

[Aneesh Kumar K.V]

Aneesh Kumar K.V aneesh.ku...@linux.vnet.ibm.com writes:

 Stefan Hajnoczi stefa...@gmail.com writes:

 On Mon, Mar 30, 2015 at 02:57:14PM +0100, Stefan Hajnoczi wrote:
 These fixes are not critical but it wouldn't hurt to get them into QEMU 2.3.
 

I am not sure whether this is critical enough to go to Qemu 2.3 ? If so
I can send a pull request. I have already pushed the changes after
testing to


 https://github.com/kvaneesh/qemu.git tags/for-upstream-signed


-aneesh

Re: [Qemu-devel] [PATCH 0/2] virtfs-proxy-helper: small fixes

[Aneesh Kumar K.V]

Stefan Hajnoczi stefa...@gmail.com writes:

 On Mon, Mar 30, 2015 at 02:57:14PM +0100, Stefan Hajnoczi wrote:
 These fixes are not critical but it wouldn't hurt to get them into QEMU 2.3.
 
 See patches for descriptions.
 
 Stefan Hajnoczi (2):
   virtfs-proxy-helper: add missing long option terminator
   virtfs-proxy-helper: fail gracefully if socket path is too long
 
  fsdev/virtfs-proxy-helper.c | 8 +++-
  1 file changed, 7 insertions(+), 1 deletion(-)

 Ping?

Applied.

-aneesh

Re: [Qemu-devel] [PATCH v2] fsdev/virtfs-proxy-helper: Fix possible overflow

[Aneesh Kumar K.V]

Stefan Hajnoczi stefa...@gmail.com writes:

 On Sat, Mar 14, 2015 at 10:00:16AM +0800, Shannon Zhao wrote:
 It's detected by coverity. As max of sockaddr_un.sun_path is
 sizeof(helper.sun_path), should check the length of source
 and use strncpy instead of strcpy.
 
 Signed-off-by: Shannon Zhao zhaoshengl...@huawei.com
 Signed-off-by: Shannon Zhao shannon.z...@linaro.org
 ---
 v1-v2: Still use strcpy [Paolo]
 ---
  fsdev/virtfs-proxy-helper.c | 1 +
  1 file changed, 1 insertion(+)
 
 diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
 index bf2e5f3..13fe032 100644
 --- a/fsdev/virtfs-proxy-helper.c
 +++ b/fsdev/virtfs-proxy-helper.c
 @@ -738,6 +738,7 @@ static int proxy_socket(const char *path, uid_t uid, 
 gid_t gid)
  return -1;
  }
  
 +g_assert(strlen(path)  sizeof(proxy.sun_path));
  sock = socket(AF_UNIX, SOCK_STREAM, 0);

 path is user input.  While the assertion check silences Coverity, it is
 not suitable for input validation.  Users expect a graceful exit with an
 error message, not an assertion failure if the given path is too long.

 I will send a patch.

That is the proxy helper. The assert will cause an exit() which is good,
isn't it ? I did update the qemu side of the patch to do a graceful exit

-aneesh

[Qemu-devel] [PATCH 3/6] hw/9pfs/virtio-9p-posix-acl: Fix out-of-bounds access

[Aneesh Kumar K.V]

From: Shannon Zhao zhaoshengl...@huawei.com

It's detected by coverity. Fix out-of-bounds access of the function 
mp_dacl_listxattr.

Signed-off-by: Shannon Zhao zhaoshengl...@huawei.com
Signed-off-by: Shannon Zhao shannon.z...@linaro.org
Signed-off-by: Aneesh Kumar K.V aneesh.ku...@linux.vnet.ibm.com
---
 hw/9pfs/virtio-9p-posix-acl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/9pfs/virtio-9p-posix-acl.c b/hw/9pfs/virtio-9p-posix-acl.c
index 803d9d94f3b8..09dad071e487 100644
--- a/hw/9pfs/virtio-9p-posix-acl.c
+++ b/hw/9pfs/virtio-9p-posix-acl.c
@@ -114,7 +114,7 @@ static ssize_t mp_dacl_listxattr(FsContext *ctx, const char 
*path,
 }
 
 /* len includes the trailing NUL */
-memcpy(value, ACL_ACCESS, len);
+memcpy(value, ACL_DEFAULT, len);
 return 0;
 }
 
-- 
2.1.0

[Qemu-devel] [PATCH 5/6] virtfs-proxy: Fix possible overflow

[Aneesh Kumar K.V]

From: Shannon Zhao zhaoshengl...@huawei.com

It's detected by coverity. The socket name specified
should fit in the sockadd_un.sun_path. If not abort.

Signed-off-by: Shannon Zhao zhaoshengl...@huawei.com
Signed-off-by: Shannon Zhao shannon.z...@linaro.org
Signed-off-by: Aneesh Kumar K.V aneesh.ku...@linux.vnet.ibm.com
---
 fsdev/virtfs-proxy-helper.c | 1 +
 hw/9pfs/virtio-9p-proxy.c   | 4 
 2 files changed, 5 insertions(+)

diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index bf2e5f333121..13fe032543bc 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -738,6 +738,7 @@ static int proxy_socket(const char *path, uid_t uid, gid_t 
gid)
 return -1;
 }
 
+g_assert(strlen(path)  sizeof(proxy.sun_path));
 sock = socket(AF_UNIX, SOCK_STREAM, 0);
 if (sock  0) {
 do_perror(socket);
diff --git a/hw/9pfs/virtio-9p-proxy.c b/hw/9pfs/virtio-9p-proxy.c
index 6bb191ee6ab8..71b6198bbd22 100644
--- a/hw/9pfs/virtio-9p-proxy.c
+++ b/hw/9pfs/virtio-9p-proxy.c
@@ -1100,6 +1100,10 @@ static int connect_namedsocket(const char *path)
 int sockfd, size;
 struct sockaddr_un helper;
 
+if (strlen(path) = sizeof(helper.sun_path)) {
+fprintf(stderr, Socket name too large\n);
+return -1;
+}
 sockfd = socket(AF_UNIX, SOCK_STREAM, 0);
 if (sockfd  0) {
 fprintf(stderr, failed to create socket: %s\n, strerror(errno));
-- 
2.1.0

[Qemu-devel] [PATCH 6/6] virtio: Fix memory leaks reported by Coverity

[Aneesh Kumar K.V]

From: Stefan Weil s...@weilnetz.de

All four leaks are similar, so fix them in one patch.
Success path was not doing memory free.

Signed-off-by: Stefan Weil s...@weilnetz.de
Signed-off-by: Aneesh Kumar K.V aneesh.ku...@linux.vnet.ibm.com
---
 hw/9pfs/virtio-9p-local.c | 28 
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
index 84efb31cfec4..d6b1c0cddef9 100644
--- a/hw/9pfs/virtio-9p-local.c
+++ b/hw/9pfs/virtio-9p-local.c
@@ -486,7 +486,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath 
*dir_path,
 int err = -1;
 int serrno = 0;
 V9fsString fullname;
-char *buffer;
+char *buffer = NULL;
 
 v9fs_string_init(fullname);
 v9fs_string_sprintf(fullname, %s/%s, dir_path-data, name);
@@ -497,7 +497,6 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath 
*dir_path,
 buffer = rpath(fs_ctx, path);
 err = mknod(buffer, SM_LOCAL_MODE_BITS|S_IFREG, 0);
 if (err == -1) {
-g_free(buffer);
 goto out;
 }
 err = local_set_xattr(buffer, credp);
@@ -510,7 +509,6 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath 
*dir_path,
 buffer = rpath(fs_ctx, path);
 err = mknod(buffer, SM_LOCAL_MODE_BITS|S_IFREG, 0);
 if (err == -1) {
-g_free(buffer);
 goto out;
 }
 err = local_set_mapped_file_attr(fs_ctx, path, credp);
@@ -523,7 +521,6 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath 
*dir_path,
 buffer = rpath(fs_ctx, path);
 err = mknod(buffer, credp-fc_mode, credp-fc_rdev);
 if (err == -1) {
-g_free(buffer);
 goto out;
 }
 err = local_post_create_passthrough(fs_ctx, path, credp);
@@ -537,8 +534,8 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath 
*dir_path,
 err_end:
 remove(buffer);
 errno = serrno;
-g_free(buffer);
 out:
+g_free(buffer);
 v9fs_string_free(fullname);
 return err;
 }
@@ -550,7 +547,7 @@ static int local_mkdir(FsContext *fs_ctx, V9fsPath 
*dir_path,
 int err = -1;
 int serrno = 0;
 V9fsString fullname;
-char *buffer;
+char *buffer = NULL;
 
 v9fs_string_init(fullname);
 v9fs_string_sprintf(fullname, %s/%s, dir_path-data, name);
@@ -561,7 +558,6 @@ static int local_mkdir(FsContext *fs_ctx, V9fsPath 
*dir_path,
 buffer = rpath(fs_ctx, path);
 err = mkdir(buffer, SM_LOCAL_DIR_MODE_BITS);
 if (err == -1) {
-g_free(buffer);
 goto out;
 }
 credp-fc_mode = credp-fc_mode|S_IFDIR;
@@ -574,7 +570,6 @@ static int local_mkdir(FsContext *fs_ctx, V9fsPath 
*dir_path,
 buffer = rpath(fs_ctx, path);
 err = mkdir(buffer, SM_LOCAL_DIR_MODE_BITS);
 if (err == -1) {
-g_free(buffer);
 goto out;
 }
 credp-fc_mode = credp-fc_mode|S_IFDIR;
@@ -588,7 +583,6 @@ static int local_mkdir(FsContext *fs_ctx, V9fsPath 
*dir_path,
 buffer = rpath(fs_ctx, path);
 err = mkdir(buffer, credp-fc_mode);
 if (err == -1) {
-g_free(buffer);
 goto out;
 }
 err = local_post_create_passthrough(fs_ctx, path, credp);
@@ -602,8 +596,8 @@ static int local_mkdir(FsContext *fs_ctx, V9fsPath 
*dir_path,
 err_end:
 remove(buffer);
 errno = serrno;
-g_free(buffer);
 out:
+g_free(buffer);
 v9fs_string_free(fullname);
 return err;
 }
@@ -657,7 +651,7 @@ static int local_open2(FsContext *fs_ctx, V9fsPath 
*dir_path, const char *name,
 int err = -1;
 int serrno = 0;
 V9fsString fullname;
-char *buffer;
+char *buffer = NULL;
 
 /*
  * Mark all the open to not follow symlinks
@@ -673,7 +667,6 @@ static int local_open2(FsContext *fs_ctx, V9fsPath 
*dir_path, const char *name,
 buffer = rpath(fs_ctx, path);
 fd = open(buffer, flags, SM_LOCAL_MODE_BITS);
 if (fd == -1) {
-g_free(buffer);
 err = fd;
 goto out;
 }
@@ -688,7 +681,6 @@ static int local_open2(FsContext *fs_ctx, V9fsPath 
*dir_path, const char *name,
 buffer = rpath(fs_ctx, path);
 fd = open(buffer, flags, SM_LOCAL_MODE_BITS);
 if (fd == -1) {
-g_free(buffer);
 err = fd;
 goto out;
 }
@@ -704,7 +696,6 @@ static int local_open2(FsContext *fs_ctx, V9fsPath 
*dir_path, const char *name,
 buffer = rpath(fs_ctx, path);
 fd = open(buffer, flags, credp-fc_mode);
 if (fd == -1) {
-g_free(buffer);
 err = fd;
 goto out;
 }
@@ -722,8 +713,8 @@ err_end:
 close(fd);
 remove(buffer);
 errno = serrno;
-g_free(buffer);
 out:
+g_free(buffer);
 v9fs_string_free(fullname);
 return err;
 }
@@ -736,7 +727,7 @@ static int local_symlink(FsContext *fs_ctx, const char 
*oldpath,
 int

[Qemu-devel] [PULL] VirtFS update

[Aneesh Kumar K.V]

Hi,

Please pull the below update for VirtFS

The following changes since commit ee74801035b0b5f1fdfd4e31d3a53f511f91c804:

  Merge remote-tracking branch 'remotes/lalrae/tags/mips-20150311' into staging 
(2015-03-11 18:22:15 +)

are available in the git repository at:

  https://github.com/kvaneesh/qemu.git for-upstream

for you to fetch changes up to 4ed7b2c3a78f785a1bcbe575e08c379b166723e3:

  virtio: Fix memory leaks reported by Coverity (2015-03-16 13:32:24 +0530)


Michael Tokarev (2):
  9pfs-local: simplify/optimize local_mapped_attr_path()
  9pfs-proxy: tiny cleanups in proxy_pwritev and proxy_preadv

Shannon Zhao (3):
  hw/9pfs/virtio-9p-posix-acl: Fix out-of-bounds access
  fsdev/virtfs-proxy-helper: Fix improper use of negative value
  virtfs-proxy: Fix possible overflow

Stefan Weil (1):
  virtio: Fix memory leaks reported by Coverity

 fsdev/virtfs-proxy-helper.c   |  4 
 hw/9pfs/virtio-9p-local.c | 52 ---
 hw/9pfs/virtio-9p-posix-acl.c |  2 +-
 hw/9pfs/virtio-9p-proxy.c | 22 +-
 4 files changed, 36 insertions(+), 44 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 4/6] fsdev/virtfs-proxy-helper: Fix improper use of negative value

[Aneesh Kumar K.V]

From: Shannon Zhao zhaoshengl...@huawei.com

It's detected by coverity. Check the return value of proxy_marshal.

Signed-off-by: Shannon Zhao zhaoshengl...@huawei.com
Signed-off-by: Shannon Zhao shannon.z...@linaro.org
Signed-off-by: Aneesh Kumar K.V aneesh.ku...@linux.vnet.ibm.com
---
 fsdev/virtfs-proxy-helper.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index c1da2d78e78b..bf2e5f333121 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -262,6 +262,9 @@ static int send_status(int sockfd, struct iovec *iovec, int 
status)
  */
 msg_size = proxy_marshal(iovec, 0, ddd, header.type,
  header.size, status);
+if (msg_size  0) {
+return msg_size;
+}
 retval = socket_write(sockfd, iovec-iov_base, msg_size);
 if (retval  0) {
 return retval;
-- 
2.1.0

Re: [Qemu-devel] [PATCH] fsdev/virtfs-proxy-helper: Fix improper use of negative value

[Aneesh Kumar K.V]

Shannon Zhao zhaoshengl...@huawei.com writes:

 It's detected by coverity. Check the return value of proxy_marshal.

 Signed-off-by: Shannon Zhao zhaoshengl...@huawei.com
 Signed-off-by: Shannon Zhao shannon.z...@linaro.org

Applied

 ---
  fsdev/virtfs-proxy-helper.c | 3 +++
  1 file changed, 3 insertions(+)

 diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
 index c1da2d7..bf2e5f3 100644
 --- a/fsdev/virtfs-proxy-helper.c
 +++ b/fsdev/virtfs-proxy-helper.c
 @@ -262,6 +262,9 @@ static int send_status(int sockfd, struct iovec *iovec, 
 int status)
   */
  msg_size = proxy_marshal(iovec, 0, ddd, header.type,
   header.size, status);
 +if (msg_size  0) {
 +return msg_size;
 +}
  retval = socket_write(sockfd, iovec-iov_base, msg_size);
  if (retval  0) {
  return retval;
 -- 
 1.8.3.1

Re: [Qemu-devel] [PATCH v2] fsdev/virtfs-proxy-helper: Fix possible overflow

[Aneesh Kumar K.V]

Shannon Zhao zhaoshengl...@huawei.com writes:

 It's detected by coverity. As max of sockaddr_un.sun_path is
 sizeof(helper.sun_path), should check the length of source
 and use strncpy instead of strcpy.

updated such that,

The socket name specified should fit in the sockadd_un.sun_path. If not
abort.

with that applied.


 Signed-off-by: Shannon Zhao zhaoshengl...@huawei.com
 Signed-off-by: Shannon Zhao shannon.z...@linaro.org
 ---
 v1-v2: Still use strcpy [Paolo]
 ---
  fsdev/virtfs-proxy-helper.c | 1 +
  1 file changed, 1 insertion(+)

 diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
 index bf2e5f3..13fe032 100644
 --- a/fsdev/virtfs-proxy-helper.c
 +++ b/fsdev/virtfs-proxy-helper.c
 @@ -738,6 +738,7 @@ static int proxy_socket(const char *path, uid_t uid, 
 gid_t gid)
  return -1;
  }
  
 +g_assert(strlen(path)  sizeof(proxy.sun_path));
  sock = socket(AF_UNIX, SOCK_STREAM, 0);
  if (sock  0) {
  do_perror(socket);
 -- 
 1.8.3.1

[Qemu-devel] [PATCH 1/6] 9pfs-local: simplify/optimize local_mapped_attr_path()

[Aneesh Kumar K.V]

From: Michael Tokarev m...@tls.msk.ru

Omit one unnecessary memory allocation for components
of the path and create the resulting path directly given
lengths of the components.

Do not use basename(3) because there are 2 versions of
this function which differs when argument ends with
slash character, use strrchr() instead so we have
consistent result.  This also makes sure the function
will do the right thing in corner cases (eg, empty
pathname is given), when basename(3) return entirely
another string.

Signed-off-by: Michael Tokarev m...@tls.msk.ru
Signed-off-by: Aneesh Kumar K.V aneesh.ku...@linux.vnet.ibm.com
---
 hw/9pfs/virtio-9p-local.c | 24 +++-
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
index d05c91779f2c..84efb31cfec4 100644
--- a/hw/9pfs/virtio-9p-local.c
+++ b/hw/9pfs/virtio-9p-local.c
@@ -45,19 +45,17 @@
 
 static char *local_mapped_attr_path(FsContext *ctx, const char *path)
 {
-char *dir_name;
-char *tmp_path = g_strdup(path);
-char *base_name = basename(tmp_path);
-char *buffer;
-
-/* NULL terminate the directory */
-dir_name = tmp_path;
-*(base_name - 1) = '\0';
-
-buffer = g_strdup_printf(%s/%s/%s/%s,
- ctx-fs_root, dir_name, VIRTFS_META_DIR, base_name);
-g_free(tmp_path);
-return buffer;
+int dirlen;
+const char *name = strrchr(path, '/');
+if (name) {
+dirlen = name - path;
+++name;
+} else {
+name = path;
+dirlen = 0;
+}
+return g_strdup_printf(%s/%.*s/%s/%s, ctx-fs_root,
+   dirlen, path, VIRTFS_META_DIR, name);
 }
 
 static FILE *local_fopen(const char *path, const char *mode)
-- 
2.1.0

[Qemu-devel] [PATCH 2/6] 9pfs-proxy: tiny cleanups in proxy_pwritev and proxy_preadv

[Aneesh Kumar K.V]

From: Michael Tokarev m...@tls.msk.ru

Don't compare syscall return with -1, use 0 condition.
Don't introduce useless local variables when we already
have similar variable
Rename local variable to be consistent with other usages
Finally make the two methods, read and write, to be similar to each other

Signed-off-by: Michael Tokarev m...@tls.msk.ru
Signed-off-by: Aneesh Kumar K.V aneesh.ku...@linux.vnet.ibm.com
---
 hw/9pfs/virtio-9p-proxy.c | 18 --
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/hw/9pfs/virtio-9p-proxy.c b/hw/9pfs/virtio-9p-proxy.c
index 59c7445deab9..6bb191ee6ab8 100644
--- a/hw/9pfs/virtio-9p-proxy.c
+++ b/hw/9pfs/virtio-9p-proxy.c
@@ -693,16 +693,16 @@ static ssize_t proxy_preadv(FsContext *ctx, 
V9fsFidOpenState *fs,
 const struct iovec *iov,
 int iovcnt, off_t offset)
 {
+ssize_t ret;
 #ifdef CONFIG_PREADV
-return preadv(fs-fd, iov, iovcnt, offset);
+ret = preadv(fs-fd, iov, iovcnt, offset);
 #else
-int err = lseek(fs-fd, offset, SEEK_SET);
-if (err == -1) {
-return err;
-} else {
-return readv(fs-fd, iov, iovcnt);
+ret = lseek(fs-fd, offset, SEEK_SET);
+if (ret = 0) {
+ret = readv(fs-fd, iov, iovcnt);
 }
 #endif
+return ret;
 }
 
 static ssize_t proxy_pwritev(FsContext *ctx, V9fsFidOpenState *fs,
@@ -714,10 +714,8 @@ static ssize_t proxy_pwritev(FsContext *ctx, 
V9fsFidOpenState *fs,
 #ifdef CONFIG_PREADV
 ret = pwritev(fs-fd, iov, iovcnt, offset);
 #else
-int err = lseek(fs-fd, offset, SEEK_SET);
-if (err == -1) {
-return err;
-} else {
+ret = lseek(fs-fd, offset, SEEK_SET);
+if (ret = 0) {
 ret = writev(fs-fd, iov, iovcnt);
 }
 #endif
-- 
2.1.0

Re: [Qemu-devel] [PATCH] hw/9pfs/virtio-9p-proxy: Fix possible overflow

[Aneesh Kumar K.V]

Shannon Zhao zhaoshengl...@huawei.com writes:

 It's detected by coverity. As max of sockaddr_un.sun_path is
 sizeof(helper.sun_path), should check the length of source
 and use strncpy instead of strcpy.

 Signed-off-by: Shannon Zhao zhaoshengl...@huawei.com
 Signed-off-by: Shannon Zhao shannon.z...@linaro.org
 ---
  hw/9pfs/virtio-9p-proxy.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

 diff --git a/hw/9pfs/virtio-9p-proxy.c b/hw/9pfs/virtio-9p-proxy.c
 index 59c7445..fb1ab7b 100644
 --- a/hw/9pfs/virtio-9p-proxy.c
 +++ b/hw/9pfs/virtio-9p-proxy.c
 @@ -1102,12 +1102,13 @@ static int connect_namedsocket(const char *path)
  int sockfd, size;
  struct sockaddr_un helper;
  
 +g_assert(strlen(path)  sizeof(helper.sun_path));

Since we are doing this from within Qemu, I did the below and folded
that into other sockadd_un.sun_path size checking patch.

diff --git a/hw/9pfs/virtio-9p-proxy.c b/hw/9pfs/virtio-9p-proxy.c
index 6bb191ee6ab8..71b6198bbd22 100644
--- a/hw/9pfs/virtio-9p-proxy.c
+++ b/hw/9pfs/virtio-9p-proxy.c
@@ -1100,6 +1100,10 @@ static int connect_namedsocket(const char *path)
 int sockfd, size;
 struct sockaddr_un helper;
 
+if (strlen(path) = sizeof(helper.sun_path)) {
+fprintf(stderr, Socket name too large\n);
+return -1;
+}
 sockfd = socket(AF_UNIX, SOCK_STREAM, 0);
 if (sockfd  0) {
 fprintf(stderr, failed to create socket: %s\n, strerror(errno));


Let me know if that is ok for you.
 
  sockfd = socket(AF_UNIX, SOCK_STREAM, 0);
  if (sockfd  0) {
  fprintf(stderr, failed to create socket: %s\n, strerror(errno));
  return -1;
  }
 -strcpy(helper.sun_path, path);
 +strncpy(helper.sun_path, path, sizeof(helper.sun_path));
  helper.sun_family = AF_UNIX;
  size = strlen(helper.sun_path) + sizeof(helper.sun_family);
  if (connect(sockfd, (struct sockaddr *)helper, size)  0) {
 -- 
 1.8.3.1

Re: [Qemu-devel] [PATCH v2] virtio: Fix memory leaks reported by Coverity

[Aneesh Kumar K.V]

Stefan Weil s...@weilnetz.de writes:

 All four leaks are similar, so fix them in one patch.

Ok had to spent some time to figure out which was the path that was not
freeing memory. So added extra information to commit message.

Success path was not doing memory free.

Applied.


 Signed-off-by: Stefan Weil s...@weilnetz.de
 ---

 v1 only fixed one of those leaks.
 v2 fixes all similar leaks.

  hw/9pfs/virtio-9p-local.c |   28 
  1 file changed, 8 insertions(+), 20 deletions(-)

 diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
 index d05c917..d66abcd 100644
 --- a/hw/9pfs/virtio-9p-local.c
 +++ b/hw/9pfs/virtio-9p-local.c
 @@ -488,7 +488,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath 
 *dir_path,
  int err = -1;
  int serrno = 0;
  V9fsString fullname;
 -char *buffer;
 +char *buffer = NULL;
  
  v9fs_string_init(fullname);
  v9fs_string_sprintf(fullname, %s/%s, dir_path-data, name);
 @@ -499,7 +499,6 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath 
 *dir_path,
  buffer = rpath(fs_ctx, path);
  err = mknod(buffer, SM_LOCAL_MODE_BITS|S_IFREG, 0);
  if (err == -1) {
 -g_free(buffer);
  goto out;
  }
  err = local_set_xattr(buffer, credp);
 @@ -512,7 +511,6 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath 
 *dir_path,
  buffer = rpath(fs_ctx, path);
  err = mknod(buffer, SM_LOCAL_MODE_BITS|S_IFREG, 0);
  if (err == -1) {
 -g_free(buffer);
  goto out;
  }
  err = local_set_mapped_file_attr(fs_ctx, path, credp);
 @@ -525,7 +523,6 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath 
 *dir_path,
  buffer = rpath(fs_ctx, path);
  err = mknod(buffer, credp-fc_mode, credp-fc_rdev);
  if (err == -1) {
 -g_free(buffer);
  goto out;
  }
  err = local_post_create_passthrough(fs_ctx, path, credp);
 @@ -539,8 +536,8 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath 
 *dir_path,
  err_end:
  remove(buffer);
  errno = serrno;
 -g_free(buffer);
  out:
 +g_free(buffer);
  v9fs_string_free(fullname);
  return err;
  }
 @@ -552,7 +549,7 @@ static int local_mkdir(FsContext *fs_ctx, V9fsPath 
 *dir_path,
  int err = -1;
  int serrno = 0;
  V9fsString fullname;
 -char *buffer;
 +char *buffer = NULL;
  
  v9fs_string_init(fullname);
  v9fs_string_sprintf(fullname, %s/%s, dir_path-data, name);
 @@ -563,7 +560,6 @@ static int local_mkdir(FsContext *fs_ctx, V9fsPath 
 *dir_path,
  buffer = rpath(fs_ctx, path);
  err = mkdir(buffer, SM_LOCAL_DIR_MODE_BITS);
  if (err == -1) {
 -g_free(buffer);
  goto out;
  }
  credp-fc_mode = credp-fc_mode|S_IFDIR;
 @@ -576,7 +572,6 @@ static int local_mkdir(FsContext *fs_ctx, V9fsPath 
 *dir_path,
  buffer = rpath(fs_ctx, path);
  err = mkdir(buffer, SM_LOCAL_DIR_MODE_BITS);
  if (err == -1) {
 -g_free(buffer);
  goto out;
  }
  credp-fc_mode = credp-fc_mode|S_IFDIR;
 @@ -590,7 +585,6 @@ static int local_mkdir(FsContext *fs_ctx, V9fsPath 
 *dir_path,
  buffer = rpath(fs_ctx, path);
  err = mkdir(buffer, credp-fc_mode);
  if (err == -1) {
 -g_free(buffer);
  goto out;
  }
  err = local_post_create_passthrough(fs_ctx, path, credp);
 @@ -604,8 +598,8 @@ static int local_mkdir(FsContext *fs_ctx, V9fsPath 
 *dir_path,
  err_end:
  remove(buffer);
  errno = serrno;
 -g_free(buffer);
  out:
 +g_free(buffer);
  v9fs_string_free(fullname);
  return err;
  }
 @@ -659,7 +653,7 @@ static int local_open2(FsContext *fs_ctx, V9fsPath 
 *dir_path, const char *name,
  int err = -1;
  int serrno = 0;
  V9fsString fullname;
 -char *buffer;
 +char *buffer = NULL;
  
  /*
   * Mark all the open to not follow symlinks
 @@ -675,7 +669,6 @@ static int local_open2(FsContext *fs_ctx, V9fsPath 
 *dir_path, const char *name,
  buffer = rpath(fs_ctx, path);
  fd = open(buffer, flags, SM_LOCAL_MODE_BITS);
  if (fd == -1) {
 -g_free(buffer);
  err = fd;
  goto out;
  }
 @@ -690,7 +683,6 @@ static int local_open2(FsContext *fs_ctx, V9fsPath 
 *dir_path, const char *name,
  buffer = rpath(fs_ctx, path);
  fd = open(buffer, flags, SM_LOCAL_MODE_BITS);
  if (fd == -1) {
 -g_free(buffer);
  err = fd;
  goto out;
  }
 @@ -706,7 +698,6 @@ static int local_open2(FsContext *fs_ctx, V9fsPath 
 *dir_path, const char *name,
  buffer = rpath(fs_ctx, path);
  fd = open(buffer, flags, credp-fc_mode);
  if (fd == -1) {
 -g_free(buffer);
  err = fd;
  goto out;
  }
 @@ 

Re: [Qemu-devel] [PATCH] hw/9pfs/virtio-9p-posix-acl: Fix out-of-bounds access

[Aneesh Kumar K.V]

Shannon Zhao zhaoshengl...@huawei.com writes:

 It's detected by coverity. Fix out-of-bounds access of the function 
 mp_dacl_listxattr.

 Signed-off-by: Shannon Zhao zhaoshengl...@huawei.com
 Signed-off-by: Shannon Zhao shannon.z...@linaro.org
 ---
  hw/9pfs/virtio-9p-posix-acl.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/hw/9pfs/virtio-9p-posix-acl.c b/hw/9pfs/virtio-9p-posix-acl.c
 index 803d9d9..09dad07 100644
 --- a/hw/9pfs/virtio-9p-posix-acl.c
 +++ b/hw/9pfs/virtio-9p-posix-acl.c
 @@ -114,7 +114,7 @@ static ssize_t mp_dacl_listxattr(FsContext *ctx, const 
 char *path,
  }

  /* len includes the trailing NUL */
 -memcpy(value, ACL_ACCESS, len);
 +memcpy(value, ACL_DEFAULT, len);
  return 0;
  }

Applied.

Thanks
-aneesh

Re: [Qemu-devel] [PATCH v2] 9pfs-local: simplify/optimize local_mapped_attr_path()

[Aneesh Kumar K.V]

Michael Tokarev m...@tls.msk.ru writes:

 Omit one unnecessary memory allocation for components
 of the path and create the resulting path directly given
 lengths of the components.

 Do not use basename(3) because there are 2 versions of
 this function which differs when argument ends with
 slash character, use strrchr() instead so we have
 consistent result.  This also makes sure the function
 will do the right thing in corner cases (eg, empty
 pathname is given), when basename(3) return entirely
 another string.

 Signed-off-by: Michael Tokarev m...@tls.msk.ru

Applied.

Thanks
-aneesh

Re: [Qemu-devel] [PATCH v2] 9pfs-proxy: tiny cleanups in proxy_pwritev and proxy_preadv

[Aneesh Kumar K.V]

Michael Tokarev m...@tls.msk.ru writes:

 Don't compare syscall return with -1, use 0 condition.
 Don't introduce useless local variables when we already
 have similar variable
 Rename local variable to be consistent with other usages
 Finally make the two methods, read and write, to be similar to each other

 Signed-off-by: Michael Tokarev m...@tls.msk.ru

Applied.

Thanks
-aneesh

Re: [Qemu-devel] [PATCH 1/3] 9pfs-proxy: simplify error handling

[Aneesh Kumar K.V]

Michael Tokarev m...@tls.msk.ru writes:

 10.03.2015 20:41, Aneesh Kumar K.V пишет:
 Michael Tokarev m...@tls.msk.ru writes:
 
 08.03.2015 19:27, Aneesh Kumar K.V wrote:
 Michael Tokarev m...@tls.msk.ru writes:
 []
 Actually, after reading almost whole 9pfs and fsdev code, I can
 say with great confidence this code is nearly hopeless.

 Is that about the 9pfs-proxy code, or the rest of 9pfs. I understand

 Well. the marshal/unmarshal interface is in core code as far as
 I can see, and it is very fragile at best, as the below example of
 its usage shows.  I haven't dug deeper.  So far, it was only the
 9pfs proxy code.
 
 May be i am missing something here. Can you help me understand the
 issue. 
 
 static int v9fs_receive_status(V9fsProxy *proxy,
struct iovec *reply, int *status)
 ...
 proxy_unmarshal(reply, 0, dd, header.type, header.size);
 if (header.size != sizeof(int)) {
 *status = -ENOBUFS;
 }
 ...
 proxy_unmarshal(reply, PROXY_HDR_SZ, d, status);

 proxy_unmarshall(), for d element, expects an int32_t
 pointer.  Here we have int pointer, and compare its
 size with sizeof(int).  This is a generic problem of whole
 v9fs_(un)marshall interface, which is in the core of 9pfs...
 this is a status return, which is int32.
 
 Proxy helper do write sizeof(int) as a part of header response. So
 it read the header.size and check whether it is same as what it is
 expecting. If not it error out. So i am not sure what the issue you are
 listing here.

 Nope.  Both proxy helper and qemu uses v9fs_marshal/unmarshal interface
 which writes/reads a 4-byte uint32, which is the same size no matter
 which compiler/architecture you're on.  Not only these functions uses
 this size on wire, they also expect the same type of argument to the
 function.

That is better. So what you are looking at is only the proxy marshal and
unmarshal code. I was wondering what issues you were pointing out by

This is a generic problem of whole
   v9fs_(un)marshall interface, which is in the core of 9pfs...
   this is a status return, which is int32.


 However, as shown in the above example, at least some users of this
 interface uses int* instead of int32_t*, and sizeof(int) is compiler-
 and architecture-dependent, it is not fixed to 4 bytes (or else we'd
 not need a int32_t to start with, and life would be much simpler).

 The rule is that if you exchange data with some other process, unless
 it is a forked version of your own process, you should use some fixed
 interface, which is not dependent on some conditions.

 This is not some made-up situation really, even in this context: for
 example, in Debian we allow to install packages of different architectures
 on the same machine, and they work together.  We'we qemu-system-common
 package which contains the proxy helper, and eg qemu-system-x86 or
 qemu-system-arm, and it is perfectly valid to have 64bit qemu-system-common
 working together with 32bit qemu-system-mips, so that 32bit qemu binary
 will talk with 64bit proxy.

The code was originally written to be used on same os/cpu combination to
help virtfs usage via libvirt. AFAIU here we don't have issues
across 32 and 64 bit, because int remains 32 bit in both the
place. Which os/cpu combination are we looking w.r.t ILP64 ?. Please
note that the code is written primary on x86_64. So some assumptions
could be wrong. Rather than saying

   Oh well.  I've no idea how this code has been accepted.
   It is absolute crap.

it would be nice if we take up specific issues, w.r.t the os/cpu
combination where we are running into issues, we may be able to fix this
easily.



 Well, you can treat the proxy helper to be internal part of qemu which
 can't be intermixed between versions and architectures -- after all,
 little-endian-arch qemu can't talk to big-endian proxy since there's
 no byte swapping is going on -- it's a good idea to mention this in
 the package. ;)

That is correct. It was not written to be used in such scenarios


 But your v9fs_marshal interface supposed to be generic and should be
 used right to start with.  All args of v9fs_(un)marshall should carry
 the same types of arguments these functions expect, which are
 int32_t not int, int64_t not long or long long or timespec_t or any
 other special type and so on.  Or else BadThings will happen, and
 often you wont even notice.

Note: At this point i haven't picked any of the patches posted, so if
you have anything specific you want me to take up, please repost then
with correct version number (v1, v2...)

-aneesh

Re: [Qemu-devel] [PATCH] 9pfs-local: simplify/optimize local_mapped_attr_path()

[Aneesh Kumar K.V]

Michael Tokarev m...@tls.msk.ru writes:

 Omit one unnecessary memory allocation for components of the path
 and create the resulting path directly given lengths of the components.

 This uses (char*) cast because basename() accepts a char* without const,
 for unknown reason.  Maybe it is better to use strrchr(), but I'm not
 sure for various forms of directory component delimiter.

basename(3) says:
Both dirname() and basename() may modify the contents of path, so it may
be desirable to pass a copy when calling one of these functions



 And according to basename(3) manpage, it might return different strings
 for various corner cases, for example for empty argument it returns a
 string . which is obviously at some other address, so both the old
 code and new code will do a bad thing here.  So maybe it is actually
 better to use strrchr() after all.

 Signed-off-by: Michael Tokarev m...@tls.msk.ru
 ---
  hw/9pfs/virtio-9p-local.c | 17 -
  1 file changed, 4 insertions(+), 13 deletions(-)

 diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
 index d05c917..fddc242 100644
 --- a/hw/9pfs/virtio-9p-local.c
 +++ b/hw/9pfs/virtio-9p-local.c
 @@ -45,19 +45,10 @@
  
  static char *local_mapped_attr_path(FsContext *ctx, const char *path)
  {
 -char *dir_name;
 -char *tmp_path = g_strdup(path);
 -char *base_name = basename(tmp_path);
 -char *buffer;
 -
 -/* NULL terminate the directory */
 -dir_name = tmp_path;
 -*(base_name - 1) = '\0';
 -
 -buffer = g_strdup_printf(%s/%s/%s/%s,
 - ctx-fs_root, dir_name, VIRTFS_META_DIR, base_name);
 -g_free(tmp_path);
 -return buffer;
 +const char *name = basename((char*)path);

as per the man page basename could end up modifying 'path' and we don't
want to modify the input argument of the function.


 +int dirlen = name - path - 1;
 +return g_strdup_printf(%s/%.*s/%s/%s, ctx-fs_root,
 +   dirlen, path, VIRTFS_META_DIR, name);
  }
  
  static FILE *local_fopen(const char *path, const char *mode)

I am not sure whether we really need all these cleanups without really
fixing anyi specific issue.

-aneesh

Re: [Qemu-devel] [PATCH] 9pfs-proxy: tiny cleanups in proxy_pwritev and proxy_preadv

[Aneesh Kumar K.V]

Michael Tokarev m...@tls.msk.ru writes:

 Don't compare syscall return with -1, use 0 condition.
 Don't introduce useless local variables when we already
 have similar variable
 Rename local variable to be consistent with other usages

 Signed-off-by: Michael Tokarev m...@tls.msk.ru
 ---
  hw/9pfs/virtio-9p-proxy.c | 12 +---
  1 file changed, 5 insertions(+), 7 deletions(-)

 diff --git a/hw/9pfs/virtio-9p-proxy.c b/hw/9pfs/virtio-9p-proxy.c
 index 59c7445..a01804a 100644
 --- a/hw/9pfs/virtio-9p-proxy.c
 +++ b/hw/9pfs/virtio-9p-proxy.c
 @@ -696,9 +696,9 @@ static ssize_t proxy_preadv(FsContext *ctx, 
 V9fsFidOpenState *fs,
  #ifdef CONFIG_PREADV
  return preadv(fs-fd, iov, iovcnt, offset);
  #else
 -int err = lseek(fs-fd, offset, SEEK_SET);
 -if (err == -1) {
 -return err;
 +int ret = lseek(fs-fd, offset, SEEK_SET);
 +if (err  0)
 +return ret;
  } else {
  return readv(fs-fd, iov, iovcnt);
  }

I am not sure this is needed, whether to use int err or int ret as the
variable name is simply a matter of preference for the author. Also did
you compile test the above ? (note: int ret followed by a check with
variable name 'err').


 @@ -714,10 +714,8 @@ static ssize_t proxy_pwritev(FsContext *ctx, 
 V9fsFidOpenState *fs,
  #ifdef CONFIG_PREADV
  ret = pwritev(fs-fd, iov, iovcnt, offset);
  #else
 -int err = lseek(fs-fd, offset, SEEK_SET);
 -if (err == -1) {
 -return err;
 -} else {
 +ret = lseek(fs-fd, offset, SEEK_SET);
 +if (ret = 0) {
  ret = writev(fs-fd, iov, iovcnt);
  }
  #endif

-aneesh

Re: [Qemu-devel] [PATCH 1/3] 9pfs-proxy: simplify error handling

[Aneesh Kumar K.V]

Michael Tokarev m...@tls.msk.ru writes:

 08.03.2015 19:27, Aneesh Kumar K.V wrote:
 Michael Tokarev m...@tls.msk.ru writes:
 []
 Actually, after reading almost whole 9pfs and fsdev code, I can
 say with great confidence this code is nearly hopeless.
 
 Is that about the 9pfs-proxy code, or the rest of 9pfs. I understand

 Well. the marshal/unmarshal interface is in core code as far as
 I can see, and it is very fragile at best, as the below example of
 its usage shows.  I haven't dug deeper.  So far, it was only the
 9pfs proxy code.

May be i am missing something here. Can you help me understand the
issue. 

 static int v9fs_receive_status(V9fsProxy *proxy,
struct iovec *reply, int *status)
 ...
 proxy_unmarshal(reply, 0, dd, header.type, header.size);
 if (header.size != sizeof(int)) {
 *status = -ENOBUFS;
 }
 ...
 proxy_unmarshal(reply, PROXY_HDR_SZ, d, status);

 proxy_unmarshall(), for d element, expects an int32_t
 pointer.  Here we have int pointer, and compare its
 size with sizeof(int).  This is a generic problem of whole
 v9fs_(un)marshall interface, which is in the core of 9pfs...
 this is a status return, which is int32.

Proxy helper do write sizeof(int) as a part of header response. So
it read the header.size and check whether it is same as what it is
expecting. If not it error out. So i am not sure what the issue you are
listing here.

-aneesh

Re: [Qemu-devel] [PATCH 2/7] 9pfs: Fix warnings from Sparse

[Aneesh Kumar K.V]

Stefan Weil s...@weilnetz.de writes:

 Sparse report:

 9pfs/virtio-9p.c:1953:9: warning: returning void-valued expression
 9pfs/virtio-9p-handle.c:143:5: warning: returning void-valued expression
 9pfs/virtio-9p-handle.c:160:5: warning: returning void-valued expression
 9pfs/virtio-9p-local.c:384:5: warning: returning void-valued expression
 9pfs/virtio-9p-local.c:415:5: warning: returning void-valued expression
 9pfs/virtio-9p-proxy.c:672:5: warning: returning void-valued expression
 9pfs/virtio-9p-proxy.c:689:5: warning: returning void-valued expression

 Cc: Michael S. Tsirkin m...@redhat.com
 Cc: Aneesh Kumar K.V aneesh.ku...@linux.vnet.ibm.com
 Signed-off-by: Stefan Weil s...@weilnetz.de

Reviewed-by: Aneesh Kumar K.V aneesh.ku...@linux.vnet.ibm.com

 ---
  hw/9pfs/virtio-9p-handle.c | 4 ++--
  hw/9pfs/virtio-9p-local.c  | 4 ++--
  hw/9pfs/virtio-9p-proxy.c  | 4 ++--
  hw/9pfs/virtio-9p.c| 3 ++-
  4 files changed, 8 insertions(+), 7 deletions(-)

 diff --git a/hw/9pfs/virtio-9p-handle.c b/hw/9pfs/virtio-9p-handle.c
 index 4b79cef..13eabb9 100644
 --- a/hw/9pfs/virtio-9p-handle.c
 +++ b/hw/9pfs/virtio-9p-handle.c
 @@ -140,7 +140,7 @@ static int handle_opendir(FsContext *ctx,
  
  static void handle_rewinddir(FsContext *ctx, V9fsFidOpenState *fs)
  {
 -return rewinddir(fs-dir);
 +rewinddir(fs-dir);
  }
  
  static off_t handle_telldir(FsContext *ctx, V9fsFidOpenState *fs)
 @@ -157,7 +157,7 @@ static int handle_readdir_r(FsContext *ctx, 
 V9fsFidOpenState *fs,
  
  static void handle_seekdir(FsContext *ctx, V9fsFidOpenState *fs, off_t off)
  {
 -return seekdir(fs-dir, off);
 +seekdir(fs-dir, off);
  }
  
  static ssize_t handle_preadv(FsContext *ctx, V9fsFidOpenState *fs,
 diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
 index a183eee..fec580b 100644
 --- a/hw/9pfs/virtio-9p-local.c
 +++ b/hw/9pfs/virtio-9p-local.c
 @@ -381,7 +381,7 @@ static int local_opendir(FsContext *ctx,
  
  static void local_rewinddir(FsContext *ctx, V9fsFidOpenState *fs)
  {
 -return rewinddir(fs-dir);
 +rewinddir(fs-dir);
  }
  
  static off_t local_telldir(FsContext *ctx, V9fsFidOpenState *fs)
 @@ -412,7 +412,7 @@ again:
  
  static void local_seekdir(FsContext *ctx, V9fsFidOpenState *fs, off_t off)
  {
 -return seekdir(fs-dir, off);
 +seekdir(fs-dir, off);
  }
  
  static ssize_t local_preadv(FsContext *ctx, V9fsFidOpenState *fs,
 diff --git a/hw/9pfs/virtio-9p-proxy.c b/hw/9pfs/virtio-9p-proxy.c
 index 59c7445..edab402 100644
 --- a/hw/9pfs/virtio-9p-proxy.c
 +++ b/hw/9pfs/virtio-9p-proxy.c
 @@ -669,7 +669,7 @@ static int proxy_opendir(FsContext *ctx,
  
  static void proxy_rewinddir(FsContext *ctx, V9fsFidOpenState *fs)
  {
 -return rewinddir(fs-dir);
 +rewinddir(fs-dir);
  }
  
  static off_t proxy_telldir(FsContext *ctx, V9fsFidOpenState *fs)
 @@ -686,7 +686,7 @@ static int proxy_readdir_r(FsContext *ctx, 
 V9fsFidOpenState *fs,
  
  static void proxy_seekdir(FsContext *ctx, V9fsFidOpenState *fs, off_t off)
  {
 -return seekdir(fs-dir, off);
 +seekdir(fs-dir, off);
  }
  
  static ssize_t proxy_preadv(FsContext *ctx, V9fsFidOpenState *fs,
 diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
 index 5861a5b..4964da0 100644
 --- a/hw/9pfs/virtio-9p.c
 +++ b/hw/9pfs/virtio-9p.c
 @@ -1950,7 +1950,8 @@ static void v9fs_write(void *opaque)
  
  err = pdu_unmarshal(pdu, offset, dqd, fid, off, count);
  if (err  0) {
 -return complete_pdu(s, pdu, err);
 +complete_pdu(s, pdu, err);
 +return;
  }
  offset += err;
  v9fs_init_qiov_from_pdu(qiov_full, pdu, offset, count, true);
 -- 
 2.1.4

Re: [Qemu-devel] [PATCH 1/5] 9pfs-proxy: simplify v9fs_request(), P1

[Aneesh Kumar K.V]

Michael Tokarev m...@tls.msk.ru writes:

 08.03.2015 19:39, Aneesh Kumar K.V wrote:
 Michael Tokarev m...@tls.msk.ru writes:
 
 This simplifies code in v9fs_request() a bit by replacing several
 ifs with a common variable check and rearranging error/cleanup
 code a bit.
 
 Is this -V2 of
 http://mid.gmane.org/b98f675750ef0535cab41225240db1657fc2fe00.1425678142.git@msgid.tls.msk.ru

 No, this one (simplify v9fs_reqeust(), P1) was a first version.
 The above URL points to the v2, a second version.

Please post an update patch series with the variable rename and the v9fs
simplification as two different patches.



 I am slightly confused with the patch series. It does split the patch as
 I wanted, but i am not sure which one is the latest, so that i can start
 applying them.

 That URL points to last.  I merged several small patches into larger ones.

 Initially I thought I just plug a small resource leak so the patch will
 be small.  But the more I understood the code, the bigger the changes
 were becoming.  So at some point it become unreasonable to keep small
 changes since they're related to bigger changes and since bigger changes
 touches the same code anyway.


Please keep the patches smaller. If needed i can fold them up when
merging the changes.

-aneesh

Re: [Qemu-devel] [PATCH 1/3] 9pfs-proxy: simplify error handling

[Aneesh Kumar K.V]

Michael Tokarev m...@tls.msk.ru writes:

 07.03.2015 23:37, Eric Blake wrote:
 On 03/06/2015 02:43 PM, Michael Tokarev wrote:
 All filesystem methods that call common v9fs_request() function
 also convert return value to errno.  Move this conversion to the
 common function and remove redundand error handling in methods.
 
 s/redundand/redundant/

 Heh.  Is this all that I can say about this patch? ;)

 Actually, after reading almost whole 9pfs and fsdev code, I can
 say with great confidence this code is nearly hopeless.

Is that about the 9pfs-proxy code, or the rest of 9pfs. I understand
that the error handling can definitely get some cleanup. I mentioned
that in my previous mail
mail. http://mid.gmane.org/87oav7iy5v@linux.vnet.ibm.com


 Patch 3 shows just one (huge) example.  There are so many issues
 with this code, I'm afraid I don't have know the words to express
 it.

 Again, patch 3 shows a good example.  Another example:

 static int v9fs_receive_status(V9fsProxy *proxy,
struct iovec *reply, int *status)
 ...
 proxy_unmarshal(reply, 0, dd, header.type, header.size);
 if (header.size != sizeof(int)) {
 *status = -ENOBUFS;
 }
 ...
 proxy_unmarshal(reply, PROXY_HDR_SZ, d, status);

 proxy_unmarshall(), for d element, expects an int32_t
 pointer.  Here we have int pointer, and compare its
 size with sizeof(int).  This is a generic problem of whole
 v9fs_(un)marshall interface, which is in the core of 9pfs...
 this is a status return, which is int32.

 Oh well.  I've no idea how this code has been accepted.
 It is absolute crap.


-aneesh

Re: [Qemu-devel] [PATCH 1/3] 9pfs-proxy: simplify error handling

[Aneesh Kumar K.V]

Michael Tokarev m...@tls.msk.ru writes:

 All filesystem methods that call common v9fs_request() function
 also convert return value to errno.  Move this conversion to the
 common function and remove redundand error handling in methods.

 I didn't remove local `retval' variable in simple functions to
 keep the code consistent.

 Also, proxy_truncate() seem to prefer zero successful return
 instead of returning whatever the helper returned, maybe this
 should be changed.

 This also removes (harmless) double call to v9fs_string_free()
 in proxy_mkdir(), and renames local variables in some functions
 for consistency.

Can you keep the variable rename as a separate patch. That will make it
easier to review. Let me know if you want me to do that for you.

-aneesh

Re: [Qemu-devel] [PATCH 1/5] 9pfs-proxy: simplify v9fs_request(), P1

[Aneesh Kumar K.V]

Michael Tokarev m...@tls.msk.ru writes:

 This simplifies code in v9fs_request() a bit by replacing several
 ifs with a common variable check and rearranging error/cleanup
 code a bit.

Is this -V2 of
http://mid.gmane.org/b98f675750ef0535cab41225240db1657fc2fe00.1425678142.git@msgid.tls.msk.ru

I am slightly confused with the patch series. It does split the patch as
I wanted, but i am not sure which one is the latest, so that i can start
applying them.


 Signet-off-by: Michael Tokarev m...@tls.msk.ru
 ---
  hw/9pfs/virtio-9p-proxy.c | 48 
 ---
  1 file changed, 20 insertions(+), 28 deletions(-)

 diff --git a/hw/9pfs/virtio-9p-proxy.c b/hw/9pfs/virtio-9p-proxy.c
 index 59c7445..f252fe4 100644
 --- a/hw/9pfs/virtio-9p-proxy.c
 +++ b/hw/9pfs/virtio-9p-proxy.c
 @@ -299,7 +299,7 @@ static int v9fs_request(V9fsProxy *proxy, int type,
  dev_t rdev;
  va_list ap;
  int size = 0;
 -int retval = 0;
 +int retval = 0, err;
  uint64_t offset;
  ProxyHeader header = { 0, 0};
  struct timespec spec[2];
 @@ -310,10 +310,11 @@ static int v9fs_request(V9fsProxy *proxy, int type,
  
  qemu_mutex_lock(proxy-mutex);
  
 -if (proxy-sockfd == -1) {
 +if (proxy-sockfd  0) {
  retval = -EIO;
 -goto err_out;
 +goto out;
  }
 +
  iovec = proxy-out_iovec;
  reply = proxy-in_iovec;
  va_start(ap, fmt);
 @@ -529,15 +530,15 @@ static int v9fs_request(V9fsProxy *proxy, int type,
  va_end(ap);
  
  if (retval  0) {
 -goto err_out;
 +goto out;
  }
  
  /* marshal the header details */
  proxy_marshal(iovec, 0, dd, header.type, header.size);
  header.size += PROXY_HDR_SZ;
  
 -retval = qemu_write_full(proxy-sockfd, iovec-iov_base, header.size);
 -if (retval != header.size) {
 +err = qemu_write_full(proxy-sockfd, iovec-iov_base, header.size);
 +if (err != header.size) {
  goto close_error;
  }
  
 @@ -548,9 +549,7 @@ static int v9fs_request(V9fsProxy *proxy, int type,
   * A file descriptor is returned as response for
   * T_OPEN,T_CREATE on success
   */
 -if (v9fs_receivefd(proxy-sockfd, retval)  0) {
 -goto close_error;
 -}
 +err = v9fs_receivefd(proxy-sockfd, retval);
  break;
  case T_MKNOD:
  case T_MKDIR:
 @@ -564,41 +563,34 @@ static int v9fs_request(V9fsProxy *proxy, int type,
  case T_REMOVE:
  case T_LSETXATTR:
  case T_LREMOVEXATTR:
 -if (v9fs_receive_status(proxy, reply, retval)  0) {
 -goto close_error;
 -}
 +err = v9fs_receive_status(proxy, reply, retval);
  break;
  case T_LSTAT:
  case T_READLINK:
  case T_STATFS:
  case T_GETVERSION:
 -if (v9fs_receive_response(proxy, type, retval, response)  0) {
 -goto close_error;
 -}
 +err = v9fs_receive_response(proxy, type, retval, response);
  break;
  case T_LGETXATTR:
  case T_LLISTXATTR:
  if (!size) {
 -if (v9fs_receive_status(proxy, reply, retval)  0) {
 -goto close_error;
 -}
 +err = v9fs_receive_status(proxy, reply, retval);
  } else {
 -if (v9fs_receive_response(proxy, type, retval, response)  0) {
 -goto close_error;
 -}
 +err = v9fs_receive_response(proxy, type, retval, response);
  }
  break;
  }
  
 -err_out:
 -qemu_mutex_unlock(proxy-mutex);
 -return retval;
 -
 +if (err  0) {
  close_error:
 -close(proxy-sockfd);
 -proxy-sockfd = -1;
 +close(proxy-sockfd);
 +proxy-sockfd = -1;
 +retval = -EIO;
 +}
 +
 +out:
  qemu_mutex_unlock(proxy-mutex);
 -return -EIO;
 +return retval;
  }
  
  static int proxy_lstat(FsContext *fs_ctx, V9fsPath *fs_path, struct stat 
 *stbuf)
 -- 
 2.1.4

Re: [Qemu-devel] 9pfs-local: open2() deletes existing data?

[Aneesh Kumar K.V]

Michael Tokarev m...@tls.msk.ru writes:

 I was looking at various interesting functions in hw/9pfs/virtio-9p-local.c
 and noticed local_open2() which basically tries to open a file in a
 filesystem, and if that is successful, it tries to set file credentials
 using a configured mechanism, and if that fails, it deletes the file.

 Now I wonder what happens if we tried to open an existing file but was
 not able to set credentials for whatever reason -- eg, because the
 underlying filesystem does not support xattrs, or whatever.  It looks
 to me that we will remove the user file!

 If that's the case, it looks like it is a very serious bug...

That callback is used for create. What is used for open is local_open()

-aneesh

Re: [Qemu-devel] 9pfs-proxy: -retval vs errno vs -1

[Aneesh Kumar K.V]

Michael Tokarev m...@tls.msk.ru writes:

 Another interesting tidbit is in hw/9pfs/virtio-9p-proxy.c.

 All filesystem methods use common v9fs_request() function,
 which returns -errno.  So far so good.

 Now, *all* places which call this function, does this:

 retval = v9fs_request(...);
 if (retval  0) {
 errno = -retval;
 }
 return retval;

 and *some* does this:

 retval = v9fs_request(...);
 if (retval  0) {
 errno = -retval;
 retval = -1;
 }
 return retval;

We should be able to drop that retval = -1;

-aneesh

[Qemu-devel] [PATCH] target-ppc: Use right page size with hash table lookup

[Aneesh Kumar K.V]

We look at two sizes specified in ISA (4K, 64K). If not found matching,
we consider it 16MB.

Without this patch we would fail to lookup address above 16MB range.
Below 16MB happened to work before because the kernel have a liner
mapping and we always looked up hash for 0xc000. The
actual real address was computed by using the 16MB offset
with the real address found with the above hash.

Without Fix:
(gdb) x/16x 0xc100
0xc100 list_entries+453208:   Cannot access memory at address 
0xc100
(gdb)

With Fix:
(gdb)  x/16x 0xc100
0xc100 list_entries+453208:   0x  0x  
0x  0x
0xc110 list_entries+453224:   0x  0x  
0x  0x
0xc120 list_entries+453240:   0x  0x  
0x  0x
0xc130 list_entries+453256:   0x  0x  
0x  0x

Signed-off-by: Aneesh Kumar K.V aneesh.ku...@linux.vnet.ibm.com
---
 target-ppc/cpu.h|  1 +
 target-ppc/mmu-hash64.c | 37 ++---
 target-ppc/mmu-hash64.h |  3 +++
 3 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index aae33a9237d7..b706b9fdf6c3 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -45,6 +45,7 @@
 # define TARGET_VIRT_ADDR_SPACE_BITS 64
 #endif
 
+#define TARGET_PAGE_BITS_64K 16
 #define TARGET_PAGE_BITS_16M 24
 
 #else /* defined (TARGET_PPC64) */
diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
index b0278c95e1d3..971751f7de89 100644
--- a/target-ppc/mmu-hash64.c
+++ b/target-ppc/mmu-hash64.c
@@ -388,6 +388,24 @@ static hwaddr ppc_hash64_pteg_search(CPUPPCState *env, 
hwaddr hash,
 return -1;
 }
 
+static uint64_t ppc_hash64_page_shift(ppc_slb_t *slb)
+{
+uint64_t epnshift;
+
+/* Page size according to the SLB, which we use to generate the
+ * EPN for hash table lookup..  When we implement more recent MMU
+ * extensions this might be different from the actual page size
+ * encoded in the PTE */
+if ((slb-vsid  SLB_VSID_LLP_MASK) == SLB_VSID_4K) {
+epnshift = TARGET_PAGE_BITS;
+} else if ((slb-vsid  SLB_VSID_LLP_MASK) == SLB_VSID_64K) {
+epnshift = TARGET_PAGE_BITS_64K;
+} else {
+epnshift = TARGET_PAGE_BITS_16M;
+}
+return epnshift;
+}
+
 static hwaddr ppc_hash64_htab_lookup(CPUPPCState *env,
  ppc_slb_t *slb, target_ulong eaddr,
  ppc_hash_pte64_t *pte)
@@ -396,12 +414,7 @@ static hwaddr ppc_hash64_htab_lookup(CPUPPCState *env,
 hwaddr hash;
 uint64_t vsid, epnshift, epnmask, epn, ptem;
 
-/* Page size according to the SLB, which we use to generate the
- * EPN for hash table lookup..  When we implement more recent MMU
- * extensions this might be different from the actual page size
- * encoded in the PTE */
-epnshift = (slb-vsid  SLB_VSID_L)
-? TARGET_PAGE_BITS_16M : TARGET_PAGE_BITS;
+epnshift = ppc_hash64_page_shift(slb);
 epnmask = ~((1ULL  epnshift) - 1);
 
 if (slb-vsid  SLB_VSID_B) {
@@ -448,12 +461,14 @@ static hwaddr ppc_hash64_htab_lookup(CPUPPCState *env,
 static hwaddr ppc_hash64_pte_raddr(ppc_slb_t *slb, ppc_hash_pte64_t pte,
target_ulong eaddr)
 {
+hwaddr mask;
+int target_page_bits;
 hwaddr rpn = pte.pte1  HPTE64_R_RPN;
-/* FIXME: Add support for SLLP extended page sizes */
-int target_page_bits = (slb-vsid  SLB_VSID_L)
-? TARGET_PAGE_BITS_16M : TARGET_PAGE_BITS;
-hwaddr mask = (1ULL  target_page_bits) - 1;
-
+/*
+ * We support 4K, 64K and 16M now
+ */
+target_page_bits = ppc_hash64_page_shift(slb);
+mask = (1ULL  target_page_bits) - 1;
 return (rpn  ~mask) | (eaddr  mask);
 }
 
diff --git a/target-ppc/mmu-hash64.h b/target-ppc/mmu-hash64.h
index 49e385db90fb..291750f3e5a5 100644
--- a/target-ppc/mmu-hash64.h
+++ b/target-ppc/mmu-hash64.h
@@ -37,6 +37,9 @@ void ppc_hash64_store_hpte(CPUPPCState *env, target_ulong 
index,
 #define SLB_VSID_C  0x0080ULL /* class */
 #define SLB_VSID_LP 0x0030ULL
 #define SLB_VSID_ATTR   0x0FFFULL
+#define SLB_VSID_LLP_MASK   (SLB_VSID_L | SLB_VSID_LP)
+#define SLB_VSID_4K 0xULL
+#define SLB_VSID_64K0x0110ULL
 
 /*
  * Hash page table definitions
-- 
2.1.0

[Qemu-devel] [PULL] VirtFS update

[Aneesh Kumar K.V]

The following changes since commit 30eaca3acdf17d7bcbd1213eb149c02037edfb0b:

  Merge remote-tracking branch 'remotes/spice/tags/pull-spice-20140902-1' into 
staging (2014-09-02 10:26:10 +0100)

are available in the git repository at:


  https://github.com/kvaneesh/qemu.git for-upstream

for you to fetch changes up to 840a1bf2832f5783d1070547fcb460216fc219e5:

  hw/9pfs: Don't return type from host in readdir on local 9p filesystem 
(2014-09-04 10:51:13 -0500)


Aneesh Kumar K.V (1):
  hw/9pfs: Use little-endian format for xattr values

Bastian Blank (1):
  hw/9pfs: Don't return type from host in readdir on local 9p filesystem

 hw/9pfs/virtio-9p-local.c | 49 +++
 1 file changed, 24 insertions(+), 25 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 2/2] hw/9pfs: Don't return type from host in readdir on local 9p filesystem

[Aneesh Kumar K.V]

From: Bastian Blank wa...@debian.org

When using mapped mode in 9pfs, readdir implementation
should not return file type in d_type from the host
readdir, instead, it should use the type stored in
the extended attributes.  Since d_type is optional
and reading ext attrs for every readdir is expensive,
it should be sufficient to just set d_type to DT_UNKNOWN,
so guest will know to look it up separately.

This is a -stable material.

Signed-off-by: Bastian Blank wa...@debian.org
Signed-off-by: Michael Tokarev m...@tls.msk.ru
Signed-off-by: Aneesh Kumar K.V aneesh.ku...@linux.vnet.ibm.com
---
 hw/9pfs/virtio-9p-local.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
index 2787ddbe1f5e..a183eee662b6 100644
--- a/hw/9pfs/virtio-9p-local.c
+++ b/hw/9pfs/virtio-9p-local.c
@@ -397,12 +397,15 @@ static int local_readdir_r(FsContext *ctx, 
V9fsFidOpenState *fs,
 
 again:
 ret = readdir_r(fs-dir, entry, result);
-if (ctx-export_flags  V9FS_SM_MAPPED_FILE) {
+if (ctx-export_flags  V9FS_SM_MAPPED) {
+entry-d_type = DT_UNKNOWN;
+} else if (ctx-export_flags  V9FS_SM_MAPPED_FILE) {
 if (!ret  *result != NULL 
 !strcmp(entry-d_name, VIRTFS_META_DIR)) {
 /* skp the meta data directory */
 goto again;
 }
+entry-d_type = DT_UNKNOWN;
 }
 return ret;
 }
-- 
1.9.1

[Qemu-devel] [PATCH 1/2] hw/9pfs: Use little-endian format for xattr values

[Aneesh Kumar K.V]

With security_model=mapped-xattr, we encode the uid,gid and other file
attributes as extended attributes of the file. We save them under
user.virtfs.* namespace.

Use little-endian encoding for on-disk values. This enables us to export
the same directory from both little-endian and big-endian hosts.

NOTE: This will break big-endian host that have virtFS exports
using security model mapped-xattr. They will have to use external tools
to convert the xattr to little-endian format.

Signed-off-by: Aneesh Kumar K.V aneesh.ku...@linux.vnet.ibm.com
---
 hw/9pfs/virtio-9p-local.c | 44 
 1 file changed, 20 insertions(+), 24 deletions(-)

diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
index 3b0b6a9b1d7d..2787ddbe1f5e 100644
--- a/hw/9pfs/virtio-9p-local.c
+++ b/hw/9pfs/virtio-9p-local.c
@@ -135,17 +135,17 @@ static int local_lstat(FsContext *fs_ctx, V9fsPath 
*fs_path, struct stat *stbuf)
 mode_t tmp_mode;
 dev_t tmp_dev;
 if (getxattr(buffer, user.virtfs.uid, tmp_uid, sizeof(uid_t))  0) {
-stbuf-st_uid = tmp_uid;
+stbuf-st_uid = le32_to_cpu(tmp_uid);
 }
 if (getxattr(buffer, user.virtfs.gid, tmp_gid, sizeof(gid_t))  0) {
-stbuf-st_gid = tmp_gid;
+stbuf-st_gid = le32_to_cpu(tmp_gid);
 }
 if (getxattr(buffer, user.virtfs.mode,
 tmp_mode, sizeof(mode_t))  0) {
-stbuf-st_mode = tmp_mode;
+stbuf-st_mode = le32_to_cpu(tmp_mode);
 }
 if (getxattr(buffer, user.virtfs.rdev, tmp_dev, sizeof(dev_t))  0) 
{
-stbuf-st_rdev = tmp_dev;
+stbuf-st_rdev = le64_to_cpu(tmp_dev);
 }
 } else if (fs_ctx-export_flags  V9FS_SM_MAPPED_FILE) {
 local_mapped_file_attr(fs_ctx, path, stbuf);
@@ -255,29 +255,29 @@ static int local_set_xattr(const char *path, FsCred 
*credp)
 int err;
 
 if (credp-fc_uid != -1) {
-err = setxattr(path, user.virtfs.uid, credp-fc_uid, sizeof(uid_t),
-0);
+uint32_t tmp_uid = cpu_to_le32(credp-fc_uid);
+err = setxattr(path, user.virtfs.uid, tmp_uid, sizeof(uid_t), 0);
 if (err) {
 return err;
 }
 }
 if (credp-fc_gid != -1) {
-err = setxattr(path, user.virtfs.gid, credp-fc_gid, sizeof(gid_t),
-0);
+uint32_t tmp_gid = cpu_to_le32(credp-fc_gid);
+err = setxattr(path, user.virtfs.gid, tmp_gid, sizeof(gid_t), 0);
 if (err) {
 return err;
 }
 }
 if (credp-fc_mode != -1) {
-err = setxattr(path, user.virtfs.mode, credp-fc_mode,
-sizeof(mode_t), 0);
+uint32_t tmp_mode = cpu_to_le32(credp-fc_mode);
+err = setxattr(path, user.virtfs.mode, tmp_mode, sizeof(mode_t), 0);
 if (err) {
 return err;
 }
 }
 if (credp-fc_rdev != -1) {
-err = setxattr(path, user.virtfs.rdev, credp-fc_rdev,
-sizeof(dev_t), 0);
+uint64_t tmp_rdev = cpu_to_le64(credp-fc_rdev);
+err = setxattr(path, user.virtfs.rdev, tmp_rdev, sizeof(dev_t), 0);
 if (err) {
 return err;
 }
@@ -630,21 +630,17 @@ static int local_fstat(FsContext *fs_ctx, int fid_type,
 mode_t tmp_mode;
 dev_t tmp_dev;
 
-if (fgetxattr(fd, user.virtfs.uid,
-  tmp_uid, sizeof(uid_t))  0) {
-stbuf-st_uid = tmp_uid;
+if (fgetxattr(fd, user.virtfs.uid, tmp_uid, sizeof(uid_t))  0) {
+stbuf-st_uid = le32_to_cpu(tmp_uid);
 }
-if (fgetxattr(fd, user.virtfs.gid,
-  tmp_gid, sizeof(gid_t))  0) {
-stbuf-st_gid = tmp_gid;
+if (fgetxattr(fd, user.virtfs.gid, tmp_gid, sizeof(gid_t))  0) {
+stbuf-st_gid = le32_to_cpu(tmp_gid);
 }
-if (fgetxattr(fd, user.virtfs.mode,
-  tmp_mode, sizeof(mode_t))  0) {
-stbuf-st_mode = tmp_mode;
+if (fgetxattr(fd, user.virtfs.mode, tmp_mode, sizeof(mode_t))  0) {
+stbuf-st_mode = le32_to_cpu(tmp_mode);
 }
-if (fgetxattr(fd, user.virtfs.rdev,
-  tmp_dev, sizeof(dev_t))  0) {
-stbuf-st_rdev = tmp_dev;
+if (fgetxattr(fd, user.virtfs.rdev, tmp_dev, sizeof(dev_t))  0) {
+stbuf-st_rdev = le64_to_cpu(tmp_dev);
 }
 } else if (fs_ctx-export_flags  V9FS_SM_MAPPED_FILE) {
 errno = EOPNOTSUPP;
-- 
1.9.1

Re: [Qemu-devel] qemu-system-sparc64 hang (possibly virtio related?) with 2.1

[Aneesh Kumar K.V]

Stefan Hajnoczi stefa...@gmail.com writes:

 On Tue, Sep 02, 2014 at 02:12:45PM +0100, Mark Cave-Ayland wrote:
 Fortunately I can reproduce the issue with a debug-enabled build of
 qemu-system-sparc64, and I've posted a backtrace obtained during the hung
 state at http://www.ilande.co.uk/tmp/sparc64-gdb-bt.txt. I can't see
 anything too obvious, other than that the ppoll() could possibly be waiting
 for a bad file descriptor?

 The backtrace looks like a normal QEMU run.  Nothing obvious there.

 This suggests the QEMU monitor is still operational and the guest is
 still executing code.

 Does the I/O time out inside the guest?  Normally messages are printed
 in dmesg if I/O requests are pending for too long.

I am also finding this on ppc64. The system hang even with virtio-9p, so
it may not be virtio blk related. git bisect is complicated because few
commits in between cause other boot failures with ppc64.

-aneesh

Re: [Qemu-devel] qemu-system-sparc64 hang (possibly virtio related?) with 2.1

[Aneesh Kumar K.V]

Aneesh Kumar K.V aneesh.ku...@linux.vnet.ibm.com writes:

 Stefan Hajnoczi stefa...@gmail.com writes:

 On Tue, Sep 02, 2014 at 02:12:45PM +0100, Mark Cave-Ayland wrote:
 Fortunately I can reproduce the issue with a debug-enabled build of
 qemu-system-sparc64, and I've posted a backtrace obtained during the hung
 state at http://www.ilande.co.uk/tmp/sparc64-gdb-bt.txt. I can't see
 anything too obvious, other than that the ppoll() could possibly be waiting
 for a bad file descriptor?

 The backtrace looks like a normal QEMU run.  Nothing obvious there.

 This suggests the QEMU monitor is still operational and the guest is
 still executing code.

 Does the I/O time out inside the guest?  Normally messages are printed
 in dmesg if I/O requests are pending for too long.

 I am also finding this on ppc64. The system hang even with virtio-9p, so
 it may not be virtio blk related. git bisect is complicated because few
 commits in between cause other boot failures with ppc64.

The bad commit is:

git show cc943c36faa192cd4b32af8fe5edb31894017d35
commit cc943c36faa192cd4b32af8fe5edb31894017d35
Author: Jan Kiszka jan.kis...@siemens.com
Date:   Sun Jul 27 09:08:29 2014 +0200

pci: Use bus master address space for delivering MSI/MSI-X messages


git bisect log
git bisect start
# good: [5149e557d786ab83748588c9b1b13b43c81af6ab] Open 2.1 development tree
git bisect good 5149e557d786ab83748588c9b1b13b43c81af6ab
# bad: [246ad8e69be03d2774401adf63d7e1da8df166ac] hw/9pfs: Don't return type 
from host in readdir on local 9p filesystem
git bisect bad 246ad8e69be03d2774401adf63d7e1da8df166ac
# good: [d1a721ab816d1b954c0988aafdec4e109b953a9f] target-ppc: Add POWER8's TIR 
SPR
git bisect good d1a721ab816d1b954c0988aafdec4e109b953a9f
# good: [85d1277e668106294d134a101729c6f36289da1a] virtio: move common virtio 
properties to bus class device
git bisect good 85d1277e668106294d134a101729c6f36289da1a
# good: [86298845e127365e8a5b7419a5ee9039bbd1837f] qtest: Adding qtest_memset 
and qmemset.
git bisect good 86298845e127365e8a5b7419a5ee9039bbd1837f
# bad: [33886ebeec0c0ff6253a49253fae0db44c9ed0f3] Merge remote-tracking branch 
'remotes/kevin/tags/for-upstream' into staging
git bisect bad 33886ebeec0c0ff6253a49253fae0db44c9ed0f3
# bad: [8e6e2c2ae7a81f625cf1cb320891d5270e277548] Merge remote-tracking branch 
'remotes/qmp-unstable/queue/qmp' into staging
git bisect bad 8e6e2c2ae7a81f625cf1cb320891d5270e277548
# good: [39ba3bf69c4ef4d8a8b683ee7282efd25b3f01ff] qcow2: fix new_blocks 
double-free in alloc_refcount_block()
git bisect good 39ba3bf69c4ef4d8a8b683ee7282efd25b3f01ff
# good: [5edbdbcdf882e4220adc7dbf433351077cd1fbbc] ivshmem: check the value 
returned by fstat()
git bisect good 5edbdbcdf882e4220adc7dbf433351077cd1fbbc
# bad: [f2c85a2f36f57f155cda7bc9f7c42b44f1a2439e] Merge remote-tracking branch 
'remotes/mst/tags/for_upstream' into staging
git bisect bad f2c85a2f36f57f155cda7bc9f7c42b44f1a2439e
# bad: [988eba0f681bd4f82e9e02998da8106f165ed82c] pc-dimm: fix up error message
git bisect bad 988eba0f681bd4f82e9e02998da8106f165ed82c
# bad: [d209c7440a642ba08bbb0f13e22390460d3661ed] hw/audio/intel-hda: Fix MSI 
capability address
git bisect bad d209c7440a642ba08bbb0f13e22390460d3661ed
# bad: [cc943c36faa192cd4b32af8fe5edb31894017d35] pci: Use bus master address 
space for delivering MSI/MSI-X messages
git bisect bad cc943c36faa192cd4b32af8fe5edb31894017d35
# good: [2d591ce2aeebf9620ff527c7946844a3122afeec] Merge remote-tracking branch 
'remotes/mdroth/qga-pull-2014-08-08' into staging
git bisect good 2d591ce2aeebf9620ff527c7946844a3122afeec
# first bad commit: [cc943c36faa192cd4b32af8fe5edb31894017d35] pci: Use bus 
master address space for delivering MSI/MSI-X messages
-bash-4.2# 

Re: [Qemu-devel] qemu-system-sparc64 hang (possibly virtio related?) with 2.1

[Aneesh Kumar K.V]

Alexander Graf ag...@suse.de writes:

 On 03.09.14 23:21, Aneesh Kumar K.V wrote:
 Aneesh Kumar K.V aneesh.ku...@linux.vnet.ibm.com writes:
 
 Stefan Hajnoczi stefa...@gmail.com writes:

 On Tue, Sep 02, 2014 at 02:12:45PM +0100, Mark Cave-Ayland wrote:
 Fortunately I can reproduce the issue with a debug-enabled build of
 qemu-system-sparc64, and I've posted a backtrace obtained during the hung
 state at http://www.ilande.co.uk/tmp/sparc64-gdb-bt.txt. I can't see
 anything too obvious, other than that the ppoll() could possibly be 
 waiting
 for a bad file descriptor?

 The backtrace looks like a normal QEMU run.  Nothing obvious there.

 This suggests the QEMU monitor is still operational and the guest is
 still executing code.

 Does the I/O time out inside the guest?  Normally messages are printed
 in dmesg if I/O requests are pending for too long.

 I am also finding this on ppc64. The system hang even with virtio-9p, so
 it may not be virtio blk related. git bisect is complicated because few
 commits in between cause other boot failures with ppc64.
 
 The bad commit is:
 
 git show cc943c36faa192cd4b32af8fe5edb31894017d35
 commit cc943c36faa192cd4b32af8fe5edb31894017d35
 Author: Jan Kiszka jan.kis...@siemens.com
 Date:   Sun Jul 27 09:08:29 2014 +0200
 
 pci: Use bus master address space for delivering MSI/MSI-X messages

 Does spapr_pci: map the MSI window in each PHB from Greg fix this for
 you on ppc64?


Yes that patch fixed the issue for me.

Thanks,
-aneesh

Re: [Qemu-devel] reporting 9pfs init errors

[Aneesh Kumar K.V]

Michael Tokarev m...@tls.msk.ru writes:

 Hello.

 I've a bugreport against debian qemu package which basically
 states that 9pfs does not work.  After some digging it turned
 out to be error reporting problem, plain and simple.  The
 error message is:

 qemu-system-x86_64: -device 
 virtio-9p-pci,id=fs0,fsdev=fsdev-fs0,mount_tag=pwk,bus=pci.0,addr=0x7: 
 Virtio-9p Failed to initialize fs-driver with id:fsdev-fs0 and export 
 path:/home/stevie/Documents/PWK
 qemu-system-x86_64: -device 
 virtio-9p-pci,id=fs0,fsdev=fsdev-fs0,mount_tag=pwk,bus=pci.0,addr=0x7: Device 
 initialization failed.
 qemu-system-x86_64: -device 
 virtio-9p-pci,id=fs0,fsdev=fsdev-fs0,mount_tag=pwk,bus=pci.0,addr=0x7: Device 
 'virtio-9p-pci' could not be initialized

 and the actual problem is that the path is not accessible from
 libvirt-spawned qemu due to permission denied (EPERM) error
 returned from statfs() call.


I have run into that issue once and really wanted to improve the error
handling during startup. I guess we have better error handling during
runtime, ie, file system related errors are correctly mapped and
send to guest. We could definitely imporve our init error handling. 


-aneesh

Re: [Qemu-devel] 9p mapped-* security model infos are architecture-specific

[Aneesh Kumar K.V]

Michael Tokarev m...@tls.msk.ru writes:

 I haven't noticed this email - which is almost a month old now - until today.
 So replying now...

 30.07.2014 21:43, Aneesh Kumar K.V wrote:
 Aneesh Kumar K.V aneesh.ku...@linux.vnet.ibm.com writes:
 
 Michael Tokarev m...@tls.msk.ru writes:

 Apparently the the mapped-* security models results in a raw bytes
 being dumped to host without any architecture normalization (in
 host byte order).  This may even lead to security issues in guest
 when the same files are served from another host for example.

 This bug has been initially submitted against debian qemu package, see
 http://bugs.debian.org/755740


 Thanks for reporting the bug. Yes we do have issue with
 mapped-xattr. But mapped-file should be ok. We record the uid/gid as
 string in the file.
 
 What would be the best way to fix this in a backward compatible way ?
 Considering most of the users will be little endian host, we could do always
 store in little endian format which of-course will break big-endian
 hosts. We could possibly ask them to update xattr using external tools ?

 If there's no way to _detect_ the used format (maybe doing some guessing, --
 if that's possible to do in a reliable way, it should be good), that's
 one of 2 possible options as I see it: that or introduce a new format
 entirely, maybe with another attribute name.

 It might not be even required to use an external tool for conversion.
 Again, if qemu is able to detect wrong endiannes, it might just
 update things itself, or print a warning and switch to an old format,
 or something like that.

I was not able to come up with a way to detect wrong endianness. 


 But the guessing idea might not be as bad really.  I haven't looked
 closely which information is stored in there, -- but it is possible
 that some fields should have zeros in some bytes for example, and
 if these aren't zero but becomes zeros after endianness conversion
 that might be a good indicator.


No, they are 32 bit numbers and we can't make any assumptions w.r.t
upper half/lower half being zero



 I'm not sure the runtime code should be able to work with both formats
 at the same time.  Actually, I'm not sure this is a big issue to
 start with -- indeed, you said it already, majority of users of 9pfs
 should be little endian hosts, -- are there any big endian hosts
 using this, at all? :)  How about trying to detect (preferrable at
 init time) and refusing to start if old/wrong format is detected.

 Maybe have a compile-time define to use native or little endian
 format is a good idea too.


That would confuse further. It also impact the interoperability of
export path across different build of qemus.


 Bastian, since you discovered this issue, you might be using
 a host with uncommon endianness, what do you think?


-aneesh